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

Set the default minLevel and maxLevel of LevelRangeFilter #1503

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

vy
Copy link
Member

@vy vy commented Jun 5, 2023

The default minLevel and maxLevel of LevelRangeFilter are both set to ERROR. As hinted in #1495, this is unintuitive and excludes FATAL by default, unless minLevel is explicitly overridden. This PR replaces these defaults with OFF and ALL, respectively, and adds documentation.

Warning! This change breaks the backward compatibility. Though the expected impact is low, since

  1. The filter wasn't documented
  2. The majority of users are expected to only configure a maxLevel. This will still work, yet, after this PR, the filter will also allow events with levels higher than ERROR.

@vy vy force-pushed the LevelRangeFilter-defaults branch from 0a097b4 to 91cb61b Compare June 5, 2023 20:14
@vy vy added this to the 2.21.0 milestone Jun 5, 2023
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.

Looks good to me.

Comment on lines +78 to +79
@PluginAttribute("onMatch") final Result onMatch,
@PluginAttribute("onMismatch") final Result onMismatch) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might take this opportunity to switch to the builder pattern. AbstractFilter.AbstractFilterBuilder has already defaults for onMatch and onMismatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have indeed seen that TODO and checked it. Though the required LoC doubles. Hence, sticked to this version.

@garydgregory
Copy link
Member

LGTM

@vy vy merged commit 911fd82 into 2.x Jun 6, 2023
@vy vy deleted the LevelRangeFilter-defaults branch June 6, 2023 18:14
vy added a commit that referenced this pull request Jun 6, 2023
@ppkarwasz ppkarwasz modified the milestones: 2.21.0, 2.20.1 Aug 1, 2023
jvz pushed a commit to jvz/logging-log4j2 that referenced this pull request Sep 1, 2023
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.

3 participants