Skip to content
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

Jump to bottom

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

Merged
merged 3 commits into from
Mar 2, 2024
Merged

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

merged 3 commits into from
Mar 2, 2024

Conversation

panbingkun
Copy link
Contributor

Follow up for #2338,
Fixes #2337 and makes StatusLogger take debug mode into account.

@panbingkun panbingkun mentioned this pull request Mar 2, 2024
Closed
@panbingkun
Copy link
Contributor Author

cc @vy @ppkarwasz

vy
vy requested changes Mar 2, 2024
View reviewed changes
log4j-api/src/main/java/org/apache/logging/log4j/status/StatusConsoleListener.java Outdated
@@ -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.

@panbingkun
Copy link
Contributor Author

The local verification results are as follows:

...
2024-03-02T12:30:05.718720Z main DEBUG Registering MBean org.apache.logging.log4j2:type=5ffd2b27
2024-03-02T12:30:05.719599Z main DEBUG Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=StatusLogger
2024-03-02T12:30:05.719958Z main DEBUG Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=ContextSelector
2024-03-02T12:30:05.720390Z main DEBUG Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=Loggers,name=
2024-03-02T12:30:05.720736Z main DEBUG Registering MBean org.apache.logging.log4j2:type=5ffd2b27,component=Appenders,name=console
2024-03-02T12:30:05.721543Z main TRACE Using default SystemClock for timestamps.
2024-03-02T12:30:05.721726Z main DEBUG org.apache.logging.log4j.core.util.SystemClock supports precise timestamps.
2024-03-02T12:30:05.721839Z main TRACE Using DummyNanoClock for nanosecond timestamps.
2024-03-02T12:30:05.721929Z main DEBUG Reconfiguration complete for context[name=5ffd2b27] at URI /Users/panbingkun/Developer/spark/spark-client/spark-4.0.0-SNAPSHOT-bin-3.3.6/conf/log4j2.properties (org.apache.logging.log4j.core.LoggerContext@3857f613) with optional ClassLoader: null
2024-03-02T12:30:05.721979Z main DEBUG Shutdown hook enabled. Registering a new one.
2024-03-02T12:30:05.722241Z main DEBUG LoggerContext[name=5ffd2b27, org.apache.logging.log4j.core.LoggerContext@3857f613] started OK.
2024-03-02T12:30:05.769692Z main TRACE Log4jLoggerFactory.getContext() found anchor interface org.apache.spark.internal.Logging
2024-03-02T12:30:05.849111Z main TRACE Log4jLoggerFactory.getContext() found anchor class org.apache.spark.network.util.JavaUtils
2024-03-02T12:30:05.951557Z main TRACE Log4jLoggerFactory.getContext() found anchor class org.apache.hadoop.conf.Configuration
2024-03-02T12:30:05.951733Z main TRACE Log4jLoggerFactory.getContext() found anchor class org.apache.hadoop.conf.Configuration
2024-03-02T12:30:05.957285Z main TRACE Log4jLoggerFactory.getContext() found anchor class org.apache.hadoop.util.Preconditions
2024-03-02T12:30:05.998250Z main TRACE Log4jLoggerFactory.getContext() found anchor interface org.apache.spark.internal.Logging
2024-03-02T12:30:05.999710Z main DEBUG AsyncLogger.ThreadNameStrategy=UNCACHED (user specified null, default is UNCACHED)
2024-03-02T12:30:05.999769Z main TRACE Using default SystemClock for timestamps.
2024-03-02T12:30:05.999811Z main DEBUG org.apache.logging.log4j.core.util.SystemClock supports precise timestamps.
...

It's okay.

@panbingkun panbingkun requested a review from vy March 2, 2024 12:33
ppkarwasz
ppkarwasz approved these changes Mar 2, 2024
View reviewed changes
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

LGTM

@vy vy self-assigned this Mar 2, 2024
@vy vy added bug Incorrect, unexpected, or unintended behavior of existing code api Affects the public API labels Mar 2, 2024
@vy vy added this to the 2.23.1 milestone Mar 2, 2024
@vy
Copy link
Member

vy commented Mar 2, 2024

Great job! 💯
Thanks @panbingkun! 🙇

@vy vy merged commit 28b70d1 into apache:2.x Mar 2, 2024
9 checks passed
@panbingkun
Copy link
Contributor Author

panbingkun commented Mar 3, 2024 •
edited

Thank you very much for the help provided by @vy and @ppkarwasz
Also, may I friendly ask a question, do we have a planned release date for the official version 2.23.1?

@vy
Copy link
Member

vy commented Mar 4, 2024

do we have a planned release date for the official version 2.23.1?

Yes, it was last week, until you told us StatusLogger doesn't work. 😝 Putting joke aside, I think we will try again this week.

@panbingkun
Copy link
Contributor Author

do we have a planned release date for the official version 2.23.1?

Yes, it was last week, until you told us StatusLogger doesn't work. 😝 Putting joke aside, I think we will try again this week.

Haha.., thank you very much! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppkarwasz ppkarwasz ppkarwasz approved these changes

@vy vy Awaiting requested review from vy

Assignees

@vy vy

Labels
api Affects the public API bug Incorrect, unexpected, or unintended behavior of existing code
Projects
None yet
Milestone
2.23.1
3 participants
@panbingkun @vy @ppkarwasz