-
Notifications
You must be signed in to change notification settings - Fork 28k
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-48320][CORE][DOCS] Add structured logging guide to the scala and java doc #46634
Conversation
@@ -22,7 +22,7 @@ import java.util.Locale | |||
* All structured logging `keys` used in `MDC` must be extends `LogKey` | |||
*/ | |||
trait LogKey { | |||
val name: String = this.toString.toLowerCase(Locale.ROOT) | |||
def name: String = this.getClass.getSimpleName.stripSuffix("$").toLowerCase(Locale.ROOT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -104,8 +112,8 @@ public void testBasicMsgLogger() { | |||
Pair.of(Level.DEBUG, debugFn), | |||
Pair.of(Level.TRACE, traceFn)).forEach(pair -> { | |||
try { | |||
assert (captureLogOutput(pair.getRight()).matches( | |||
expectedPatternForBasicMsg(pair.getLeft()))); | |||
Assertions.assertTrue(captureLogOutput(pair.getRight()).matches( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace assert
with Assertions.assertTrue
@@ -140,15 +148,13 @@ public void testLoggerWithMDC() { | |||
Runnable errorFn = () -> logger().error(msgWithMDC, executorIDMDC); | |||
Runnable warnFn = () -> logger().warn(msgWithMDC, executorIDMDC); | |||
Runnable infoFn = () -> logger().info(msgWithMDC, executorIDMDC); | |||
Runnable debugFn = () -> logger().debug(msgWithMDC, executorIDMDC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After #46405, the following 2 lines of code are no longer needed.
@@ -22,7 +22,7 @@ import java.util.Locale | |||
* All structured logging `keys` used in `MDC` must be extends `LogKey` | |||
*/ | |||
trait LogKey { | |||
val name: String = this.toString.toLowerCase(Locale.ROOT) | |||
def name: String = this.getClass.getSimpleName.stripSuffix("$").toLowerCase(Locale.ROOT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we have a val or a lazy val to memorize the key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's use lazy val
, I have tested it and it works well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final CUSTOM_LOG_KEY CUSTOM_LOG_KEY = new CUSTOM_LOG_KEY(); | ||
} | ||
``` | ||
and use it in `java` code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have   here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* If you want to output logs in `scala code` through the structured log framework, you can define `custom LogKeys` as follows: | ||
|
||
```scala | ||
object CustomLogKeys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not mention CustomLogKeys here. It is not necessary, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
|
||
when your project has **depended on `scala`**, | ||
```scala | ||
object CustomLogKeys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the content seems duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to delete the description of the last case - 3?
common/utils/src/main/scala/org/apache/spark/internal/README.md
Outdated
Show resolved
Hide resolved
scala/java
doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace all references to external
and external system
- we dont want to set expectations that external system integrations are encouraged.
Keep it as custom integrations instead - if power users and frameworks want to leverage the integration, this would be the path for them as it is not explicitly private [spark]
.
I added a few cases below, but I would expect @gengliangwang to be better aware and give much better guidance than what I can of what else might need to be changed - as he is more actively reviewing the PR's :-)
common/utils/src/main/java/org/apache/spark/internal/SparkLogger.java
Outdated
Show resolved
Hide resolved
common/utils/src/main/java/org/apache/spark/internal/SparkLogger.java
Outdated
Show resolved
Hide resolved
common/utils/src/main/scala/org/apache/spark/internal/Logging.scala
Outdated
Show resolved
Hide resolved
common/utils/src/main/scala/org/apache/spark/internal/Logging.scala
Outdated
Show resolved
Hide resolved
common/utils/src/test/java/org/apache/spark/util/PatternSparkLoggerSuite.java
Outdated
Show resolved
Hide resolved
common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java
Outdated
Show resolved
Hide resolved
common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java
Outdated
Show resolved
Hide resolved
common/utils/src/test/java/org/apache/spark/util/StructuredSparkLoggerSuite.java
Outdated
Show resolved
Hide resolved
common/utils/src/test/java/org/apache/spark/util/PatternSparkLoggerSuite.java
Outdated
Show resolved
Hide resolved
common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mridul Muralidharan <1591700+mridulm@users.noreply.github.com>
Thank you very much for your patient and careful review. ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @panbingkun.
Looks good to me - I will let @gengliangwang review and merge; he is much more knowledgeable about this in general !
Yeah, Thank you very much! ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mridulm @panbingkun This README is for Spark developers. As long as it doesn't mention 3rd party supports, can we just keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine to it, WDYT @mridulm ?
we can just put the new description in the scala/java
doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will wait for @mridulm's response until this weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC @panbingkun included the contents into the existing code; which is also a nice addition to the code documentation.
IMO that is a better overall approach given this is internal to spark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have adopted the plan of deleting README.md
LGTM except for one comment. |
This reverts commit a5fcf8f.
scala/java
doc
Thanks, merging to master |
What changes were proposed in this pull request?
The pr aims to add
external third-party ecosystem access
guide to thescala/java
doc.The external third-party ecosystem is very extensive. Currently, the document covers two scenarios:
Why are the changes needed?
Provide instructions for external third-party ecosystem access to the structured log framework.
Does this PR introduce any user-facing change?
Yes, When an external third-party ecosystem wants to access the structured log framework, developers can get help through this document.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No.