slf4j-jdk-platform-logging always logging Level.ALL messages regardless of logger level

[peterhalicky]

So, sun.security.ssl.SSLLogger logs its finest events with Level.ALL. While that on its own is a bug (and I reported it), I'd say slf4j-jdk-platform-logging is still handling it incorrectly. It gets written out as TRACE level, regardless of level setting of the logger.

I'm all for treating the Level.ALL as TRACE, but it should also be filtered as such. I suppose in SLF4JPlatformLogger.log(...) method instead of current:

        if (jplLevel == Level.ALL) {
            performLog(org.slf4j.event.Level.TRACE, bundle, msg, thrown, params);
            return;
        }

it should be:

        if (jplLevel == Level.ALL) {
            if (slf4jLogger.isEnabledForLevel(org.slf4j.event.Level.TRACE)) {
                performLog(org.slf4j.event.Level.TRACE, bundle, msg, thrown, params);
            }
            return;
        }

I will gladly submit a PR if this looks okay to the relevant people here.

[ceki]

@peterhalicky As you mention, it is quite incorrect to log with the Level.ALL which is below Level.FINEST/TRACE. it is means that all logs should be allowed. I agree with your assessment regarding filtering using Level.TRACE. I was probably confused regarding the meaning of Level.ALL when writing the code below:

private void log(Level jplLevel, ResourceBundle bundle, String msg, Throwable thrown, Object... params) {
        if (jplLevel == Level.OFF)
            return;

        if (jplLevel == Level.ALL) {
            performLog(org.slf4j.event.Level.TRACE, bundle, msg, thrown, params);
            return;
        }
   ...

[ceki]

@peterhalicky Fixed in commit f7b34c2. Let me know what you think.

[peterhalicky]

@ceki I'm not sure if I agree with mapping the Level.OFF level to Level.ERROR -- which in most typical configuration will be written to the log; I'd guess if someone logs with Level.OFF, he probably means to never actually write it in the log -- so if not dropping the message completely I'd probably instead map it to Level.TRACE (kinda same as Level.ALL, which is counterintuitive -- but it's counterintuitive to actually use those levels anyway).

[ceki]

Indeed, Level.OFF and Level.ALL are a quite counter intuitive when used as logging levels.

However, no one is supposed to use Level.OFF and Level.ALL for logging. They are intended for filtering and are vestigial from log4j 1.x where it was possible to subclass Level to create custom levels. In logback and log4j2 the Level class is final. Thus, Level.ALL makes little sense while Level.OFF can be used to filter out all logging.

Also, Level.OFF has the highest priority (not the lowest). Level.ALL has the lowest priority (not the highest).

[peterhalicky]

What I'm worried about is that when someone non-sensically uses Level.OFF for logging and we map it to Level.ERROR, it will be difficult to get rid of those messages.

[ceki]

On the contrary, in that case, you would assign the level of the logger to Level.OFF and the message would not be print (which is the desired outcome).

[peterhalicky]

Well, but if the logger is actually logging something useful at Level.INFO, WARN or ERROR, we'd miss that.

[ceki]

With only the logger-level filter, it is impossible for a logger to simultaneously suppress messages of Level.ERROR and allow messages of Level.WARN. When Level.OFF is mapped to SLF4J-Level.ERROR, you can at least suppress the message if you really wanted to.