[SPARK-48320][CORE][DOCS] Add structured logging guide to the scala and java doc #46634

panbingkun · 2024-05-17T07:28:50Z

What changes were proposed in this pull request?

The pr aims to add external third-party ecosystem access guide to the scala/java doc.

The external third-party ecosystem is very extensive. Currently, the document covers two scenarios:

Pure java (for example, an application only uses the java language - many of our internal production applications are like this)
java + scala

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?

Add new UT.
Manually test.
Pass GA.

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

No.

…uide to the doc

panbingkun · 2024-05-17T07:49:25Z

common/utils/src/main/scala/org/apache/spark/internal/LogKey.scala

@@ -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)


If we don't make such changes, the JavaCustomLogKeys(Custom LogKey that may be extended by external third-party ecosystems) class will have to be defined like this, which is very ugly and looks weird.

panbingkun · 2024-05-17T07:50:13Z

common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java

@@ -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(


replace assert with Assertions.assertTrue

panbingkun · 2024-05-17T07:53:09Z

common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java

@@ -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);


After #46405, the following 2 lines of code are no longer needed.

panbingkun · 2024-05-17T07:57:50Z

cc @gengliangwang

gengliangwang · 2024-05-20T05:59:13Z

common/utils/src/main/scala/org/apache/spark/internal/LogKey.scala

@@ -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)


shall we have a val or a lazy val to memorize the key?

Okay, let's use lazy val, I have tested it and it works well.

gengliangwang · 2024-05-20T05:59:35Z

common/utils/src/main/scala/org/apache/spark/internal/README.md

+  public static final CUSTOM_LOG_KEY CUSTOM_LOG_KEY = new CUSTOM_LOG_KEY();
+}
+```
+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;and use it in `java` code:


why do we have &nbsp here?

If &nbsp is not added, its indent looks incorrect, as shown below:

Let me incorporate this sentence into the above one, and revise it as follows:

gengliangwang · 2024-05-20T06:00:04Z

common/utils/src/main/scala/org/apache/spark/internal/README.md

+* If you want to output logs in `scala code` through the structured log framework, you can define `custom LogKeys` as follows:
+
+```scala
+object CustomLogKeys {


let's not mention CustomLogKeys here. It is not necessary, right?

gengliangwang · 2024-05-20T06:00:15Z

common/utils/src/main/scala/org/apache/spark/internal/README.md

+
+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;when your project has **depended on `scala`**,
+```scala
+object CustomLogKeys {


the content seems duplicated?

1.Use the custom LogKey defined in the scala code, and use it in the scala code
2.Use the custom LogKey defined in the java code, and use it in the java code
3.Use the custom LogKey defined in the scala code, and use it in the java code

Do we need to delete the description of the last case - 3?

panbingkun · 2024-05-20T06:51:14Z

Final effect:

common/utils/src/main/scala/org/apache/spark/internal/README.md

mridulm

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

common/utils/src/main/scala/org/apache/spark/internal/Logging.scala

common/utils/src/test/java/org/apache/spark/util/PatternSparkLoggerSuite.java

common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java

common/utils/src/test/java/org/apache/spark/util/StructuredSparkLoggerSuite.java

common/utils/src/test/java/org/apache/spark/util/PatternSparkLoggerSuite.java

common/utils/src/test/java/org/apache/spark/util/SparkLoggerSuiteBase.java

Co-authored-by: Mridul Muralidharan <1591700+mridulm@users.noreply.github.com>

panbingkun · 2024-05-23T03:04:31Z

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 :-)

Thank you very much for your patient and careful review. ❤️
I have updated it again.

mridulm

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 !

panbingkun · 2024-05-23T07:02:07Z

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! ❤️

gengliangwang · 2024-05-23T17:30:37Z

common/utils/src/main/scala/org/apache/spark/internal/README.md

@mridulm @panbingkun This README is for Spark developers. As long as it doesn't mention 3rd party supports, can we just keep it?

I'm fine to it, WDYT @mridulm ?
we can just put the new description in the scala/java doc.

I will wait for @mridulm's response until this weekend.

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.

Okay, I have adopted the plan of deleting README.md

gengliangwang · 2024-05-23T17:31:05Z

LGTM except for one comment.

This reverts commit a5fcf8f.

gengliangwang · 2024-05-25T16:42:46Z

Thanks, merging to master

[SPARK-48320][CORE][DOCS] Add external third-party ecosystem access g…

a9ff819

…uide to the doc

github-actions bot added the DOCS label May 17, 2024

panbingkun commented May 17, 2024

View reviewed changes

panbingkun marked this pull request as ready for review May 17, 2024 07:57

gengliangwang reviewed May 20, 2024

View reviewed changes

panbingkun added 4 commits May 20, 2024 14:51

update

541ccbc

Merge branch 'master' into SPARK-48320

f67d100

fix

1c766c5

Merge branch 'master' into SPARK-48320

173acdd

mridulm reviewed May 20, 2024

View reviewed changes

common/utils/src/main/scala/org/apache/spark/internal/README.md Outdated Show resolved Hide resolved

panbingkun added 2 commits May 22, 2024 10:53

Merge branch 'master' into SPARK-48320

ccba177

remove README.md

90309db

panbingkun changed the title ~~[SPARK-48320][CORE][DOCS] Add external third-party ecosystem access guide to the doc~~ [SPARK-48320][CORE][DOCS] Add external third-party ecosystem access guide to the scala/java doc May 22, 2024

update

7a96541

mridulm requested changes May 22, 2024

View reviewed changes

panbingkun and others added 4 commits May 23, 2024 10:18

Apply suggestions from code review

ebfe862

Co-authored-by: Mridul Muralidharan <1591700+mridulm@users.noreply.github.com>

update

a8cf34a

Merge branch 'master' into SPARK-48320

f2ac58f

update

d51a253

panbingkun requested a review from gengliangwang May 23, 2024 03:36

mridulm approved these changes May 23, 2024

View reviewed changes

gengliangwang reviewed May 23, 2024

View reviewed changes

panbingkun added 2 commits May 24, 2024 14:08

Merge branch 'master' into SPARK-48320

4572dcd

restore README.md

a5fcf8f

github-actions bot removed the DOCS label May 24, 2024

panbingkun added 2 commits May 24, 2024 17:08

Merge branch 'master' into SPARK-48320

d7969d5

Revert "restore README.md"

a7180df

This reverts commit a5fcf8f.

github-actions bot added the DOCS label May 25, 2024

gengliangwang approved these changes May 25, 2024

View reviewed changes

gengliangwang changed the title ~~[SPARK-48320][CORE][DOCS] Add external third-party ecosystem access guide to the scala/java doc~~ [SPARK-48320][CORE][DOCS] Add structured logging guide to the scala and java doc May 25, 2024

gengliangwang closed this in 468aa84 May 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48320][CORE][DOCS] Add structured logging guide to the scala and java doc #46634

[SPARK-48320][CORE][DOCS] Add structured logging guide to the scala and java doc #46634

panbingkun commented May 17, 2024 •

edited

panbingkun May 17, 2024

panbingkun May 17, 2024

panbingkun May 17, 2024

panbingkun commented May 17, 2024

gengliangwang May 20, 2024

panbingkun May 20, 2024

panbingkun May 20, 2024

gengliangwang May 20, 2024

panbingkun May 20, 2024

gengliangwang May 20, 2024

panbingkun May 20, 2024

gengliangwang May 20, 2024

panbingkun May 20, 2024

panbingkun May 20, 2024

panbingkun commented May 20, 2024 •

edited

mridulm left a comment •

edited

panbingkun commented May 23, 2024

mridulm left a comment

panbingkun commented May 23, 2024

gengliangwang May 23, 2024

panbingkun May 24, 2024

gengliangwang May 24, 2024

mridulm May 25, 2024

panbingkun May 25, 2024

gengliangwang commented May 23, 2024

gengliangwang commented May 25, 2024

[SPARK-48320][CORE][DOCS] Add structured logging guide to the scala and java doc #46634

[SPARK-48320][CORE][DOCS] Add structured logging guide to the scala and java doc #46634

Conversation

panbingkun commented May 17, 2024 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panbingkun commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panbingkun commented May 20, 2024 • edited

mridulm left a comment • edited

Choose a reason for hiding this comment

panbingkun commented May 23, 2024

mridulm left a comment

Choose a reason for hiding this comment

panbingkun commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengliangwang commented May 23, 2024

gengliangwang commented May 25, 2024

panbingkun commented May 17, 2024 •

edited

panbingkun commented May 20, 2024 •

edited

mridulm left a comment •

edited