[SPARK-47575][INFRA] Implement logWarning API in structured logging framework#45770
[SPARK-47575][INFRA] Implement logWarning API in structured logging framework#45770gengliangwang wants to merge 1 commit into
Conversation
|
@ueshin Thanks for the review. |
|
|
||
| def msgWithMDC: LogEntry = log"Lost executor ${MDC(EXECUTOR_ID, "1")}." | ||
|
|
||
| def msgWithMDCAndException: LogEntry = log"Error in executor ${MDC(EXECUTOR_ID, "1")}." |
There was a problem hiding this comment.
so I'm a little confused by this, it has basically just text in it like msgWithMDC so where is the "AndException" part?
There was a problem hiding this comment.
When logging with an exception, the output will contain the field "exception".
{"ts":"2024-04-01T09:43:18.892-0700","level":"ERROR","msg":"Error in executor 1.","context":{"executor_id":"1"},"exception":{"class":"java.lang.RuntimeException","msg":"OOM","stacktrace":"..."},"logger":"org.apache.spark.util.StructuredLoggingSuite"}
There was a problem hiding this comment.
Ok but the function itself doesn't have an exception in it, this just seems to be another string with MDC which is just different text then msgWithMDC looking at where you use it I see the message copied into expectedPatternForMsgWithMDCAndException and then this passed into "Logging with MDC and Exception"... why isn't this just called msgWithMDC2 since this function itself doesn't take an exception or add any functionality related to an exception
There was a problem hiding this comment.
Yes, we can make it msgWithMDC2, or reuse msgWithMDC and remove msgWithMDCAndException.
3 participants
What changes were proposed in this pull request?
Implement logWarning API in structured logging framework. Also, refactor the logging test suites to reduce duplicated code.
Why are the changes needed?
To enhance Apache Spark's logging system by implementing structured logging.
Does this PR introduce any user-facing change?
No
How was this patch tested?
New unit tests
Was this patch authored or co-authored using generative AI tooling?
No