Skip to content

Commit

Permalink
[SPARK-48145][CORE] Remove logDebug and logTrace with MDC in JAVA str…
Browse files Browse the repository at this point in the history 
…uctured logging framework

### What changes were proposed in this pull request?

Since we are targeting on migration INFO/WARN/ERROR level logs to structure logging, this PR removes the logDebug and logTrace methods from the JAVA structured logging framework.

### Why are the changes needed?

In the log migration PR apache#46390, there are unnecessary changes such as updating
```
logger.debug("Task {} need to spill {} for {}", taskAttemptId,
            Utils.bytesToString(required - got), requestingConsumer);
```
to
```
LOGGER.debug("Task {} need to spill {} for {}", String.valueOf(taskAttemptId),
            Utils.bytesToString(required - got), requestingConsumer.toString());
```

With this PR, we can avoid such changes during log migrations.
### Does this PR introduce _any_ user-facing change?

No
### How was this patch tested?

Existing UT.
### Was this patch authored or co-authored using generative AI tooling?

No

Closes apache#46405 from gengliangwang/updateJavaLog.

Authored-by: Gengliang Wang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
  • Loading branch information
@gengliangwang @dongjoon-hyun
gengliangwang authored and dongjoon-hyun committed May 6, 2024
1 parent 526e414 commit de8ba85
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 51 deletions.
49 changes: 21 additions & 28 deletions common/utils/src/main/java/org/apache/spark/internal/Logger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,50 +110,43 @@ public void debug(String msg) {
slf4jLogger.debug(msg);
}

public void debug(String msg, Throwable throwable) {
slf4jLogger.debug(msg, throwable);
public void debug(String format, Object arg) {
slf4jLogger.debug(format, arg);
}

public void debug(String msg, MDC... mdcs) {
if (mdcs == null || mdcs.length == 0) {
slf4jLogger.debug(msg);
} else if (slf4jLogger.isDebugEnabled()) {
withLogContext(msg, mdcs, null, mt -> slf4jLogger.debug(mt.message));
}
public void debug(String format, Object arg1, Object arg2) {
slf4jLogger.debug(format, arg1, arg2);
}

public void debug(String msg, Throwable throwable, MDC... mdcs) {
if (mdcs == null || mdcs.length == 0) {
slf4jLogger.debug(msg);
} else if (slf4jLogger.isDebugEnabled()) {
withLogContext(msg, mdcs, throwable, mt -> slf4jLogger.debug(mt.message, mt.throwable));
}
public void debug(String format, Object... arguments) {
slf4jLogger.debug(format, arguments);
}

public void debug(String msg, Throwable throwable) {
slf4jLogger.debug(msg, throwable);
}

public void trace(String msg) {
slf4jLogger.trace(msg);
}

public void trace(String msg, Throwable throwable) {
slf4jLogger.trace(msg, throwable);
public void trace(String format, Object arg) {
slf4jLogger.trace(format, arg);
}

public void trace(String msg, MDC... mdcs) {
if (mdcs == null || mdcs.length == 0) {
slf4jLogger.trace(msg);
} else if (slf4jLogger.isTraceEnabled()) {
withLogContext(msg, mdcs, null, mt -> slf4jLogger.trace(mt.message));
}
public void trace(String format, Object arg1, Object arg2) {
slf4jLogger.trace(format, arg1, arg2);
}

public void trace(String msg, Throwable throwable, MDC... mdcs) {
if (mdcs == null || mdcs.length == 0) {
slf4jLogger.trace(msg);
} else if (slf4jLogger.isTraceEnabled()) {
withLogContext(msg, mdcs, throwable, mt -> slf4jLogger.trace(mt.message, mt.throwable));
}
public void trace(String format, Object... arguments) {
slf4jLogger.trace(format, arguments);
}

public void trace(String msg, Throwable throwable) {
slf4jLogger.trace(msg, throwable);
}


private void withLogContext(
String pattern,
MDC[] mdcs,
Expand Down
28 changes: 5 additions & 23 deletions common/utils/src/test/java/org/apache/spark/util/LoggerSuiteBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,7 @@ public void testLoggerWithMDC() {
List.of(
Pair.of(Level.ERROR, errorFn),
Pair.of(Level.WARN, warnFn),
Pair.of(Level.INFO, infoFn),
Pair.of(Level.DEBUG, debugFn),
Pair.of(Level.TRACE, traceFn)).forEach(pair -> {
Pair.of(Level.INFO, infoFn)).forEach(pair -> {
try {
assert (captureLogOutput(pair.getRight()).matches(
expectedPatternForMsgWithMDC(pair.getLeft())));
Expand All @@ -162,14 +160,10 @@ public void testLoggerWithMDCs() {
Runnable errorFn = () -> logger().error(msgWithMDCs, mdcs);
Runnable warnFn = () -> logger().warn(msgWithMDCs, mdcs);
Runnable infoFn = () -> logger().info(msgWithMDCs, mdcs);
Runnable debugFn = () -> logger().debug(msgWithMDCs, mdcs);
Runnable traceFn = () -> logger().trace(msgWithMDCs, mdcs);
List.of(
Pair.of(Level.ERROR, errorFn),
Pair.of(Level.WARN, warnFn),
Pair.of(Level.INFO, infoFn),
Pair.of(Level.DEBUG, debugFn),
Pair.of(Level.TRACE, traceFn)).forEach(pair -> {
Pair.of(Level.INFO, infoFn)).forEach(pair -> {
try {
assert (captureLogOutput(pair.getRight()).matches(
expectedPatternForMsgWithMDCs(pair.getLeft())));
Expand All @@ -185,14 +179,10 @@ public void testLoggerWithMDCsAndException() {
Runnable errorFn = () -> logger().error(msgWithMDCs, exception, mdcs);
Runnable warnFn = () -> logger().warn(msgWithMDCs, exception, mdcs);
Runnable infoFn = () -> logger().info(msgWithMDCs, exception, mdcs);
Runnable debugFn = () -> logger().debug(msgWithMDCs, exception, mdcs);
Runnable traceFn = () -> logger().trace(msgWithMDCs, exception, mdcs);
List.of(
Pair.of(Level.ERROR, errorFn),
Pair.of(Level.WARN, warnFn),
Pair.of(Level.INFO, infoFn),
Pair.of(Level.DEBUG, debugFn),
Pair.of(Level.TRACE, traceFn)).forEach(pair -> {
Pair.of(Level.INFO, infoFn)).forEach(pair -> {
try {
assert (captureLogOutput(pair.getRight()).matches(
expectedPatternForMsgWithMDCsAndException(pair.getLeft())));
Expand All @@ -207,14 +197,10 @@ public void testLoggerWithMDCValueIsNull() {
Runnable errorFn = () -> logger().error(msgWithMDC, executorIDMDCValueIsNull);
Runnable warnFn = () -> logger().warn(msgWithMDC, executorIDMDCValueIsNull);
Runnable infoFn = () -> logger().info(msgWithMDC, executorIDMDCValueIsNull);
Runnable debugFn = () -> logger().debug(msgWithMDC, executorIDMDCValueIsNull);
Runnable traceFn = () -> logger().trace(msgWithMDC, executorIDMDCValueIsNull);
List.of(
Pair.of(Level.ERROR, errorFn),
Pair.of(Level.WARN, warnFn),
Pair.of(Level.INFO, infoFn),
Pair.of(Level.DEBUG, debugFn),
Pair.of(Level.TRACE, traceFn)).forEach(pair -> {
Pair.of(Level.INFO, infoFn)).forEach(pair -> {
try {
assert (captureLogOutput(pair.getRight()).matches(
expectedPatternForMsgWithMDCValueIsNull(pair.getLeft())));
Expand All @@ -229,14 +215,10 @@ public void testLoggerWithExternalSystemCustomLogKey() {
Runnable errorFn = () -> logger().error("{}", externalSystemCustomLog);
Runnable warnFn = () -> logger().warn("{}", externalSystemCustomLog);
Runnable infoFn = () -> logger().info("{}", externalSystemCustomLog);
Runnable debugFn = () -> logger().debug("{}", externalSystemCustomLog);
Runnable traceFn = () -> logger().trace("{}", externalSystemCustomLog);
List.of(
Pair.of(Level.ERROR, errorFn),
Pair.of(Level.WARN, warnFn),
Pair.of(Level.INFO, infoFn),
Pair.of(Level.DEBUG, debugFn),
Pair.of(Level.TRACE, traceFn)).forEach(pair -> {
Pair.of(Level.INFO, infoFn)).forEach(pair -> {
try {
assert (captureLogOutput(pair.getRight()).matches(
expectedPatternForExternalSystemCustomLogKey(pair.getLeft())));
Expand Down

0 comments on commit de8ba85

Please sign in to comment.