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.

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

Issue 1501: increase default StatusLogger level to warn #1592

Draft
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

lukaszspyra
Copy link
Contributor

Fix of #1501.

  1. StatusLogger default level bumped from ERROR to WARN (used when no config active);
  2. StatusConfiguration default level bumped from ERROR to WARN(used as default value). It's method withStatus(String status) modified to default to WARN (used when status string value found in config file is invalid);
  3. AbstractConfiguration, method getDefaultStatus() modified to default to WARN, as it is called in a.o. JsonConfiguration and does not change if there is no status key in config file.

It's method getDefaultStatus() is initially used, when configuring StatusConfiguration from found config file. Status doesn't change if file misses status key node.
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.

@lukaszspyra,

Except the remarks below, it looks good to me.

Since this is a breaking change, I posted a thread on dev to check for consensus.

@rgoers
Copy link
Member

rgoers commented Jul 26, 2023

I am not in favor of this. We should fix the original problem that a JSON or YAML configuration is found but dependencies are missing to log at error instead.

@lukaszspyra
Copy link
Contributor Author

I am not in favor of this. We should fix the original problem that a JSON or YAML configuration is found but dependencies are missing to log at error instead.

Looking at the thread on dev, I believe majority did not like the idea of changing default StatusLogger level, so changes reverted and error logged as suggested (JSON/YAML ConfigurationFactory is not active and found corresponding config file).

@ppkarwasz
Copy link
Contributor

@lukaszspyra,

Sorry for the delayed answer.

Your PR looks good to me, if the default status logger level of ERROR can not be changed.

Personally I don't agree that WARN messages should be hidden by default and I prefer your original version, but there is no consensus on this matter in the PMC. @vy what do you think?

vy added a commit that referenced this pull request Jan 22, 2024
…`ConfigurationFactory`s (#1592)

Co-authored-by: Łukasz Spyra (@lukaszspyra)
@vy
Copy link
Member

vy commented Jan 22, 2024

@lukaszspyra, I am sorry for the delay. There were more pressing issues. As a part of our ongoing effort to clean up the log4j-api in 2.x, we are re-visiting the idea of decreasing the default StatusLogger level from ERROR to WARN. PMC is mostly concerned about the noise this will create for existing users.

Once the default level is set to WARN, some warnings will start getting displayed on unrecognized/ignored properties. This is good. OTOH, there will also appear some not much useful warnings, e.g., The bufferSize is set to ... but bufferedIo is false. Would you be interested in fixing these (e.g., lowering the level of such messages to INFO or conditionally enabling them) and adding that your PR? I believe we (@ppkarwasz?) can provide assistance for such an effort. PMC needs to be convinced anyway. @ppkarwasz will take care of those.

@ppkarwasz ppkarwasz self-assigned this Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants