[SPARK-47579][CORE][PART1] Migrate logInfo with variables to structured logging framework #46362

zeotuan · 2024-05-03T09:12:19Z

The PR aims to migrate logInfo in Core module with variables to structured logging framework.

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?

Pass GA.

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

No.

zeotuan · 2024-05-06T02:17:32Z

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

@@ -26,13 +26,16 @@ trait LogKey {
 }

 /**
- * Various keys used for mapped diagnostic contexts(MDC) in logging.
- * All structured logging keys should be defined here for standardization.
+ * Various keys used for mapped diagnostic contexts(MDC) in logging. All structured logging keys


Many of these formatting changes happened as the result of running dev/scalafmt. Should we keep them?

zeotuan · 2024-05-06T02:26:10Z

@gengliangwang Hi please help review

gengliangwang · 2024-05-06T17:36:58Z

core/src/main/scala/org/apache/spark/BarrierCoordinator.scala

- * all the requests for a group of `barrier()` calls are received. If the coordinator is unable to
- * collect enough global sync requests within a configured time, fail all the requests and return
- * an Exception with timeout message.
+ * request is generated by `BarrierTaskContext.barrier()`, and identified by stageId +


Please remove unnecessary changes

gengliangwang · 2024-05-06T17:40:17Z

@zeotuan Thanks for the work. However, there are a lot of unnecessary changes in this PR, which are probably from scalafmt. This makes the PR review hard.
Usually, Spark PRs require cleaner code changes. Could you revert to c9e0209? You will have to fix the errors from ./dev/lint-scala manually.

zeotuan · 2024-05-06T22:58:47Z

@zeotuan Thanks for the work. However, there are a lot of unnecessary changes in this PR, which are probably from scalafmt. This makes the PR review hard. Usually, Spark PRs require cleaner code changes. Could you revert to c9e0209? You will have to fix the errors from ./dev/lint-scala manually.

Sure I will revert it. I was just wondering if there is any plan to fix/apply all these scalafmt changes or why would it produce such different result.

…fo-backup

gengliangwang · 2024-05-06T23:16:42Z

I was just wondering if there is any plan to fix/apply all these scalafmt changes or why would it produce such different result.

There has been such discussions. The major concern of not having such a format tool is to avoid making the git blame harder.

core/src/main/scala/org/apache/spark/BarrierTaskContext.scala

gengliangwang · 2024-05-06T23:54:40Z

core/src/main/scala/org/apache/spark/MapOutputTracker.scala

@@ -489,21 +488,24 @@ private[spark] class MapOutputTrackerMasterEndpoint(
  extends RpcEndpoint with Logging {

  logDebug("init") // force eager creation of logger
+  private def logInfoMsg(msg: MessageWithContext, shuffleId: Int, context: RpcCallContext): Unit = {


nit: add a blank line

gengliangwang · 2024-05-07T06:09:19Z

core/src/main/scala/org/apache/spark/SparkContext.scala

@@ -1855,7 +1859,9 @@ class SparkContext(config: SparkConf) extends Logging {
        root,
        if (uri.getFragment != null) uri.getFragment else source.getName)
      logInfo(
-        s"Unpacking an archive $path from ${source.getAbsolutePath} to ${dest.getAbsolutePath}")
+        log"Unpacking an archive ${MDC(LogKeys.PATH, path)}" +
+          log" from ${MDC(LogKeys.SOURCE_ABSOLUTE_PATH, source.getAbsolutePath)}" +


nit: how about simply SOURCE_PATH

gengliangwang · 2024-05-07T06:09:29Z

core/src/main/scala/org/apache/spark/SparkContext.scala

-        s"Unpacking an archive $path from ${source.getAbsolutePath} to ${dest.getAbsolutePath}")
+        log"Unpacking an archive ${MDC(LogKeys.PATH, path)}" +
+          log" from ${MDC(LogKeys.SOURCE_ABSOLUTE_PATH, source.getAbsolutePath)}" +
+          log" to ${MDC(LogKeys.DESTINATION_ABSOLUTE_PATH, dest.getAbsolutePath)}")


nit: how about simply DESTINATION_PATH

gengliangwang · 2024-05-07T06:21:39Z

repl/src/test/scala/org/apache/spark/repl/SparkShellSuite.scala

@@ -26,6 +26,8 @@ import scala.concurrent.duration._

 import org.apache.spark.ProcessTestUtils.ProcessOutputCapturer
 import org.apache.spark.SparkFunSuite
+import org.apache.spark.internal.{MDC, MessageWithContext}


We don't need to migrate the log entries from tests. Let's revert the changes in this file to avoid introducing new log keys.

gengliangwang · 2024-05-07T06:22:03Z

core/src/main/scala/org/apache/spark/deploy/FaultToleranceTest.scala

@@ -187,7 +188,7 @@ private object FaultToleranceTest extends App with Logging {
      fn
      numPassed += 1
      logInfo("==============================================")
-      logInfo("Passed: " + name)
+      logInfo(log"Passed: ${MDC(TEST_NAME, name)}")


We don't need to migrate the log entries from tests. Let's revert the changes in this file to avoid introducing new log keys.

gengliangwang · 2024-05-07T06:22:21Z

LGTM otherwise. Thanks for the good work!

gengliangwang · 2024-05-07T21:37:28Z

@zeotuan FYI I pushed a commit b45b340 to reduce a log key. Please help review it and let me know what you think of it.

zeotuan · 2024-05-07T23:16:59Z

@gengliangwang Thank you, LGTM!

gengliangwang · 2024-05-08T00:34:52Z

Thanks, merging to master

…ed logging framework The PR aims to migrate `logInfo` in Core module with variables to structured logging framework. ### 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? - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46362 from zeotuan/coreInfo. Lead-authored-by: Tuan Pham <Tuan.Pham@wisetechglobal.com> Co-authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

Migrate Spark Core LogInfo Part 1

c9e0209

github-actions bot added WEB UI SPARK SHELL CORE PYTHON labels May 3, 2024

fix scalafmt

82a1012

github-actions bot added SQL STRUCTURED STREAMING KUBERNETES BUILD DOCS INFRA R AVRO PANDAS API ON SPARK CONNECT and removed STRUCTURED STREAMING KUBERNETES BUILD DOCS INFRA R AVRO PANDAS API ON SPARK CONNECT labels May 6, 2024

zeotuan commented May 6, 2024

View reviewed changes

zeotuan marked this pull request as ready for review May 6, 2024 02:17

gengliangwang reviewed May 6, 2024

View reviewed changes

Merge branch 'master' of https://github.com/zeotuan/spark into coreIn…

563bc67

…fo-backup

zeotuan force-pushed the coreInfo branch from 499a76a to 563bc67 Compare May 6, 2024 23:10

github-actions bot removed the SQL label May 6, 2024

gengliangwang reviewed May 6, 2024

View reviewed changes

core/src/main/scala/org/apache/spark/BarrierTaskContext.scala Outdated Show resolved Hide resolved

zeotuan added 2 commits May 7, 2024 09:20

Fix ambiguous import

fdffedb

Update message buffer type

16b636f

gengliangwang reviewed May 6, 2024

View reviewed changes

Formatting fix

d16bc8a

gengliangwang reviewed May 7, 2024

View reviewed changes

Fix Changes Request

faec03b

github-actions bot removed the SPARK SHELL label May 7, 2024

zeotuan and others added 3 commits May 7, 2024 16:50

Remove structured logging from tests

58e40f1

Merge branch 'master' into coreInfo

a0c68e4

remove key ADDED_JARS_MESSAGE

b45b340

gengliangwang approved these changes May 7, 2024

View reviewed changes

gengliangwang approved these changes May 8, 2024

View reviewed changes

gengliangwang closed this in a15adeb May 8, 2024

gengliangwang changed the title ~~[SPARK-47240][CORE][PART1] Migrate logInfo with variables to structured logging framework~~ [SPARK-47579][CORE][PART1] Migrate logInfo with variables to structured logging framework May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-47579][CORE][PART1] Migrate logInfo with variables to structured logging framework #46362

[SPARK-47579][CORE][PART1] Migrate logInfo with variables to structured logging framework #46362

zeotuan commented May 3, 2024

zeotuan May 6, 2024 •

edited

zeotuan commented May 6, 2024

gengliangwang May 6, 2024

gengliangwang commented May 6, 2024

zeotuan commented May 6, 2024

gengliangwang commented May 6, 2024

gengliangwang May 6, 2024

gengliangwang May 7, 2024

gengliangwang May 7, 2024

gengliangwang May 7, 2024

gengliangwang May 7, 2024

gengliangwang commented May 7, 2024

gengliangwang commented May 7, 2024

zeotuan commented May 7, 2024

gengliangwang commented May 8, 2024

[SPARK-47579][CORE][PART1] Migrate logInfo with variables to structured logging framework #46362

[SPARK-47579][CORE][PART1] Migrate logInfo with variables to structured logging framework #46362

Conversation

zeotuan commented May 3, 2024

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?

zeotuan May 6, 2024 • edited

Choose a reason for hiding this comment

zeotuan commented May 6, 2024

gengliangwang May 6, 2024

Choose a reason for hiding this comment

gengliangwang commented May 6, 2024

zeotuan commented May 6, 2024

gengliangwang commented May 6, 2024

gengliangwang May 6, 2024

Choose a reason for hiding this comment

gengliangwang May 7, 2024

Choose a reason for hiding this comment

gengliangwang May 7, 2024

Choose a reason for hiding this comment

gengliangwang May 7, 2024

Choose a reason for hiding this comment

gengliangwang May 7, 2024

Choose a reason for hiding this comment

gengliangwang commented May 7, 2024

gengliangwang commented May 7, 2024

zeotuan commented May 7, 2024

gengliangwang commented May 8, 2024

zeotuan May 6, 2024 •

edited