withMinimalLevel feels wrong / doesn't work in slf4j

[kubukoz]

Hi!

I've noticed weird behavior: I was using the monoid logger to combine two loggers (consoleLogger.withMinimalLevel(Info) |+| fileLogger(minLevel=Debug).

I passed this to the sfl4j interop without changes, and to my surprise, I started seeing DEBUG logs in my console! When I removed the file logger, leaving just the console one, I only saw INFO and above in the console.

I believe the culprit is this: the slfj4 logger calls the underlying logger's log method:

def log(msg: LoggerMessage): F[Unit]

And so does the Monoid logger. When I used the monoid logger directly with methods like .debug, I wouldn't notice it, because it would call the underlying debug methods:

def debug[M](msg: => M)(implicit render: Render[M], position: Position): F[Unit] =
  x.debug(msg) *> y.debug(msg)

So the level-based filtering would apply, and everything would work well.

But because the sfl4j logger calls log directly, after only checking that the message fits the underlying logger level (which, in case of MonoidLogger, is the lowest one - Debug), all loggers that take part in a monoid composition will print the message, even if they are wrapped in withMinimalLevel(max).

I think this is wrong - and the culprit is probably in the design of DefaultLogger and the log signatures:

def log(msg: LoggerMessage): F[Unit]

This one never does any filtering.

private def log[M](level: Level, msg: => M, ctx: Map[String, String] = Map.empty, t: Option[Throwable] = None)(
    implicit render: Render[M],
    position: Position
): F[Unit]

This one does the filtering and, if the level matches the underlying logger, forwards to def log(LoggerMessage).

I think DefaultLogger should instead:

• be a concrete class with a log: LoggerMessage=>F[Unit] parameter or an abstract class with an abstract method like that
• implement every other method like def trace[M](msg: => M, ctx: Map[String, String])(implicit render: Render[M], position: Position): F[Unit] as plain forwarders to a still- abstract log method: log(Level.Trace, msg, ctx)
• not have any level-based filtering

A new abstraction, like MinLevelLogger should be introduced. It would:

• be a wrapper over other loggers
• extend DefaultLogger to get all methods (except for log) implemented
• implement def log(LoggerMessage) and do the filtering based on the argument's level - further forwarding to underlying or F.unit.

[kubukoz]

Oh, and val minLevel could disappear from loggers. It would only be a parameter to MinLevelLogger.

An additional benefit of this redesign would be having a single place where level filtering is done - right now I can see it in DefaultLogger and OdinLoggerAdapter, there are possibly more.

I'm not sure how bad this will be for performance... but it shouldn't be too hard to implement and just see.

[sergeykolbasov]

Good catch. Need to think about it further.

Original implementation had a MinLevelLogger proxy, but it was impossible to lower the level after it's set there

[kubukoz]

#244 worked, can you make a release? :)

[sergeykolbasov]

Sure. 0.11.0 is on the way to maven

[kubukoz]

Thanks!