[SPARK-48059][CORE] Implement the structured log framework on the java side #46301

panbingkun · 2024-04-30T11:45:19Z

What changes were proposed in this pull request?

The pr aims to implement the structured log framework on the java side.

Why are the changes needed?

Currently, the structured log framework on the scala side is basically available, but theSpark Core code also includes some Java code, which also needs to be connected to the structured log framework.

Does this PR introduce any user-facing change?

Yes, only for developers.

How was this patch tested?

Add some new UT.
Pass GA.

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

No.

…a side

panbingkun · 2024-04-30T11:50:32Z

@gengliangwang
In the migration of logs to the structured log framework, I found that we missed the corresponding implementation on the java side. This PR is to supplement it.

panbingkun · 2024-04-30T13:56:59Z

We currently support three ways to write logs:

loggger.{error/warn/info/debug/trace}(string)
loggger.{error/warn/info/debug/trace}(string, throwable)
loggger.{error/warn/info/debug/trace}({string template with {} inside}, JMDC...), eg:
logger.info("Executor {} failed", JMDC(EXECUTOR_ID, "xxxx")

After this pr, we can prohibit importing the following two classes in the java code:

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

gengliangwang · 2024-04-30T18:17:27Z

@panbingkun Thanks for bringing this up.
I check the current Spark java code:

find . -name "*.java"|xargs grep "logInfo\|logWarn\|logError"|grep -v target|grep -v test

and found no log4j usages. Is this for external systems? If yes, the external system should be able to use the current Logging trait as well, right?

panbingkun · 2024-04-30T18:47:42Z

@panbingkun Thanks for bringing this up. I check the current Spark java code:
find . -name "*.java"|xargs grep "logInfo\|logWarn\|logError"|grep -v target|grep -v test
and found no log4j usages. Is this for external systems? If yes, the external system should be able to use the current Logging trait as well, right?

@gengliangwang
Hmm..., let me give you some examples, eg:

spark/core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java

Lines 285 to 290 in 9caa6f7

 logger.info( 

 "{} bytes of memory were used by task {} but are not associated with specific consumers", 

 memoryNotAccountedFor, taskAttemptId); 

 logger.info( 

 "{} bytes of memory are used for execution and {} bytes of memory are used for storage", 

 memoryManager.executionMemoryUsed(), memoryManager.storageMemoryUsed());

spark/core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

Line 395 in 9caa6f7

logger.error("Was unable to delete spill file {}", file.getAbsolutePath());

spark/connector/spark-ganglia-lgpl/src/main/java/com/codahale/metrics/ganglia/GangliaReporter.java

Line 336 in 9caa6f7

LOGGER.warn("Unable to report histogram {}", sanitizedName, e);

gengliangwang · 2024-04-30T20:29:16Z

@panbingkun I see. There are about 88 loggings with variables

find . -name "*.java"|xargs grep -i "logger.info\|logger.warn\|logger.error"|grep "{}"| grep -v target|grep -v test

Since all the logs are using {} as variable placeholders, shall we try following the approach Sequence of Key-Value Pairs (Seq[(K, V)]) from the SPIP?
Also, I think we can reference the LogKey.scala in the java side.

panbingkun · 2024-05-02T02:31:23Z

@panbingkun I see. There are about 88 loggings with variables
find . -name "*.java"|xargs grep -i "logger.info\|logger.warn\|logger.error"|grep "{}"| grep -v target|grep -v test
Since all the logs are using {} as variable placeholders, shall we try following the approach Sequence of Key-Value Pairs (Seq[(K, V)]) from the SPIP? Also, I think we can reference the LogKey.scala in the java side.

I think this is a good suggestion. Let me try it.

panbingkun · 2024-05-02T06:06:12Z

common/utils/src/main/java/org/apache/spark/internal/Logger.java

+ }
+ }
+
+ public void error(String msg, MDC... mdcs) {


Although the following design is shown in the document,

But I think the parameter void error(String msg, MDC... mdcs) is more in line with the style of Java? WDYT @gengliangwang

Either way is fine to me

panbingkun · 2024-05-02T14:32:46Z

@gengliangwang
This pr is ready for review.

gengliangwang · 2024-05-03T01:22:15Z

common/utils/src/main/java/org/apache/spark/internal/Logger.java

+ if (mdcs == null || mdcs.length == 0) {
+ slf4jLogger.warn(msg);
+ } else {
+ withLogContext(msg, mdcs, null, mt -> slf4jLogger.warn(mt.message));


Let's check the log level before calling withLogContext. For example, if the log level is ERROR, we don't need to enter the method withLogContext

gengliangwang · 2024-05-03T23:30:05Z

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

+import org.apache.spark.internal.LogKeys;
+import org.apache.spark.internal.MDC;
+
+public abstract class LoggerSuiteBase {


nit: ideally, we can have a followup to reduce the duplicated test code.

gengliangwang · 2024-05-03T23:30:18Z

Thanks, merging to master

panbingkun · 2024-05-04T00:26:05Z

Thanks, merging to master

Thank you very much.
Based on this, I will do log migration and auxiliary work on the java side.
(eg, after the migration is completed, we can add a rule in checkstyle that prohibits importing org.slf4j.Logger and org.slf4j.LoggerFactory)

gengliangwang · 2024-05-04T00:27:06Z

@panbingkun Awesome, thanks for the work!

…a side ### What changes were proposed in this pull request? The pr aims to implement the structured log framework on the `java side`. ### Why are the changes needed? Currently, the structured log framework on the `scala side` is basically available, but the`Spark Core` code also includes some `Java code`, which also needs to be connected to the structured log framework. ### Does this PR introduce _any_ user-facing change? Yes, only for developers. ### How was this patch tested? - Add some new UT. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46301 from panbingkun/structured_logger_java. Authored-by: panbingkun <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>

### What changes were proposed in this pull request? Two new classes `org.apache.spark.internal.Logger` and `org.apache.spark.internal.LoggerFactory` were introduced from #46301. Given that Logger is a widely recognized **interface** in Log4j, it may lead to confusion to have a class with the same name. To avoid this and clarify its purpose within the Spark framework, I propose renaming `org.apache.spark.internal.Logger` to `org.apache.spark.internal.SparkLogger`. Similarly, to maintain consistency, `org.apache.spark.internal.LoggerFactory` should be renamed to `org.apache.spark.internal.SparkLoggerFactory`. ### Why are the changes needed? To avoid naming confusion and clarify the java Spark logger purpose within the logging framework ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? GA tests ### Was this patch authored or co-authored using generative AI tooling? No Closes #46600 from gengliangwang/refactorLogger. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>

[SPARK-48059][CORE] Implement the structured log framework on the jav…

f256ca8

…a side

fix style

7216263

Merge branch 'master' into structured_logger_java

1efbd70

fix

26dea93

panbingkun commented May 2, 2024

View reviewed changes

panbingkun added 2 commits May 2, 2024 21:58

update

d2f6aaf

Merge branch 'master' into structured_logger_java

cbb1f1f

panbingkun marked this pull request as ready for review May 2, 2024 14:31

gengliangwang reviewed May 3, 2024

View reviewed changes

panbingkun added 2 commits May 3, 2024 20:44

Merge branch 'master' into structured_logger_java

b7747ee

update

ee57438

gengliangwang approved these changes May 3, 2024

View reviewed changes

gengliangwang reviewed May 3, 2024

View reviewed changes

gengliangwang closed this in 5c01f19 May 3, 2024

gengliangwang mentioned this pull request May 15, 2024

[SPARK-48291][Core] Rename Java Logger as SparkLogger #46600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48059][CORE] Implement the structured log framework on the java side #46301

[SPARK-48059][CORE] Implement the structured log framework on the java side #46301

panbingkun commented Apr 30, 2024 •

edited

panbingkun commented Apr 30, 2024

panbingkun commented Apr 30, 2024 •

edited

gengliangwang commented Apr 30, 2024

panbingkun commented Apr 30, 2024

gengliangwang commented Apr 30, 2024

panbingkun commented May 2, 2024

panbingkun May 2, 2024

gengliangwang May 2, 2024

panbingkun commented May 2, 2024

gengliangwang May 3, 2024

panbingkun May 3, 2024

gengliangwang May 3, 2024

gengliangwang commented May 3, 2024

panbingkun commented May 4, 2024

gengliangwang commented May 4, 2024

[SPARK-48059][CORE] Implement the structured log framework on the java side #46301

[SPARK-48059][CORE] Implement the structured log framework on the java side #46301

Conversation

panbingkun commented Apr 30, 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?

panbingkun commented Apr 30, 2024

panbingkun commented Apr 30, 2024 • edited

gengliangwang commented Apr 30, 2024

panbingkun commented Apr 30, 2024

gengliangwang commented Apr 30, 2024

panbingkun commented May 2, 2024

panbingkun May 2, 2024

Choose a reason for hiding this comment

gengliangwang May 2, 2024

Choose a reason for hiding this comment

panbingkun commented May 2, 2024

gengliangwang May 3, 2024

Choose a reason for hiding this comment

panbingkun May 3, 2024

Choose a reason for hiding this comment

gengliangwang May 3, 2024

Choose a reason for hiding this comment

gengliangwang commented May 3, 2024

panbingkun commented May 4, 2024

gengliangwang commented May 4, 2024

panbingkun commented Apr 30, 2024 •

edited

panbingkun commented Apr 30, 2024 •

edited