Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-47688][CORE] Support three methods of the log concatenation in the structured logging framework #45813

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Apr 2, 2024

What changes were proposed in this pull request?

The pr aims to support three methods of the log concatenation in the structured logging framework. eg:

  • 1.log"... MDC(...) $normal_variables ..."
log"Driver terminated with exit code ${MDC(EXIT_CODE, exitCode)}! Shutting down. $remoteAddress"
  • 2.s"... $normal_variables ..." ++ log"... MDC(...) ..."
log"Driver terminated with exit code ${MDC(EXIT_CODE, exitCode)}! Shutting down." ++ s"$remoteAddress"
  • 3.log"... MDC(...) ..." ++ s"... $normal_variables ..."
s"$remoteAddress" ++ log"Driver terminated with exit code ${MDC(EXIT_CODE, exitCode)}! Shutting down."

Why are the changes needed?

Sometimes we just want to make some simple variable replacements in the log text, without the need for MDC
At present, the above logs, compile will fail.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add new UT.

Was this patch authored or co-authored using generative AI tooling?

No.

@panbingkun
Copy link
Contributor Author

@panbingkun panbingkun changed the title [SPARK-47688][CORE] Support concatenation normal variables and MDC [SPARK-47688][CORE] Support three types of the log concatenation in the structured logging framework Apr 2, 2024
object MDC {

implicit class StringImprovements(val s: String) {
def ++(mdc: MessageWithContext): MessageWithContext = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name cannot be called + and does not take effect, so it is called ++

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, in String ++ String, ++ is an alias for contact, which sounds reasonable.
image

@panbingkun
Copy link
Contributor Author

Some of the comments were not added. I want to add them to another PR #45808 , after this is merged, as they should conflict during the merge, I can handle them all at once.

@panbingkun panbingkun changed the title [SPARK-47688][CORE] Support three types of the log concatenation in the structured logging framework [SPARK-47688][CORE] Support three methods of the log concatenation in the structured logging framework Apr 2, 2024
@@ -96,17 +109,21 @@ trait Logging {
}

implicit class LogStringContext(val sc: StringContext) {
def log(args: MDC*): MessageWithContext = {
def log(args: Any*): MessageWithContext = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The args here is intended to MDC. So that we can enforce all the variables to be MDC.

@gengliangwang
Copy link
Member

log"Driver terminated with exit code ${MDC(EXIT_CODE, exitCode)}! Shutting down. $remoteAddress"

In this case, we should convert remoteAddress as MDC as well.

@panbingkun
Copy link
Contributor Author

@gengliangwang
Do we need to support the second(Sting + MDC) and third(MDC + Sting) methods besides the first one?

@gengliangwang
Copy link
Member

Do we need to support the second(Sting + MDC) and third(MDC + Sting) methods besides the first one?

So by the end of this project, all the log entries containing variables will use logError(log"..."). logError(s"...") will not be allowed.
I am not sure about how easy to build such scala style check if we allow log"..." + str . If it is complicated, we can stay with MDC concat only.

@panbingkun
Copy link
Contributor Author

Do we need to support the second(Sting + MDC) and third(MDC + Sting) methods besides the first one?

So by the end of this project, all the log entries containing variables will use logError(log"..."). logError(s"...") will not be allowed. I am not sure about how easy to build such scala style check if we allow log"..." + str . If it is complicated, we can stay with MDC concat only.

Okay, regarding this issue, I will investigate it carefully again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants