Repository: netty/netty
Issue: Use Log4j2 for logging
Labels: improvement

Log4j2 came out a while ago an claims to have significant [performance benefits](http://logging.apache.org/log4j/2.x/manual/async.html#Performance) over SLF4j variants (logback).  The new architecture claims to be much more asynchronous in nature and they are using the LMAX Disruptor to reduce contention in a multithreaded environment.  They also have an [slf4j adapter](http://logging.apache.org/log4j/2.x/log4j-to-slf4j/index.html) which may ease the initial transition.  It may be worth doing some investigation to see how much netty could benefit from this, what netty versions would be eligible for this change, and how big of an effort this would be.


Issue comments:
@trustin @normanmaurer - Not saying we have the bandwidth for this now, but WDYT?

@Scottmitch sounds good to me :+1: 

@Scottmitch sounds good to me. I wonder it will change our internal logging API though.

Yeah we should be careful about this.
-- 
Norman Maurer

On 3 Nov 2014 at 09:09:18, Trustin Lee (notifications@github.com) wrote:

@Scottmitch sounds good to me. I wonder it will change our internal logging API though.

—
Reply to this email directly or view it on GitHub.

Can't we just do another optional logger impl? 

I'm open to any changes in our internal logging API.  However, I wonder if we are going to end up with moving it out of the internal package because it may be very useful to do this:

``` java
public class MyNettyHandler extends ChannelHandlerAdapter {
    public void channelRead(ctx, msg) {
        ctx.logger().info("received {}", msg);
    }
}
```

.. where the returned logger will also log the additional information such as remote/local address.

You can do this. But please remove this point from your homepage then:

Ease of use
    No additional dependencies but JDK 1.5 (or above)

@WhiteTrashLord it's still optional since the logger is never exposed right away - just the facade.. if no logger is in the classpath it will default to jdk logging

@trustin I like the idea.

> Am 03.11.2014 um 09:16 schrieb Trustin Lee notifications@github.com:
> 
> I'm open to any changes in our internal logging API. However, I wonder if we are going to end up with moving it out of the internal package because it may be very useful to do this:
> 
> public class MyNettyHandler extends ChannelHandlerAdapter {
>     public void channelRead(ctx, msg) {
>         ctx.logger().info("received {}", msg);
>     }
> }
> .. where the returned logger will also log the additional information such as remote/local address.
> 
> —
> Reply to this email directly or view it on GitHub.

seems like the `ActorLogging`

@trustin - I also like the idea of exposing the logger.  I haven't looked into this yet but one thing to consider is it a problem (i.e. origin of log unclear) if the logger is initialized with the class type (i.e. `logger = LogManager.getLogger(MyClass.class)`)? I'm not sure if this is applicable for netty but this seems like a common pattern to initialize logger objects.

I think the context could provider an lazy initialized logger,once the user call `logger()`on it ,will first instantiate one if it dose not exist,and also expose the `Loggers`or `LogManager` for user to get an logger, by Class type,by channel ,channel context or etc.


Pull request review comments:
package private and final?

does `core` belong here, or should this just be optional and/or for testing?

It should be just for testing. Updated.

![MINOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-minor.png) Make the "logger" logger private static final. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AS1312)

package private ?

final ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

merge lines ?

can this be a singleton?

I saw other similar classes are not singleton. 

understood. I don't think we have to follow this "precedent" as the user could just use the singleton instead of creating a new object.

@normanmaurer - WDYT?

They are internal classes, couldn't we just fix all of them?

You mean make all final?

> Am 31.03.2016 um 07:38 schrieb Trustin Lee notifications@github.com:
> 
> In common/src/main/java/io/netty/util/internal/logging/Log4J2LoggerFactory.java:
> 
> > - \* version 2.0 (the "License"); you may not use this file except in compliance
> > - \* with the License. You may obtain a copy of the License at:
> > - *
> > - \*   http://www.apache.org/licenses/LICENSE-2.0
> > - *
> > - \* Unless required by applicable law or agreed to in writing, software
> > - \* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> > - \* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> > - \* License for the specific language governing permissions and limitations
> > - \* under the License.
> > - */
> >   +package io.netty.util.internal.logging;
> >   +
> >   +import org.apache.logging.log4j.LogManager;
> >   +
> >   +public final class Log4J2LoggerFactory extends InternalLoggerFactory {
> >   They are internal classes, couldn't we just fix all of them?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub

Making them all final singleton.

I think we should at least keep the public constructor to not break stuff as while its in the internal package user tend to set a logger

> Am 31.03.2016 um 07:46 schrieb Trustin Lee notifications@github.com:
> 
> In common/src/main/java/io/netty/util/internal/logging/Log4J2LoggerFactory.java:
> 
> > - \* version 2.0 (the "License"); you may not use this file except in compliance
> > - \* with the License. You may obtain a copy of the License at:
> > - *
> > - \*   http://www.apache.org/licenses/LICENSE-2.0
> > - *
> > - \* Unless required by applicable law or agreed to in writing, software
> > - \* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> > - \* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> > - \* License for the specific language governing permissions and limitations
> > - \* under the License.
> > - */
> >   +package io.netty.util.internal.logging;
> >   +
> >   +import org.apache.logging.log4j.LogManager;
> >   +
> >   +public final class Log4J2LoggerFactory extends InternalLoggerFactory {
> >   Making them all final singleton.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub

> Making them all final singleton.

I suggest that doing it in a separate PR since it's not related to the title.

In addition, should the constructor of these factory classes be private? If so, it will break some user codes since InternalLoggerFactory suggests using `InternalLoggerFactory.setDefaultFactory(new Log4JLoggerFactory());`

I wonder why a user calls `InternalLoggerFactory.setDefaultFactory()`. It tries to detect the right logger automatically in the order of following preference:
- SLF4J
- Log4J
- JUL

When does this fail and a user has to configure manually?

I'm not sure if it ever fails but I have seen multiple times where user specify it manually (for whatever reason). So I would say let use mark the constructor deprecated and add static fields that should be used in the future. WDYT ?

@trustin and I agree with @windie that we should just do this in another PR :+1: 

new PR for fixing others sound good to me. we can discuss in the other PR but if these classes are internal lets just fix them ... i would rather not carry around deprecated APIs when we don't have to.

> @trustin[1] and I agree with @windie[2] that we should just do this in
> another PR :+1:

SGTM

Links:
1. https://github.com/trustin
2. https://github.com/windie


Patch that resolved the issue:
diff --git a/common/pom.xml b/common/pom.xml
index a9f9fda6186..876faf3e5b1 100644
--- a/common/pom.xml
+++ b/common/pom.xml
@@ -61,6 +61,16 @@
       <artifactId>log4j</artifactId>
       <optional>true</optional>
     </dependency>
+    <dependency>
+      <groupId>org.apache.logging.log4j</groupId>
+      <artifactId>log4j-api</artifactId>
+      <optional>true</optional>
+    </dependency>
+    <dependency>
+      <groupId>org.apache.logging.log4j</groupId>
+      <artifactId>log4j-core</artifactId>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
   <build>
diff --git a/common/src/main/java/io/netty/util/internal/logging/Log4J2Logger.java b/common/src/main/java/io/netty/util/internal/logging/Log4J2Logger.java
new file mode 100644
index 00000000000..64bdfed2e06
--- /dev/null
+++ b/common/src/main/java/io/netty/util/internal/logging/Log4J2Logger.java
@@ -0,0 +1,180 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.util.internal.logging;
+
+import org.apache.logging.log4j.Logger;
+
+final class Log4J2Logger extends AbstractInternalLogger {
+
+    private static final long serialVersionUID = 5485418394879791397L;
+
+    private final transient Logger logger;
+
+    Log4J2Logger(Logger logger) {
+        super(logger.getName());
+        this.logger = logger;
+    }
+
+    @Override
+    public boolean isTraceEnabled() {
+        return logger.isTraceEnabled();
+    }
+
+    @Override
+    public void trace(String msg) {
+        logger.trace(msg);
+    }
+
+    @Override
+    public void trace(String format, Object arg) {
+        logger.trace(format, arg);
+    }
+
+    @Override
+    public void trace(String format, Object argA, Object argB) {
+        logger.trace(format, argA, argB);
+    }
+
+    @Override
+    public void trace(String format, Object... arguments) {
+        logger.trace(format, arguments);
+    }
+
+    @Override
+    public void trace(String msg, Throwable t) {
+        logger.trace(msg, t);
+    }
+
+    @Override
+    public boolean isDebugEnabled() {
+        return logger.isDebugEnabled();
+    }
+
+    @Override
+    public void debug(String msg) {
+        logger.debug(msg);
+    }
+
+    @Override
+    public void debug(String format, Object arg) {
+        logger.debug(format, arg);
+    }
+
+    @Override
+    public void debug(String format, Object argA, Object argB) {
+        logger.debug(format, argA, argB);
+    }
+
+    @Override
+    public void debug(String format, Object... arguments) {
+        logger.debug(format, arguments);
+    }
+
+    @Override
+    public void debug(String msg, Throwable t) {
+        logger.debug(msg, t);
+    }
+
+    @Override
+    public boolean isInfoEnabled() {
+        return logger.isInfoEnabled();
+    }
+
+    @Override
+    public void info(String msg) {
+        logger.info(msg);
+    }
+
+    @Override
+    public void info(String format, Object arg) {
+        logger.info(format, arg);
+    }
+
+    @Override
+    public void info(String format, Object argA, Object argB) {
+        logger.info(format, argA, argB);
+    }
+
+    @Override
+    public void info(String format, Object... arguments) {
+        logger.info(format, arguments);
+    }
+
+    @Override
+    public void info(String msg, Throwable t) {
+        logger.info(msg, t);
+    }
+
+    @Override
+    public boolean isWarnEnabled() {
+        return logger.isWarnEnabled();
+    }
+
+    @Override
+    public void warn(String msg) {
+        logger.warn(msg);
+    }
+
+    @Override
+    public void warn(String format, Object arg) {
+        logger.warn(format, arg);
+    }
+
+    @Override
+    public void warn(String format, Object... arguments) {
+        logger.warn(format, arguments);
+    }
+
+    @Override
+    public void warn(String format, Object argA, Object argB) {
+        logger.warn(format, argA, argB);
+    }
+
+    @Override
+    public void warn(String msg, Throwable t) {
+        logger.warn(msg, t);
+    }
+
+    @Override
+    public boolean isErrorEnabled() {
+        return logger.isErrorEnabled();
+    }
+
+    @Override
+    public void error(String msg) {
+        logger.error(msg);
+    }
+
+    @Override
+    public void error(String format, Object arg) {
+        logger.error(format, arg);
+    }
+
+    @Override
+    public void error(String format, Object argA, Object argB) {
+        logger.error(format, argA, argB);
+    }
+
+    @Override
+    public void error(String format, Object... arguments) {
+        logger.error(format, arguments);
+    }
+
+    @Override
+    public void error(String msg, Throwable t) {
+        logger.error(msg, t);
+    }
+}
diff --git a/common/src/main/java/io/netty/util/internal/logging/Log4J2LoggerFactory.java b/common/src/main/java/io/netty/util/internal/logging/Log4J2LoggerFactory.java
new file mode 100644
index 00000000000..6dfda57af43
--- /dev/null
+++ b/common/src/main/java/io/netty/util/internal/logging/Log4J2LoggerFactory.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.util.internal.logging;
+
+import org.apache.logging.log4j.LogManager;
+
+public final class Log4J2LoggerFactory extends InternalLoggerFactory {
+
+    @Override
+    public InternalLogger newInstance(String name) {
+        return new Log4J2Logger(LogManager.getLogger(name));
+    }
+}
diff --git a/pom.xml b/pom.xml
index c97a2da3379..b7e2c945520 100644
--- a/pom.xml
+++ b/pom.xml
@@ -382,6 +382,11 @@
         <artifactId>commons-logging</artifactId>
         <version>1.1.3</version>
       </dependency>
+      <dependency>
+        <groupId>org.apache.logging.log4j</groupId>
+        <artifactId>log4j-api</artifactId>
+        <version>2.3</version>
+      </dependency>
       <dependency>
         <groupId>log4j</groupId>
         <artifactId>log4j</artifactId>
@@ -522,6 +527,14 @@
         <version>1.5.7</version>
         <scope>test</scope>
       </dependency>
+
+      <!-- Test dependency for log4j2 tests -->
+      <dependency>
+        <groupId>org.apache.logging.log4j</groupId>
+        <artifactId>log4j-core</artifactId>
+        <version>2.3</version>
+        <scope>test</scope>
+      </dependency>
     </dependencies>
   </dependencyManagement>
 


Test patch:
diff --git a/common/src/test/java/io/netty/util/internal/logging/Log4J2LoggerFactoryTest.java b/common/src/test/java/io/netty/util/internal/logging/Log4J2LoggerFactoryTest.java
new file mode 100644
index 00000000000..e0db431e9d7
--- /dev/null
+++ b/common/src/test/java/io/netty/util/internal/logging/Log4J2LoggerFactoryTest.java
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.util.internal.logging;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class Log4J2LoggerFactoryTest {
+
+    @Test
+    public void testCreation() {
+        InternalLogger logger = new Log4J2LoggerFactory().newInstance("foo");
+        assertTrue(logger instanceof Log4J2Logger);
+        assertEquals("foo", logger.name());
+    }
+}
diff --git a/common/src/test/java/io/netty/util/internal/logging/Log4J2LoggerTest.java b/common/src/test/java/io/netty/util/internal/logging/Log4J2LoggerTest.java
new file mode 100644
index 00000000000..96ee4346c8f
--- /dev/null
+++ b/common/src/test/java/io/netty/util/internal/logging/Log4J2LoggerTest.java
@@ -0,0 +1,224 @@
+/*
+ * Copyright 2016 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.util.internal.logging;
+
+import org.junit.Test;
+import org.apache.logging.log4j.Logger;
+
+import static org.easymock.EasyMock.createStrictMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+import static org.junit.Assert.assertTrue;
+
+public class Log4J2LoggerTest {
+    private static final Exception e = new Exception();
+
+    @Test
+    public void testIsTraceEnabled() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        expect(mock.isTraceEnabled()).andReturn(true);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        assertTrue(logger.isTraceEnabled());
+        verify(mock);
+    }
+
+    @Test
+    public void testIsDebugEnabled() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        expect(mock.isDebugEnabled()).andReturn(true);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        assertTrue(logger.isDebugEnabled());
+        verify(mock);
+    }
+
+    @Test
+    public void testIsInfoEnabled() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        expect(mock.isInfoEnabled()).andReturn(true);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        assertTrue(logger.isInfoEnabled());
+        verify(mock);
+    }
+
+    @Test
+    public void testIsWarnEnabled() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        expect(mock.isWarnEnabled()).andReturn(true);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        assertTrue(logger.isWarnEnabled());
+        verify(mock);
+    }
+
+    @Test
+    public void testIsErrorEnabled() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        expect(mock.isErrorEnabled()).andReturn(true);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        assertTrue(logger.isErrorEnabled());
+        verify(mock);
+    }
+
+    @Test
+    public void testTrace() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.trace("a");
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.trace("a");
+        verify(mock);
+    }
+
+    @Test
+    public void testTraceWithException() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.trace("a", e);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.trace("a", e);
+        verify(mock);
+    }
+
+    @Test
+    public void testDebug() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.debug("a");
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.debug("a");
+        verify(mock);
+    }
+
+    @Test
+    public void testDebugWithException() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.debug("a", e);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.debug("a", e);
+        verify(mock);
+    }
+
+    @Test
+    public void testInfo() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.info("a");
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.info("a");
+        verify(mock);
+    }
+
+    @Test
+    public void testInfoWithException() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.info("a", e);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.info("a", e);
+        verify(mock);
+    }
+
+    @Test
+    public void testWarn() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.warn("a");
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.warn("a");
+        verify(mock);
+    }
+
+    @Test
+    public void testWarnWithException() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.warn("a", e);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.warn("a", e);
+        verify(mock);
+    }
+
+    @Test
+    public void testError() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.error("a");
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.error("a");
+        verify(mock);
+    }
+
+    @Test
+    public void testErrorWithException() {
+        Logger mock = createStrictMock(Logger.class);
+
+        expect(mock.getName()).andReturn("foo");
+        mock.error("a", e);
+        replay(mock);
+
+        InternalLogger logger = new Log4J2Logger(mock);
+        logger.error("a", e);
+        verify(mock);
+    }
+}