You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dtenedor (via GitHub)" <gi...@apache.org> on 2024/04/23 21:47:38 UTC
[PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
dtenedor opened a new pull request, #46192:
URL: https://github.com/apache/spark/pull/46192
### What changes were proposed in this pull request?
Migrate logInfo with variables of the streaming module to structured logging framework. This transforms the logInfo entries of the following API
```
def logInfo(msg: => String): Unit
```
to
```
def logInfo(entry: LogEntry): Unit
```
### Why are the changes needed?
To enhance Apache Spark's logging system by implementing structured logging.
### Does this PR introduce _any_ user-facing change?
Yes, Spark core logs will contain additional MDC
### How was this patch tested?
Compiler and scala style checks, as well as code review.
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1578573119
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/ProgressReporter.scala:
##########
@@ -104,8 +105,8 @@ class ProgressReporter(
addNewProgress(newProgress)
if (lastNoExecutionProgressEventTime > Long.MinValue) {
postEvent(new QueryIdleEvent(id, runId, formatTimestamp(currentTriggerStartTimestamp)))
- logInfo(s"Streaming query has been idle and waiting for new data more than " +
- s"${noDataProgressEventInterval} ms.")
+ logInfo(log"Streaming query has been idle and waiting for new data more than " +
+ log"${MDC(STREAMING_QUERY_PROGRESS, noDataProgressEventInterval)} ms.")
Review Comment:
This one doesn't seem right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1578512880
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala:
##########
@@ -171,7 +173,9 @@ class MicroBatchExecution(
// Materialize source to avoid creating it in every batch
val metadataPath = s"$resolvedCheckpointRoot/sources/$nextSourceId"
nextSourceId += 1
- logInfo(s"Reading table [$table] from DataSourceV2 named '$srcName' $dsStr")
+ logInfo(log"Reading table [${MDC(STREAMING_TABLE, table)}] " +
Review Comment:
I wonder if we need the STREAMING prefix for these variables. But we can always revisit and combine variables later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1578579834
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/continuous/ContinuousExecution.scala:
##########
@@ -307,7 +311,8 @@ class ContinuousExecution(
} catch {
case t: Throwable if StreamExecution.isInterruptionException(t, sparkSession.sparkContext) &&
state.get() == RECONFIGURING =>
- logInfo(s"Query $id ignoring exception from reconfiguring: $t")
+ logInfo(log"Query ${MDC(QUERY_ID, id)} ignoring exception from reconfiguring: " +
+ log"${MDC(EXCEPTION, t)}")
Review Comment:
Usually we use `ERROR` for this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1578509327
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/IncrementalExecution.scala:
##########
@@ -102,7 +102,7 @@ class IncrementalExecution(
tracker).transformAllExpressionsWithPruning(
_.containsAnyPattern(CURRENT_LIKE, EXPRESSION_WITH_RANDOM_SEED)) {
case ts @ CurrentBatchTimestamp(timestamp, _, _) =>
- logInfo(s"Current batch timestamp = $timestamp")
+ logInfo(log"Current batch timestamp = ${MDC(TIME_UNITS, timestamp)}")
Review Comment:
It seems that we have been using `TIME_UNITS` for durations. This one is timestamp.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #46192: [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework
URL: https://github.com/apache/spark/pull/46192
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #46192:
URL: https://github.com/apache/spark/pull/46192#issuecomment-2078109843
Thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1578584591
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDBFileManager.scala:
##########
@@ -469,9 +475,10 @@ class RocksDBFileManager(
log"version ${MDC(FILE_VERSION, version)}", e)
}
}
- logInfo(s"Deleted ${filesToDelete.size - failedToDelete} files (failed to delete" +
- s"$failedToDelete files) not used in versions >= $minVersionToRetain")
-
+ logInfo(log"Deleted ${MDC(NUM_FILES, filesToDelete.size - failedToDelete)} files " +
+ log"(failed to delete" +
+ log"${MDC(NUM_FILES_FAILED_TO_DELETE, failedToDelete)} files) " +
+ log"not used in versions >= ${MDC(VERSION_NUMBER, minVersionToRetain)}")
Review Comment:
MIN_VERSION_NUMBER?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1579888334
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/continuous/ContinuousExecution.scala:
##########
@@ -307,7 +311,8 @@ class ContinuousExecution(
} catch {
case t: Throwable if StreamExecution.isInterruptionException(t, sparkSession.sparkContext) &&
state.get() == RECONFIGURING =>
- logInfo(s"Query $id ignoring exception from reconfiguring: $t")
+ logInfo(log"Query ${MDC(QUERY_ID, id)} ignoring exception from reconfiguring: " +
+ log"${MDC(EXCEPTION, t)}")
Review Comment:
Nice, I missed that we already had that log key, switched everything to use that instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #46192:
URL: https://github.com/apache/spark/pull/46192#issuecomment-2075678628
Note that the single test failure is flaky and unrelated; the latest CI run otherwise passes all tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on code in PR #46192:
URL: https://github.com/apache/spark/pull/46192#discussion_r1578551226
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/IncrementalExecution.scala:
##########
@@ -102,7 +102,7 @@ class IncrementalExecution(
tracker).transformAllExpressionsWithPruning(
_.containsAnyPattern(CURRENT_LIKE, EXPRESSION_WITH_RANDOM_SEED)) {
case ts @ CurrentBatchTimestamp(timestamp, _, _) =>
- logInfo(s"Current batch timestamp = $timestamp")
+ logInfo(log"Current batch timestamp = ${MDC(TIME_UNITS, timestamp)}")
Review Comment:
Good point. I created a separate BATCH_TIMESTAMP log key for this.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala:
##########
@@ -171,7 +173,9 @@ class MicroBatchExecution(
// Materialize source to avoid creating it in every batch
val metadataPath = s"$resolvedCheckpointRoot/sources/$nextSourceId"
nextSourceId += 1
- logInfo(s"Reading table [$table] from DataSourceV2 named '$srcName' $dsStr")
+ logInfo(log"Reading table [${MDC(STREAMING_TABLE, table)}] " +
Review Comment:
Yes I figured that we could separate streaming vs. regular table name references for now.
As you mention, we could combine log keys at the end of the migration. If we want do to do that, it seems like less work than splitting frequently-used general log keys into multiple specific categories. 🤷
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47597][STREAMING] Streaming: Migrate logInfo with variables to structured logging framework [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #46192:
URL: https://github.com/apache/spark/pull/46192#issuecomment-2073520844
@gengliangwang FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org