You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/06/09 13:29:32 UTC
[GitHub] [logging-log4j2] yueki1993 opened a new pull request, #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
yueki1993 opened a new pull request, #924:
URL: https://github.com/apache/logging-log4j2/pull/924
https://issues.apache.org/jira/browse/LOG4J2-3534
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz merged pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
ppkarwasz merged PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r893542193
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java:
##########
@@ -326,4 +326,9 @@ public void testEnhancedRollingFileAppender() throws Exception {
}
}
+ @Override
Review Comment:
Should I add test in `Log4j1ConfigurationFactoryTest`, too?
I don't understand the difference of `PropertiesConfigurationTest` and it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894062873
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java:
##########
@@ -129,6 +129,28 @@ public void testConsoleAppenderLevelRangeFilter() throws Exception {
assertEquals(Level.ALL, customFilterReal.getMinLevel());
final LevelRangeFilter defaultFilter = (LevelRangeFilter) filters[1];
assertEquals(Level.TRACE, defaultFilter.getMinLevel());
+
+ final ListAppender legacyAppender = (ListAppender) ((AppenderAdapter.Adapter) appender).getAppender();
+ final Logger logger = LogManager.getLogger(PropertiesConfigurationTest.class);
+
+ // denied???
+ logger.trace("TRACE");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.debug("DEBUG");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.info("INFO");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.warn("WARN");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.error("ERROR");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.fatal("FATAL");
+ assertEquals(0, legacyAppender.getEvents().size());
Review Comment:
this test fails in ba48d2b, so I took a look here. (bac3547d9aa79f8e76e1cc3efa8254ff2e053510)
Under this setting, this rejects all output logs.
I believe we expect here we get all messages accepted because we set `levelMin=ALL`.
https://github.com/yueki1993/logging-log4j1/commit/e104a91fdfc9cea5c5c05bdb74d90a7236665edf
(see: https://issues.apache.org/jira/browse/LOG4J2-2315)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894992070
##########
log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/LevelRangeFilterBuilder.java:
##########
@@ -83,19 +86,23 @@ public Filter parse(PropertiesConfiguration config) {
}
private Filter createFilter(String levelMax, String levelMin, boolean acceptOnMatch) {
- Level max = Level.FATAL;
- Level min = Level.TRACE;
+ Level max = Level.OFF;
+ Level min = Level.ALL;
if (levelMax != null) {
- max = OptionConverter.toLevel(levelMax, org.apache.log4j.Level.FATAL).getVersion2Level();
+ max = OptionConverter.toLevel(levelMax, org.apache.log4j.Level.OFF).getVersion2Level();
}
if (levelMin != null) {
- min = OptionConverter.toLevel(levelMin, org.apache.log4j.Level.DEBUG).getVersion2Level();
+ min = OptionConverter.toLevel(levelMin, org.apache.log4j.Level.ALL).getVersion2Level();
}
org.apache.logging.log4j.core.Filter.Result onMatch = acceptOnMatch
? org.apache.logging.log4j.core.Filter.Result.ACCEPT
: org.apache.logging.log4j.core.Filter.Result.NEUTRAL;
Review Comment:
OK, I won't add warning log in Log4j2 factory too because if someone misconfigures LogLevelRangeFilter, no logs will be output - they should be able to notice misconfiguration easily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894062873
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java:
##########
@@ -129,6 +129,28 @@ public void testConsoleAppenderLevelRangeFilter() throws Exception {
assertEquals(Level.ALL, customFilterReal.getMinLevel());
final LevelRangeFilter defaultFilter = (LevelRangeFilter) filters[1];
assertEquals(Level.TRACE, defaultFilter.getMinLevel());
+
+ final ListAppender legacyAppender = (ListAppender) ((AppenderAdapter.Adapter) appender).getAppender();
+ final Logger logger = LogManager.getLogger(PropertiesConfigurationTest.class);
+
+ // denied???
+ logger.trace("TRACE");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.debug("DEBUG");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.info("INFO");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.warn("WARN");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.error("ERROR");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.fatal("FATAL");
+ assertEquals(0, legacyAppender.getEvents().size());
Review Comment:
this test fails in ba48d2b, so I took a look here.
Under this setting, this rejects all output logs.
I believe we expect here we get all messages accepted because we set `levelMin=ALL`.
https://github.com/yueki1993/logging-log4j1/commit/e104a91fdfc9cea5c5c05bdb74d90a7236665edf
(see: https://issues.apache.org/jira/browse/LOG4J2-2315)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894784572
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java:
##########
@@ -326,4 +326,9 @@ public void testEnhancedRollingFileAppender() throws Exception {
}
}
+ @Override
Review Comment:
`Log4j1ConfigurationFactory` is an old factory that does not use builders. No need to add it there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894062873
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java:
##########
@@ -129,6 +129,28 @@ public void testConsoleAppenderLevelRangeFilter() throws Exception {
assertEquals(Level.ALL, customFilterReal.getMinLevel());
final LevelRangeFilter defaultFilter = (LevelRangeFilter) filters[1];
assertEquals(Level.TRACE, defaultFilter.getMinLevel());
+
+ final ListAppender legacyAppender = (ListAppender) ((AppenderAdapter.Adapter) appender).getAppender();
+ final Logger logger = LogManager.getLogger(PropertiesConfigurationTest.class);
+
+ // denied???
+ logger.trace("TRACE");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.debug("DEBUG");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.info("INFO");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.warn("WARN");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.error("ERROR");
+ assertEquals(0, legacyAppender.getEvents().size());
+ // denied???
+ logger.fatal("FATAL");
+ assertEquals(0, legacyAppender.getEvents().size());
Review Comment:
this test fails in ba48d2b, so I took a look here.
Under this setting, this rejects all output logs.
I believe we expect here we get all messages accepted because we set `levelMin=ALL`.
(see: https://issues.apache.org/jira/browse/LOG4J2-2315)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894211536
##########
log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/LevelRangeFilterBuilder.java:
##########
@@ -83,19 +86,23 @@ public Filter parse(PropertiesConfiguration config) {
}
private Filter createFilter(String levelMax, String levelMin, boolean acceptOnMatch) {
- Level max = Level.FATAL;
- Level min = Level.TRACE;
+ Level max = Level.OFF;
+ Level min = Level.ALL;
if (levelMax != null) {
- max = OptionConverter.toLevel(levelMax, org.apache.log4j.Level.FATAL).getVersion2Level();
+ max = OptionConverter.toLevel(levelMax, org.apache.log4j.Level.OFF).getVersion2Level();
}
if (levelMin != null) {
- min = OptionConverter.toLevel(levelMin, org.apache.log4j.Level.DEBUG).getVersion2Level();
+ min = OptionConverter.toLevel(levelMin, org.apache.log4j.Level.ALL).getVersion2Level();
}
org.apache.logging.log4j.core.Filter.Result onMatch = acceptOnMatch
? org.apache.logging.log4j.core.Filter.Result.ACCEPT
: org.apache.logging.log4j.core.Filter.Result.NEUTRAL;
Review Comment:
Is it better to output warning log if `max < min` here?
Current released implementation should work if we swap `levelMin` and `levelMax` in `log4j.properties`.
This is degradation for people who configure log4j like that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#discussion_r894787786
##########
log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/LevelRangeFilterBuilder.java:
##########
@@ -83,19 +86,23 @@ public Filter parse(PropertiesConfiguration config) {
}
private Filter createFilter(String levelMax, String levelMin, boolean acceptOnMatch) {
- Level max = Level.FATAL;
- Level min = Level.TRACE;
+ Level max = Level.OFF;
+ Level min = Level.ALL;
if (levelMax != null) {
- max = OptionConverter.toLevel(levelMax, org.apache.log4j.Level.FATAL).getVersion2Level();
+ max = OptionConverter.toLevel(levelMax, org.apache.log4j.Level.OFF).getVersion2Level();
}
if (levelMin != null) {
- min = OptionConverter.toLevel(levelMin, org.apache.log4j.Level.DEBUG).getVersion2Level();
+ min = OptionConverter.toLevel(levelMin, org.apache.log4j.Level.ALL).getVersion2Level();
}
org.apache.logging.log4j.core.Filter.Result onMatch = acceptOnMatch
? org.apache.logging.log4j.core.Filter.Result.ACCEPT
: org.apache.logging.log4j.core.Filter.Result.NEUTRAL;
Review Comment:
There was no warning in Log4j 1.x, so I wouldn't add it here. You can add a warning in the Log4j2 factory `LevelRangeFilter#createFilter` (using the `StatusLogger`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] yueki1993 commented on pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
yueki1993 commented on PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#issuecomment-1152876943
Hello @ppkarwasz,
Thank you for your review. I pushed `src/changes/changes.xml`.
Is it better to squash my commits?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #924: [LOG4J2-3534] Fix LevelRangeFilterBuilder to align with log4j1's behavior
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #924:
URL: https://github.com/apache/logging-log4j2/pull/924#issuecomment-1153668146
@yueki1993,
There is no need to squash the PR, since Github can do it automatically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org