[
https://issues.apache.org/jira/browse/LOG4J2-555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13935921#comment-13935921
]
Bruce Brouwer commented on LOG4J2-555:
--------------------------------------
I wanted to give an overview of what my patch is trying to do:
# Simplify most logger methods to be one line of code
# Allow classes that wrap a logger (e.g. using AbstractLoggerWrapper) to use
one line of code to log. To accomplish this, I created all the variants of the
log(...) methods that now take a FQCN as the first parameter.
# Created an interface called LoggerProvider that AbstractLogger implements.
This I think makes it a bit clearer what extra methods are needed beyond what
is in the Logger interface to succesfully wrap a logger. This also allows for
different (future) logger implementations that don't need to extend
AbstractLogger.
# I changed LoggerContext to return LoggerProvider. Most people don't tend to
go after the LoggerContext, so most people won't see this. But by doing this
here, it makes it clear that loggers coming from here are guaranteed to have
this extra functionality, and it eliminates some casting done in the code
# I tried to fix some of the method parameter ordering to be consistent: fqcn,
level, marker, message, ... This patch doesn't cover all cases where this could
be done but in the cases were I was changing method signatures anyway, I
thought I would make the order consistent.
You may notice that this patch includes a change to log4j-jcl Log4jLog.java.
This was not absolutely necessary, but it seemed somehow wrong to me to rely on
the fact that the method signatures of jcl match up perfectly to log4j2. The
patch implementation also prevents leaking a bunch of other public methods into
the implementation coming from AbstractLogger and AbstractLoggerWrapper. If
others don't agree with me, it would be easy to revert Log4jLog.java back to
what it was before, but instead of the first constructor parameter being
AbstractLogger, it would change to LoggerProvider.
I also just noticed that log4j-to-slf4j SLF4JLogger.logMessage has an
unnecessary switch statement. That should probably be cleaned up as switching
it to simply call logger.log(FQCN, level, getMarker(marker), ...) would
actually be more performant, not only because it eliminates the switch
statement, but it would also go through fewer method invocations. Do you want
me to provide an updated patch that does this?
Finally, I ran PerfTestDriver, just the "Loggers all async" tests. I am not
sure what the best thing is to do to communicate performance, but this is what
I did. I was kind of surprised to see that this patch is actually consistently
more performant than the current log4j2 code (at least that's how I'm reading
it).
h5. Original:
1. Log4j2: Loggers all async (single thread): throughput: 12,746,429 ops/sec.
latency(ns): avg=980.2 99% < 8192.0 99.99% < 1101004.8 (494079 samples)
2. Log4j2: Loggers all async (4 threads): throughput: 6,603,641 ops/sec.
latency(ns): avg=3157.4 99% < 5120.0 99.99% < 8703180.8 (3376684 samples)
3. Log4j2: Loggers all async (2 threads): throughput: 3,505,632 ops/sec.
latency(ns): avg=1103.5 99% < 5324.8 99.99% < 2516582.4 (2339717 samples)
h5. After:
1. Log4j2: Loggers all async (single thread): throughput: 13,612,251 ops/sec.
latency(ns): avg=8463.0 99% < 8192.0 99.99% < 2936012.8 (344455 samples)
2. Log4j2: Loggers all async (4 threads): throughput: 7,395,284 ops/sec.
latency(ns): avg=2690.5 99% < 5939.2 99.99% < 8231321.6 (5952262 samples)
3. Log4j2: Loggers all async (2 threads): throughput: 4,639,382 ops/sec.
latency(ns): avg=1079.2 99% < 6144.0 99.99% < 2464153.6 (2312163 samples)
> Location-based functionality broken in AbstractLoggerWrapper subclasses
> -----------------------------------------------------------------------
>
> Key: LOG4J2-555
> URL: https://issues.apache.org/jira/browse/LOG4J2-555
> Project: Log4j 2
> Issue Type: Bug
> Components: API, Core
> Affects Versions: 2.0-rc1
> Reporter: Remko Popma
> Assignee: Remko Popma
> Fix For: 2.0-rc2
>
> Attachments: LOG4J2-555-delegate.patch, log4j2-555-bbrouwer-2.patch,
> log4j2-555-bbrouwer.patch
>
>
> *How to reproduce*
> * Create a custom logger that extends {{AbstractLoggerWrapper}} (or generate
> one with the tool attached to LOG4J2-519)
> * In the custom logger provide a public method that invokes the {{log(Level,
> String)}} method
> * Configure a pattern layout that uses location, like %C for the logger FQCN
> * From a sample app, call the public method on your custom logger.
> * The output will show the class name of the custom logger instead of the
> class name of the calling class in the sample application.
> *Cause*
> {{AbstractLogger}}'s FQCN field is {{static final}} and initialized to
> {{AbstractLogger.class.getName()}}. Then, in
> {{Log4jLogEvent#calcLocation()}}, when walking over the stack trace elements,
> the element _following_ the FQCN is returned. So only loggers that directly
> subclass from {{AbstractLogger}} will work correctly. Loggers that inherit
> from {{AbstractLoggerWrapper}} are two levels removed from {{AbstractLogger}}
> and the {{calcLocation()}} method will not work correctly.
> *Solution*
> I think {{AbstractLogger}}'s FQCN field should be made non-static, and
> initialized to {{getClass().getName()}} in the constructor of
> {{AbstractLogger}}. {{Log4jLogEvent#calcLocation()}} can then be modified to
> return the {{StackElement}} whose class name matches the FQCN, instead of the
> next element. Location-based functionality should then work for arbitrarily
> deep subclass hierarchies of AbstractLogger.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
