[SPARK-48629] Migrate the residual code to structured logging framework #46986

panbingkun · 2024-06-14T08:24:47Z

What changes were proposed in this pull request?

The pr aims to migrate the residual code to structured logging framework.

Why are the changes needed?

When I reviewed the spark code, I found that some logs in the some module were not fully migrated to the structured logging framework, let's to complete unfinished work.

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.

panbingkun · 2024-06-14T08:36:00Z

In addition, I have also found similar issues in the core module, but currently there seems to be a PR https://issues.apache.org/jira/browse/SPARK-47579 still in progress, so to prevent conflicts, I skipped it.

gengliangwang · 2024-06-21T21:58:25Z

@panbingkun Could you update this one?

panbingkun · 2024-06-21T23:30:18Z

@panbingkun Could you update this one?

Okay, sure, let me do it.

gengliangwang · 2024-06-23T03:56:32Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

        } else {
          logError(log"Application Master lost connection with driver! Shutting down. " +
-            log"${MDC(LogKeys.REMOTE_ADDRESS, remoteAddress)}")
+            log"${MDC(LogKeys.HOST_PORT, remoteAddress)}")


why renaming this one?

In the context of this code, it is called HOST_PORT, and we need to be consistent.

spark/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

Lines 862 to 873 in e972dae

logInfo(log"Driver terminated or disconnected! Shutting down. " +

log"${MDC(LogKeys.HOST_PORT, remoteAddress)}")

finish(FinalApplicationStatus.SUCCEEDED, ApplicationMaster.EXIT_SUCCESS)

} else {

logError(log"Driver terminated with exit code ${MDC(LogKeys.EXIT_CODE, exitCode)}! " +

log"Shutting down. ${MDC(LogKeys.HOST_PORT, remoteAddress)}")

finish(FinalApplicationStatus.FAILED, exitCode)

}

} else {

logError(log"Application Master lost connection with driver! Shutting down. " +

log"${MDC(LogKeys.REMOTE_ADDRESS, remoteAddress)}")

finish(FinalApplicationStatus.FAILED, ApplicationMaster.EXIT_DISCONNECTED)

gengliangwang · 2024-06-23T03:59:16Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/StreamExecution.scala


-        logError(log"Query ${MDC(LogKeys.PRETTY_ID_STRING, prettyIdString)} " +
-          log"terminated with error", e)
+        logError(log"Query ${MDC(QUERY_ID, prettyIdString)} terminated with error", e)


Why renaming this one?

Let's restore it, I misread it

panbingkun · 2024-06-24T02:00:18Z

Let me double check myself.

panbingkun · 2024-06-24T02:06:54Z

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

  case object HOST extends LogKey
+  case object HOSTS extends LogKey
  case object HOST_LOCAL_BLOCKS_SIZE extends LogKey
-  case object HOST_NAME extends LogKey


Let's unify HOST_NAME to HOST and remove HOST_NAME

panbingkun · 2024-06-24T02:07:17Z

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

+  case object HOSTS extends LogKey
  case object HOST_LOCAL_BLOCKS_SIZE extends LogKey
-  case object HOST_NAME extends LogKey
-  case object HOST_NAMES extends LogKey


Let's unify HOST_NAMES to HOSTS and remove HOST_NAMES

panbingkun · 2024-06-24T02:24:32Z

core/src/main/scala/org/apache/spark/executor/Executor.scala


  logInfo(log"Starting executor ID ${LogMDC(LogKeys.EXECUTOR_ID, executorId)}" +
-    log" on host ${LogMDC(HOST_NAME, executorHostname)}")
+    log" on host ${LogMDC(HOST, executorHostname)}")


Let's unify it as HOST, the general rule is
HOST: Only host name, not including port, eg: host_name
Port: Just the port, eg: 10240
HOST_PORT: includes both host name and port, eg: host_name:10240

spark/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

Line 765 in e972dae

log"on host ${MDC(LogKeys.HOST, executorHostname)} for " +

panbingkun · 2024-06-24T02:27:16Z

core/src/main/scala/org/apache/spark/deploy/master/ui/MasterWebUI.scala

            val removedWorkers = masterEndpointRef.askSync[Integer](
              DecommissionWorkersOnHosts(hostnames))
-            logInfo(log"Decommissioning of hosts ${MDC(HOST_NAMES, hostnames)}" +
+            logInfo(log"Decommissioning of hosts ${MDC(HOSTS, hostnames)}" +


Let's unify it as HOSTS, there is a detailed explanation below.

panbingkun · 2024-06-24T02:28:34Z

...ect/client/jvm/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryListenerBus.scala

-            logWarning(
-              log"Unknown StreamingQueryListener event: " +
-                log"${MDC(LogKeys.EVENT, event)}")
+            logWarning(log"Unknown StreamingQueryListener event: ${MDC(LogKeys.EVENT, event)}")


Make our code format look pretty

panbingkun · 2024-06-24T02:47:41Z

...core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreChangelog.scala

    } catch {
      case e: Throwable =>
        abort()
-        logError(log"Fail to commit changelog file ${MDC(LogKeys.FILE_NAME, file)} " +


Let's call it PATH, and make e as one of the parameters for logError

panbingkun · 2024-06-24T02:59:14Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

-          log"""SparkContext did not initialize after waiting for
-          |${MDC(LogKeys.TIMEOUT, totalWaitTime)} ms.
-          | Please check earlier log output for errors. Failing the application.""".stripMargin)
+        logError(log"SparkContext did not initialize after waiting for " +


Make the code format pretty

panbingkun · 2024-06-24T03:12:36Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/operation/Operation.java

      if (operationLog == null) {
        LOG.error("Operation [ {} ] logging is enabled, " +
          "but its OperationLog object cannot be found.",
-          MDC.of(LogKeys.OPERATION_HANDLE_IDENTIFIER$.MODULE$, opHandle.getHandleIdentifier()));


Let's abbreviate IDENTIFIER as ID

panbingkun · 2024-06-24T03:16:50Z

...erver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetFunctionsOperation.scala

-    val cmdStr = s"catalog : $catalogName, schemaPattern : $schemaName"
-    val logMsg = s"Listing functions '$cmdStr, functionName : $functionName'"
-    logInfo(log"${MDC(LogKeys.MESSAGE, logMsg)} with ${MDC(LogKeys.STATEMENT_ID, statementId)}")
+    val cmdMDC = log"catalog : ${MDC(LogKeys.CATALOG_NAME, catalogName)}, " +


Let's make them more reasonable

panbingkun · 2024-06-24T03:17:31Z

...erver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetFunctionsOperation.scala

    // Do not change cmdStr. It's used for Hive auditing and authorization.
-    val cmdStr = s"catalog : $catalogName, schemaPattern : $schemaName"
-    val logMsg = s"Listing functions '$cmdStr, functionName : $functionName'"
-    logInfo(log"${MDC(LogKeys.MESSAGE, logMsg)} with ${MDC(LogKeys.STATEMENT_ID, statementId)}")


expand logMsg

panbingkun · 2024-06-24T03:28:36Z

@gengliangwang
It's ready.

gengliangwang

Thank you for your meticulous work!

gengliangwang · 2024-06-25T05:46:01Z

Merging to master

panbingkun · 2024-06-25T05:50:44Z

Thank you for your meticulous work!

Thank you for your patient review. ❤️

[SPARK-48629] Migrate the remaining code to structured logging framework

d734306

github-actions bot added SQL STRUCTURED STREAMING KUBERNETES YARN PYTHON DSTREAM CONNECT labels Jun 14, 2024

panbingkun mentioned this pull request Jun 14, 2024

[SPARK-47597][FOLLOWUP] Streaming: Migrate logInfo with variables to structured logging framework #46982

Closed

update

83bf8a0

github-actions bot added CORE and removed PYTHON labels Jun 22, 2024

panbingkun changed the title ~~[SPARK-48629] Migrate the remaining code to structured logging framework~~ [SPARK-48629] Migrate the residual code to structured logging framework Jun 22, 2024

fix unused import

1103b6b

github-actions bot added the WEB UI label Jun 22, 2024

gengliangwang reviewed Jun 23, 2024

View reviewed changes

Merge branch 'master' into sl_other_followup

d1e0713

panbingkun commented Jun 24, 2024

View reviewed changes

update

89538d0

panbingkun marked this pull request as ready for review June 24, 2024 03:28

update

5e61944

panbingkun requested a review from gengliangwang June 24, 2024 03:38

fix unused import

bf7ffb8

gengliangwang approved these changes Jun 25, 2024

View reviewed changes

gengliangwang closed this in d47f34f Jun 25, 2024

	logInfo(log"Driver terminated or disconnected! Shutting down. " +
	log"${MDC(LogKeys.HOST_PORT, remoteAddress)}")
	finish(FinalApplicationStatus.SUCCEEDED, ApplicationMaster.EXIT_SUCCESS)
	} else {
	logError(log"Driver terminated with exit code ${MDC(LogKeys.EXIT_CODE, exitCode)}! " +
	log"Shutting down. ${MDC(LogKeys.HOST_PORT, remoteAddress)}")
	finish(FinalApplicationStatus.FAILED, exitCode)
	}
	} else {
	logError(log"Application Master lost connection with driver! Shutting down. " +
	log"${MDC(LogKeys.REMOTE_ADDRESS, remoteAddress)}")
	finish(FinalApplicationStatus.FAILED, ApplicationMaster.EXIT_DISCONNECTED)

[SPARK-48629] Migrate the residual code to structured logging framework #46986

[SPARK-48629] Migrate the residual code to structured logging framework #46986

Uh oh!

Conversation

panbingkun commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

panbingkun commented Jun 14, 2024

Uh oh!

gengliangwang commented Jun 21, 2024

Uh oh!

panbingkun commented Jun 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun commented Jun 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun commented Jun 24, 2024

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Jun 25, 2024

Uh oh!

panbingkun commented Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

panbingkun commented Jun 14, 2024 •

edited

Loading

panbingkun Jun 24, 2024 •

edited

Loading

panbingkun Jun 24, 2024 •

edited

Loading