New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Log4J2Logger-FQCN in PatternLayout (%F:%L)%c.%M #7185

Closed
wants to merge 3 commits into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@takeseem
Contributor

takeseem commented Sep 6, 2017

Motivation:

when I use Log4j2Logger in netty with PatternLayout (%F:%L)%c.%M,
the log message incorrect output:
(Log4J2Logger.java:73)io.netty.util.internal.PlatformDependent0.debug

Modification:

see log4j2 AbstractLogger class javadoc:

Base implementation of a Logger. It is highly recommended that any Logger implementation extend this class.

And AbstractLogger#FQCN private static final String FQCN = AbstractLogger.class.getName()
so if log the right FQCN when logging, your Logger must be extend from AbstractLogger.

Result:

fix:

final class Log4J2Logger extends AbstractLogger implements InternalLogger {

    private static final long serialVersionUID = 5485418394879791397L;

    /** {@linkplain io.netty.util.internal.logging.AbstractInternalLogger#EXCEPTION_MESSAGE} */
	private static final String EXCEPTION_MESSAGE = "Unexpected exception:";
	
    private final transient AbstractLogger logger;

    Log4J2Logger(Logger logger) {
    	super(logger.getName(), logger.getMessageFactory());
    	/**
    	 * see {@linkplain AbstractLogger}: Base implementation of a Logger. It is highly recommended that any Logger implementation extend this class.
    	 * And {@linkplain AbstractLogger#FQCN} <code>private static final String FQCN = AbstractLogger.class.getName()</code>.
    	 * so if log the right FQCN when logging, your Logger must be extend from AbstractLogger.
    	 * The code won't throw ClassCastException in here.
    	 */
    	this.logger = (AbstractLogger) logger;
    }

now it can out right PatternLayout (%F:%L)%c.%M

  • before fix
2017-09-07 01:46:24,109 DEBUG [main] -Dio.netty.noUnsafe: false (Log4J2Logger.java:73)io.netty.util.internal.PlatformDependent0.debug
2017-09-07 01:46:24,110 DEBUG [main] Java version: 8 (Log4J2Logger.java:73)io.netty.util.internal.PlatformDependent0.debug
2017-09-07 01:46:24,112 DEBUG [main] sun.misc.Unsafe.theUnsafe: available (Log4J2Logger.java:68)io.netty.util.internal.PlatformDependent0.debug
2017-09-07 01:46:24,112 DEBUG [main] sun.misc.Unsafe.copyMemory: available (Log4J2Logger.java:68)io.netty.util.internal.PlatformDependent0.debug
  • after fix:
2017-09-07 02:20:43,500 DEBUG [main] -Dio.netty.noUnsafe: false (PlatformDependent0.java:361)io.netty.util.internal.PlatformDependent0.explicitNoUnsafe0
2017-09-07 02:20:43,500 DEBUG [main] Java version: 8 (PlatformDependent0.java:797)io.netty.util.internal.PlatformDependent0.javaVersion0
2017-09-07 02:20:43,502 DEBUG [main] sun.misc.Unsafe.theUnsafe: available (PlatformDependent0.java:109)io.netty.util.internal.PlatformDependent0.<clinit>
2017-09-07 02:20:43,502 DEBUG [main] sun.misc.Unsafe.copyMemory: available (PlatformDependent0.java:133)io.netty.util.internal.PlatformDependent0.<clinit>

Fixes #7186 .

If there is no issue then describe the changes introduced by this PR.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 6, 2017

Member

@takeseem thanks a lot! Can you please fix the formatting to match our code style and sign our ICLA:

https://netty.io/s/icla

Please let me know once done

Member

normanmaurer commented Sep 6, 2017

@takeseem thanks a lot! Can you please fix the formatting to match our code style and sign our ICLA:

https://netty.io/s/icla

Please let me know once done

@normanmaurer normanmaurer self-assigned this Sep 6, 2017

@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 6, 2017

Contributor

I did not find eclipse codestyle.
I did my best to set some styles: tab with 4 spaces, line limit to 120.

Contributor

takeseem commented Sep 6, 2017

I did not find eclipse codestyle.
I did my best to set some styles: tab with 4 spaces, line limit to 120.

@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 6, 2017

Contributor

If checkstyle.xml is in the netty project, we will be able to check self before pull.

Contributor

takeseem commented Sep 6, 2017

If checkstyle.xml is in the netty project, we will be able to check self before pull.

@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Sep 6, 2017

Contributor

@takeseem you can just run the Maven verify / compile goal locally, it will run the code style check.

Contributor

johnou commented Sep 6, 2017

@takeseem you can just run the Maven verify / compile goal locally, it will run the code style check.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 8, 2017

Contributor

@johnou Thanks. It's right netty/common$ mvn clean verify.

I direct run netty/common$ mvn clean checkstyle:check, the checkstyle-results.xml is use default checkstyle.xml, it's not use io/netty/checkstyle.xml rules.

The right's cmd is netty/common$ mvn -Dcheckstyle.config.location=io/netty/checkstyle.xml clean checkstyle:check

Contributor

takeseem commented Sep 8, 2017

@johnou Thanks. It's right netty/common$ mvn clean verify.

I direct run netty/common$ mvn clean checkstyle:check, the checkstyle-results.xml is use default checkstyle.xml, it's not use io/netty/checkstyle.xml rules.

The right's cmd is netty/common$ mvn -Dcheckstyle.config.location=io/netty/checkstyle.xml clean checkstyle:check

@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 8, 2017

Contributor

@normanmaurer all fixed.
I read the log4j2 sources, now class Log4J2Logger extends ExtendedLoggerWrapper implements InternalLogger.

ExtendedLoggerWrapper:

Wrapper class that exposes the protected AbstractLogger methods to support wrapped loggers.
It very fit our scene : )

Contributor

takeseem commented Sep 8, 2017

@normanmaurer all fixed.
I read the log4j2 sources, now class Log4J2Logger extends ExtendedLoggerWrapper implements InternalLogger.

ExtendedLoggerWrapper:

Wrapper class that exposes the protected AbstractLogger methods to support wrapped loggers.
It very fit our scene : )

@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 8, 2017

Contributor

waiting, I will change switch(level) code in Log4J2Logger, it's too ugly.

Contributor

takeseem commented Sep 8, 2017

waiting, I will change switch(level) code in Log4J2Logger, it's too ugly.

Fixes #7186 log in PatternLayout (%F:%L)%c.%M
incorrect output:
(Log4J2Logger.java:73)io.netty.util.internal.PlatformDependent0.debug
fixed:
(PlatformDependent0.java:361)io.netty.util.internal.PlatformDependent0.explicitNoUnsafe0
@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Sep 8, 2017

Contributor

@takeseem did you try running the commands from root instead of directly from the sub module?

Contributor

johnou commented Sep 8, 2017

@takeseem did you try running the commands from root instead of directly from the sub module?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 8, 2017

Member

@takeseem let me know once you are done and I will have a look

Member

normanmaurer commented Sep 8, 2017

@takeseem let me know once you are done and I will have a look

@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 9, 2017

Contributor

@normanmaurer all done.

Contributor

takeseem commented Sep 9, 2017

@normanmaurer all done.

@takeseem

This comment has been minimized.

Show comment
Hide comment
@takeseem

takeseem Sep 9, 2017

Contributor

@johnou If run from root: netty$ mvn clean checkstyle:check

[INFO] Netty/Common ....................................... FAILURE [ 1.776 s]
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.12.1:check (default-cli) on project netty-common: You have 7016 Checkstyle violations. -> [Help 1]

It must set checkstyle.config.location: netty$ mvn -Dcheckstyle.config.location=io/netty/checkstyle.xml clean checkstyle:check

[INFO] Netty/Common ....................................... SUCCESS [ 1.545 s]

I suggest add how to run CheckStyle into the guider doc.

Contributor

takeseem commented Sep 9, 2017

@johnou If run from root: netty$ mvn clean checkstyle:check

[INFO] Netty/Common ....................................... FAILURE [ 1.776 s]
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.12.1:check (default-cli) on project netty-common: You have 7016 Checkstyle violations. -> [Help 1]

It must set checkstyle.config.location: netty$ mvn -Dcheckstyle.config.location=io/netty/checkstyle.xml clean checkstyle:check

[INFO] Netty/Common ....................................... SUCCESS [ 1.545 s]

I suggest add how to run CheckStyle into the guider doc.

@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Sep 9, 2017

Contributor

I'm afk so unable to verify myself but running "check" should work from root as you can see from the configuration..

<plugin>
        <artifactId>maven-checkstyle-plugin</artifactId>
        <version>2.12.1</version>
        <executions>
          <execution>
            <id>check-style</id>
            <goals>
              <goal>check</goal>
            </goals>
            <phase>validate</phase>
            <configuration>
              <consoleOutput>true</consoleOutput>
              <logViolationsToConsole>true</logViolationsToConsole>
              <failsOnError>true</failsOnError>
              <failOnViolation>true</failOnViolation>
              <configLocation>io/netty/checkstyle.xml</configLocation>
              <includeTestSourceDirectory>true</includeTestSourceDirectory>
            </configuration>
          </execution>
        </executions>
Contributor

johnou commented Sep 9, 2017

I'm afk so unable to verify myself but running "check" should work from root as you can see from the configuration..

<plugin>
        <artifactId>maven-checkstyle-plugin</artifactId>
        <version>2.12.1</version>
        <executions>
          <execution>
            <id>check-style</id>
            <goals>
              <goal>check</goal>
            </goals>
            <phase>validate</phase>
            <configuration>
              <consoleOutput>true</consoleOutput>
              <logViolationsToConsole>true</logViolationsToConsole>
              <failsOnError>true</failsOnError>
              <failOnViolation>true</failOnViolation>
              <configLocation>io/netty/checkstyle.xml</configLocation>
              <includeTestSourceDirectory>true</includeTestSourceDirectory>
            </configuration>
          </execution>
        </executions>
@takeseem

Thank your.
I got something, from reviews.
I like your rigorous style.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 14, 2017

Member

Cherry-picked into 4.1 (1418914) and 4.0 (5dcd375) with a few small adjustments

Member

normanmaurer commented Sep 14, 2017

Cherry-picked into 4.1 (1418914) and 4.0 (5dcd375) with a few small adjustments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment