Remove the slf4j-simple dependency, as we use Flogger

[Jdban]

It would be good to see the logging configuration file setup in it's entirety

[oferb]

I agree. Can you give an example of what you'd like to see, or specific features that are missing?

[jbduncan]

My personal thought on this would be to incorporate google/flogger in some way. :)

[oferb]

We already use Flogger throughout: https://github.com/google/startup-os/search?q=com.google.common.flogger.FluentLogger&unscoped_q=com.google.common.flogger.FluentLogger

[jbduncan]

Oh, excuse me! Thanks for showing me that StartupOS already incorporates Flogger! 👍

I'm personally not fussed which backend logging framework is used, as long as it's not an unmaintained one. (log4j 1 comes to mind as a framework to avoid, and log4j 2 and logback come to mind as perfectly good ones to consider.)

[Jdban]

Ah, I didn't notice Flogger either. It's kinda weird to have both slf4j/slf4j-simple and flogger though, right? Looking closer, it looks almost like slf4j isn't used anywhere, maybe it should be removed then?

[oferb]

So that's a good question, some of our dependencies from Maven need it. Not sure if there's anything we can do about that.

[jbduncan]

My guess based on my limited understanding is that the slf4j-simple artifact should be unneeded if (1) we decide to switch Flogger's backend from what it is currently (JUL or otherwise) to log4j or logback, and (2) if we go for log4j, we then put an slf4j-to-log4j bridge jar on the classpath.

[oferb]

Ok, I now remembered why I didn't completely removed the dependency on slf4j-simple. When I tried, I got these pesky logs every time I ran something.

I just tried it again, so here's a PR for removing slf4j-simple: https://github.com/google/startup-os/pull/152

Running this with the PR: bazel run //tools/reviewer/service:test_tool Would print this out:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

Running without the PR doesn't. I couldn't figure out how to remove these warnings, so I just left the dependencies there.

[jbduncan]

@oferb Yeah, that all makes sense to me.

I realise that I've said this already, but I believe that if/once a backend logging framework for Flogger other than JUL is chosen, then it should remove the need for StartupOS to use slf4j-simple. :)

[jbduncan]

Hmm. It occurs to me that for log4j2 or logback to be used as the backend for Flogger, either of these issues on the Flogger issue tracker will need to be resolved first: https://github.com/google/flogger/issues/24, https://github.com/google/flogger/issues/25.

[jbduncan]

(IMO, log4j2 and logback are perfectly reasonable replacements for JUL, which is why I mentioned them as potential backends for Flogger in my last comment.)

[oferb]

Thanks for researching this! Taking a little step back, do we want to replace JUL with log4j2/logback? If so, why? :)

[jbduncan]

I think the only reason we'd want to do it so far is so that we can ultimately remove the dependency on slf4j-simple (it feels strange to me that we even need it in the first place).

To explain further, my educated guess based on my limited understanding of Java logging frameworks, is that if we decide to make use of log4j2 or logback in StartupOS for whatever reason (e.g. if we discover that they'd allow us the flexibility to customise the log messages produced by Flogger in a way that it's current default backend framework, JUL, cannot), then once we depend on log4j2/logback, it should just be a simple matter of replacing the slf4j-simple artifact on the classpath with the equivalent slf4j-log4j2 or slf4j-logback bridge artifact, as slf4j will take care of the rest.

[oferb]

Ok, got it! So until we have a reason to use one of the other backends (other than to potentially remove the dependency on slf4j-simple), I'm moving this to P3. Thanks for the explanation :-)