Skip to content

[FOLLOWUP] Fix StatusLogger log level filtering when debug mode is enabled #2340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 2, 2024
Merged

[FOLLOWUP] Fix StatusLogger log level filtering when debug mode is enabled #2340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0407c14
[FOLLOWUP] Fix StatusLogger log level filtering when debug mode is en…
panbingkun Mar 2, 2024
45bbf85
Revert "[FOLLOWUP] Fix StatusLogger log level filtering when debug mo…
panbingkun Mar 2, 2024
b7a2473
[FOLLOWUP] Fix StatusLogger log level filtering when debug mode is en…
panbingkun Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions log4j-api/src/main/java/org/apache/logging/log4j/status/StatusConsoleListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,28 @@ public class StatusConsoleListener implements StatusListener {
// `volatile` is necessary to correctly read the `stream` without holding the lock
private volatile PrintStream stream;

// whether or not enables debug mode
private final boolean debugEnabled;

/**
* Constructs a {@link StatusConsoleListener} instance writing to {@link System#out} using the supplied level.
*
* @param level the level of status messages that should appear on the console
* @throws NullPointerException on null {@code level}
*/
public StatusConsoleListener(final Level level) {
this(level, System.out);
this(level, false);
}

/**
* Constructs a {@link StatusConsoleListener} instance writing to {@link System#out} using the supplied level.
*
* @param level the level of status messages that should appear on the console
* @param debugEnabled whether or not enables debug mode
* @throws NullPointerException on null {@code level}
*/
public StatusConsoleListener(final Level level, final boolean debugEnabled) {
this(level, System.out, debugEnabled);
}

/**
Expand All @@ -65,8 +79,25 @@ public StatusConsoleListener(final Level level) {
* @throws NullPointerException on null {@code level} or {@code stream}
*/
public StatusConsoleListener(final Level level, final PrintStream stream) {
this(level, stream, false);
}

/**
* Constructs a {@link StatusConsoleListener} instance using the supplied level and stream.
* <p>
* Make sure not to use a logger stream of some sort to avoid creating an infinite loop of indirection!
* </p>
*
* @param level the level of status messages that should appear on the console
* @param stream the stream to write to
* @param debugEnabled whether or not enables debug mode
* @throws NullPointerException on null {@code level} or {@code stream}
*/
public StatusConsoleListener(final Level level, final PrintStream stream,
final boolean debugEnabled) {
this.initialLevel = this.level = requireNonNull(level, "level");
this.initialStream = this.stream = requireNonNull(stream, "stream");
this.debugEnabled = debugEnabled;
}

/**
Expand Down Expand Up @@ -134,7 +165,7 @@ public Level getStatusLevel() {
@Override
public void log(final StatusData data) {
requireNonNull(data, "data");
if (level.isLessSpecificThan(data.getLevel())) {
if (debugEnabled || level.isLessSpecificThan(data.getLevel())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct fix is to remove the filtering on level here. StatusLogger is already taking care of the filtering, listeners should not (incorrectly!) duplicate that work. Hence, instead of leaking debugEnabled to StatusConsoleListener, could you remove this if block, please?

As a result of this change, StatusConsoleListenerTest#level_and_stream_should_be_honored test will be broken. Could you rename it to stream_should_be_honored and fix it too, please? There you won't need multiple messages with different levels anymore. Checking the dump of a single message should be good enough.

I can share some development tips to quickly iterate over your changes:

  • Compile your log4j-api changes: ./mvnw clean install -Dmaven.test.skip -Dspotless.skip -Dcyclonedx.skip -Dspotbugs.skip -Drat.skip -Dbnd.baseline.skip -pl :log4j-api
  • Make sure you fixed StatusConsoleListenerTest: ./mvnw test -Dspotless.skip -Dcyclonedx.skip -Dspotbugs.skip -Drat.skip -Dbnd.baseline.skip -pl :log4j-api-test -Dtest=StatusConsoleListenerTest
  • Fix formatting: ./mvnw spotless:apply -pl :log4j-api,:log4j-api-test
  • Make sure all is in good shape: ./mvnw clean install -pl :log4j-api,:log4j-api-test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me try making it.

final String formattedStatus = data.getFormattedStatus();
stream.println(formattedStatus);
}
Expand Down
3 changes: 2 additions & 1 deletion log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,8 @@ private static final class InstanceHolder {
StatusLogger.class.getSimpleName(),
ParameterizedNoReferenceMessageFactory.INSTANCE,
Config.getInstance(),
new StatusConsoleListener(Config.getInstance().fallbackListenerLevel));
new StatusConsoleListener(Config.getInstance().fallbackListenerLevel,
Config.getInstance().debugEnabled));
}

/**
Expand Down