Use the SLF4J logging API directly #13285

chrisvest · 2023-03-19T18:46:57Z

Motivation:
SLF4J has been the defacto standard logging API in the Java world for a while. Normally we don't like to expose vendor APIs, but I think we can make an exception in this case. By using it directly, we can get rid of our own logging API code that isn't inherently relevant to what Netty does. It also means we can more easily make use of more advanced logging features like MDC and LoggingEventBuilders.

Modification:
Remove our internal logging API and use SLF4J everywhere. Switch back from log4j to using logback for our testing.

Result:
Less code to maintain, and everyone gets to use the same logging API.

Motivation: SLF4J has been the defacto standard logging API in the Java world for a while. Normally we don't like to expose vendor APIs, but I think we can make an exception in this case. By using it directly, we can get rid of our own logging API code that isn't inherently relevant to what Netty does. It also means we can more easily make use of more advanced logging features like MDC and LoggingEventBuilders. Modification: Remove our internal logging API and use SLF4J everywhere. Switch back from log4j to using logback for our testing. Result: Less code to maintain, and everyone gets to use the same logging API.

chrisvest · 2023-03-19T18:47:28Z

Draft because the Graal meta-data needs to be updated as well.

jchrys · 2023-03-20T01:13:41Z

Thank you for this PR! I love this change.
while reviewing the changes, I noticed that some license-related files might need to be modified, as it seems that we're no longer dependent on log4j.
Thanks very much!

codec-http2/src/test/java/io/netty5/handler/codec/http2/Http2StreamChannelBootstrapTest.java

common/src/main/java/io/netty5/util/NetUtil.java

example/src/main/java/io/netty5/example/factorial/FactorialServer.java

example/src/main/resources/logback.xml

handler/src/main/java/io/netty5/handler/logging/LogLevel.java

resolver-dns/src/test/resources/logback-test.xml

normanmaurer · 2023-03-20T15:13:54Z

transport-native-epoll/src/test/java/io/netty5/channel/epoll/EpollReuseAddrTest.java

@@ -81,7 +80,7 @@ public class EpollReuseAddrTest {
                BUGFIX = 0;
            }
        } else {
-            LOGGER.log(InternalLogLevel.INFO, "Unable to parse kernel version: " + kernelVersion);
+            LOGGER.info("Unable to parse kernel version: {}", kernelVersion);


why do you sometimes use atInfo()... and sometimes the above pattern ? Imho we should be consistent. I suspect the pattern above creates less GC as there is no extra builder created.

The atLevel() method is the only way to dynamically specify the log level in their 2.0 API. There were two places where I used atWarn(), and I'll fix those.

transport-native-unix-common-tests/src/main/resources/logback-test.xml

transport/src/main/java/io/netty5/channel/DefaultChannelHandlerContext.java

This is not the same as regenerating the reflection configuration, which needs to happen, but let's see what the build says about it first.

chrisvest · 2023-03-22T19:41:45Z

Hmm… the h2spec testsuite runs inside the Maven process itself, and Maven depends on slf4j 1.7, with no apparent way override this in the plugin classloader. That's why that module fails with "no such method error".

…he correct classpath The h2spec-maven-plugin runs the server with the Maven-internal classloader as the parent classloader, which means we cannot override the slf4j version that Maven otherwise uses. By dropping the parent classloader, and only relying on our own URLClassLoader, we get the correct test-classpath that we want.

chrisvest · 2023-03-23T00:27:03Z

Hmm… the h2spec testsuite runs inside the Maven process itself, and Maven depends on slf4j 1.7, with no apparent way override this in the plugin classloader. That's why that module fails with "no such method error".

Managed to work around this with a bit of class loader magic.

trustin · 2023-03-23T12:27:02Z

handler/src/main/java/io/netty5/handler/logging/LogLevel.java


 /**
- * Maps the regular {@link LogLevel}s with the {@link InternalLogLevel} ones.
+ * Logging level for configuring {@link LoggingHandler}.
 */
 public enum LogLevel {


But shouldn't we make it package-private and hide it from the public API?

It was the whole point to make this public and so "hide" slf4j from the public API .

trustin · 2023-03-23T12:29:10Z

resolver-dns/src/main/java/io/netty5/resolver/dns/LoggingDnsQueryLifecycleObserver.java

@@ -46,38 +46,38 @@ public void queryWritten(InetSocketAddress dnsServerAddress, Future<Void> future
    @Override
    public void queryCancelled(int queriesRemaining) {
        if (dnsServerAddress != null) {
-            logger.log(level, "from {} : {} cancelled with {} queries remaining", dnsServerAddress, question,
+            logger.atLevel(level).log("from {} : {} cancelled with {} queries remaining", dnsServerAddress, question,


Could extract logger.atLevel(level) into a member field so we don't have to call it every time. I guess this could be potentially creating a new instance for each log message.

The API is meant to build a new LogEvent every time. Otherwise you'll carry over data from previous log messages. For instance, the arguments (dnsServerAddress, etc.) given here will accumulate.

If we cache the builder we will also not be told about runtime log level changes.

testsuite-http2/pom.xml

testsuite-http2/src/main/java/io/netty5/testsuite/http2/Http2Server.java

trustin · 2023-03-23T14:22:19Z

Hmm? Why hide SLF4J from the public API? We pull it in as a non-shaded

dependency and it's already the de facto standard logging facade in Java ecosystem.

normanmaurer · 2023-03-23T14:37:02Z

Hmm? Why hide SLF4J from the public API? We pull it in as a non-shaded
dependency and it's already the de facto standard logging facade in Java ecosystem.

imho it is far better to just have it as an internal dependency then leaking it to the public api.

chrisvest · 2023-03-23T15:29:43Z

@normanmaurer @trustin I've addressed your comments. Please take a look.

violetagg · 2023-03-28T17:39:06Z

@chrisvest @normanmaurer There is a bit of inconsistency now in the API

The LoggingHandler works with LogLevel and thus the slf4j is not exposed to the public API
But for example Http2FrameLogger and LoggingDnsQueryLifeCycleObserverFactory are exposing directly slf4j

Are you going at some point to expose slf4j everywhere and remove LogLevel?

Adaptations made for the Netty5 SLF4J netty/netty#13285 PR. socks-proxy and codec-multipart netty contrib projects have also been updated, because io.netty5.util.internal.logging.InternalLoggerFactory is not supported anymore.

chrisvest · 2023-03-28T19:38:00Z

@violetagg Oops, looks like we didn't revert all the usages. I'll make a PR.

Motivation: The is _a lot_ going on in netty#13285, and we missed that we exposed the SLF4J level in a couple of places. We also _somehow_ missed that we broke compilation of EchoIT in the buffer-memory-segment module. Modification: Replace the remaining places that exposed SLF4J Level to the public API, and fix the compilation. Result: No more left-overs to clean up after netty#13285

chrisvest · 2023-03-28T22:15:53Z

This should fix it: #13304

Motivation: The is _a lot_ going on in #13285, and we missed that we exposed the SLF4J level in a couple of places. We also _somehow_ missed that we broke compilation of EchoIT in the buffer-memory-segment module. Modification: Replace the remaining places that exposed SLF4J Level to the public API, and fix the compilation. Result: No more left-overs to clean up after #13285 Co-authored-by: Norman Maurer <norman_maurer@apple.com>

chrisvest requested a review from normanmaurer March 19, 2023 18:46

normanmaurer reviewed Mar 20, 2023

View reviewed changes

codec-http2/src/test/java/io/netty5/handler/codec/http2/Http2StreamChannelBootstrapTest.java Show resolved Hide resolved

normanmaurer requested changes Mar 20, 2023

View reviewed changes

chrisvest added 2 commits March 21, 2023 08:38

Add license headers to log configuration files

249848e

Bring back LogLevel for LoggingHandler

88abc90

normanmaurer approved these changes Mar 22, 2023

View reviewed changes

chrisvest added 2 commits March 22, 2023 08:58

Remove log4j from NOTICE file

78798a2

Remove logging classes from Graal reflection configurations

6eccde8

This is not the same as regenerating the reflection configuration, which needs to happen, but let's see what the build says about it first.

chrisvest added 3 commits March 22, 2023 12:58

Update slf4j and logback to the latest versions

15ef83e

Avoid writing "This is most likely a bug" twice in the same log message

86f6e1c

chrisvest marked this pull request as ready for review March 23, 2023 00:26

trustin reviewed Mar 23, 2023

View reviewed changes

chrisvest added 3 commits March 23, 2023 08:06

Don't use logger.at<level> unless we have to

dde11fc

Avoid calling toString on disposed object

0bc9768

Add a comment about the classloader shenanigans in our h2spec testsuite

b2efcd9

normanmaurer approved these changes Mar 27, 2023

View reviewed changes

chrisvest merged commit 795db4a into netty:main Mar 27, 2023

chrisvest deleted the 5x-slf4j branch March 27, 2023 16:26

jchrys mentioned this pull request Mar 28, 2023

Cleanup dependencies and licenses #13302

Merged

pderop mentioned this pull request Mar 28, 2023

Adapt to Netty 5 SLF4J changes reactor/reactor-netty#2749

Merged

chrisvest mentioned this pull request Mar 28, 2023

A few more fix-ups after the logging changes in #13285 #13304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the SLF4J logging API directly #13285

Use the SLF4J logging API directly #13285

chrisvest commented Mar 19, 2023

chrisvest commented Mar 19, 2023

jchrys commented Mar 20, 2023

normanmaurer Mar 20, 2023

chrisvest Mar 23, 2023

chrisvest commented Mar 22, 2023

chrisvest commented Mar 23, 2023

trustin Mar 23, 2023

normanmaurer Mar 23, 2023

trustin Mar 23, 2023

chrisvest Mar 23, 2023

chrisvest Mar 23, 2023

trustin commented Mar 23, 2023 via email

normanmaurer commented Mar 23, 2023

chrisvest commented Mar 23, 2023

violetagg commented Mar 28, 2023

chrisvest commented Mar 28, 2023

chrisvest commented Mar 28, 2023

Use the SLF4J logging API directly #13285

Use the SLF4J logging API directly #13285

Conversation

chrisvest commented Mar 19, 2023

chrisvest commented Mar 19, 2023

jchrys commented Mar 20, 2023

normanmaurer Mar 20, 2023

Choose a reason for hiding this comment

chrisvest Mar 23, 2023

Choose a reason for hiding this comment

chrisvest commented Mar 22, 2023

chrisvest commented Mar 23, 2023

trustin Mar 23, 2023

Choose a reason for hiding this comment

normanmaurer Mar 23, 2023

Choose a reason for hiding this comment

trustin Mar 23, 2023

Choose a reason for hiding this comment

chrisvest Mar 23, 2023

Choose a reason for hiding this comment

chrisvest Mar 23, 2023

Choose a reason for hiding this comment

trustin commented Mar 23, 2023 via email

normanmaurer commented Mar 23, 2023

chrisvest commented Mar 23, 2023

violetagg commented Mar 28, 2023

chrisvest commented Mar 28, 2023

chrisvest commented Mar 28, 2023