Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-1911

DynamicThresholdFilter defaultThreshold is not used to compare against event's level when there's no matching key found.

    Details

    • Type: Documentation
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.8.2
    • Fix Version/s: None
    • Component/s: Filters

      Description

      The defaultThreshold property is not honored as documented :

      defaultThreshold Level of messages to be filtered. If there is no matching key in the key/value pairs then this level will be compared against the event's level.

      after carefully examining the source code, I found the following code is called:

      DynamicThresholdFilter.java
       private Result filter(Level level, ReadOnlyStringMap contextMap) {
              String value = (String)contextMap.getValue(this.key);
              if(value != null) {
                  Level ctxLevel = (Level)this.levelMap.get(value);
                  if(ctxLevel == null) {
                      ctxLevel = this.defaultThreshold;
                  }
      
                  return level.isMoreSpecificThan(ctxLevel)?this.onMatch:this.onMismatch;
              } else {
                  return Result.NEUTRAL;
              }
          }
      

      where level is the event's level, when there's no matching key found contextMap, Result.NEUTRAL is mistakenly returned, instead of against this.defaultThreshold.

        Activity

        Hide
        garydgregory Gary Gregory added a comment -

        Thank you for the report.

        Can you please provide a patch with a uni test?

        Show
        garydgregory Gary Gregory added a comment - Thank you for the report. Can you please provide a patch with a uni test?
        Hide
        metrxqin@gmail.com Jerry Chin added a comment -

        Gary Gregory can you tell where I can supply a patch?

        Show
        metrxqin@gmail.com Jerry Chin added a comment - Gary Gregory can you tell where I can supply a patch?
        Hide
        garydgregory Gary Gregory added a comment -

        You can either attach a diff file to this ticket or create a PR on GitHub.

        Show
        garydgregory Gary Gregory added a comment - You can either attach a diff file to this ticket or create a PR on GitHub.
        Show
        metrxqin@gmail.com Jerry Chin added a comment - see https://github.com/JerryChin/dynamicfiltertest
        Hide
        garydgregory Gary Gregory added a comment -

        Hi Jerry,

        I do not see a patch or a PR. You may want to read on how to provide a "Pull Request" on GitHub: https://help.github.com/articles/about-pull-requests/

        Gary

        Show
        garydgregory Gary Gregory added a comment - Hi Jerry, I do not see a patch or a PR. You may want to read on how to provide a "Pull Request" on GitHub: https://help.github.com/articles/about-pull-requests/ Gary
        Hide
        ralph.goers@dslextreme.com Ralph Goers added a comment - - edited

        I was able to add a test case that shows the problem and implemented a fix but after thinking about the implications of the fix for the following reasons I think the right thing to do is modify the documentation:
        1. This filter has always worked this way. User's configurations would start behaving differently if they upgrade.
        2. Currently it only compares against the default threshold if the key and value are present but no level is configured to check for that value. This change would have it also compare against the default threshold if the key is not present or the value is null, thus having the default threshold be applied for two different situations.
        3. If a key is not present in a log event this filter will currently return NEUTRAL, passing the event on for further filtering. With this change, if neither onMisMatch or onMatch are NEUTRAL then no further filtering can occur.
        4. As stated in the documentation noted above, if there is no matching KeyValuePair only then is the default threshold used. The documentation on defaultThreshold says nothing about the case where they key is not present in the ThreadContext map.

        These are significant changes in behavior. Therefore I recommend that the the documentation for defaultThreshold be modified to make it be a bit clearer. The description of the filter should also be improved to say the filter returns NEUTRAL if the specified item is not present in the ThreadContext.

        Show
        ralph.goers@dslextreme.com Ralph Goers added a comment - - edited I was able to add a test case that shows the problem and implemented a fix but after thinking about the implications of the fix for the following reasons I think the right thing to do is modify the documentation: 1. This filter has always worked this way. User's configurations would start behaving differently if they upgrade. 2. Currently it only compares against the default threshold if the key and value are present but no level is configured to check for that value. This change would have it also compare against the default threshold if the key is not present or the value is null, thus having the default threshold be applied for two different situations. 3. If a key is not present in a log event this filter will currently return NEUTRAL, passing the event on for further filtering. With this change, if neither onMisMatch or onMatch are NEUTRAL then no further filtering can occur. 4. As stated in the documentation noted above, if there is no matching KeyValuePair only then is the default threshold used. The documentation on defaultThreshold says nothing about the case where they key is not present in the ThreadContext map. These are significant changes in behavior. Therefore I recommend that the the documentation for defaultThreshold be modified to make it be a bit clearer. The description of the filter should also be improved to say the filter returns NEUTRAL if the specified item is not present in the ThreadContext.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 189070de459169d0af7ca24df9654ac924402ace in logging-log4j2's branch refs/heads/master from Ralph Goers
        [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=189070d ]

        LOG4J2-1911 Improve the documentation of the DynamicThresholdFilter

        Show
        jira-bot ASF subversion and git services added a comment - Commit 189070de459169d0af7ca24df9654ac924402ace in logging-log4j2's branch refs/heads/master from Ralph Goers [ https://git-wip-us.apache.org/repos/asf?p=logging-log4j2.git;h=189070d ] LOG4J2-1911 Improve the documentation of the DynamicThresholdFilter
        Hide
        ralph.goers@dslextreme.com Ralph Goers added a comment -

        I have changed the documentation of the DynamicThresholdFilter to add "If the log event does not contain the specified ThreadContext item NEUTRAL will be returned."

        I have added documentation on the key attribute.

        I have modified the documentation of defaultThreshold to read "The default threshold only applies if the log event contains the specified ThreadContext Map item and its value does not match any key in the key/value pairs."

        Please verify and close.

        Show
        ralph.goers@dslextreme.com Ralph Goers added a comment - I have changed the documentation of the DynamicThresholdFilter to add "If the log event does not contain the specified ThreadContext item NEUTRAL will be returned." I have added documentation on the key attribute. I have modified the documentation of defaultThreshold to read "The default threshold only applies if the log event contains the specified ThreadContext Map item and its value does not match any key in the key/value pairs." Please verify and close.

          People

          • Assignee:
            ralph.goers@dslextreme.com Ralph Goers
            Reporter:
            metrxqin@gmail.com Jerry Chin
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development