logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Remko Popma (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (LOG4J2-555) Location-based functionality broken in AbstractLoggerWrapper subclasses
Date Fri, 07 Mar 2014 01:10:42 GMT

    [ https://issues.apache.org/jira/browse/LOG4J2-555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13923194#comment-13923194
] 

Remko Popma edited comment on LOG4J2-555 at 3/7/14 1:09 AM:
------------------------------------------------------------

I won't have time to work out the details for another 2 weeks or so, so let me draw the big
picture of the refactoring I have in mind.

The goal of the refactoring is to reduce the lines of code in the convenience methods (what
is currently almost, but not quite duplicate code). These methods would end up delegating
to a single {{doLog()}} method (in both AbstractLogger and Extended/Custom Loggers, not sure
about the Writers). For example, the current code in AbstractLogger looks like this:
{code}
public void error(final String message, final Object... params) {
    if (isEnabled(Level.ERROR, null, message, params)) {
        final Message msg = messageFactory.newMessage(message, params);
        log(null, FQCN, Level.ERROR, msg, msg.getThrowable());
    }
}
{code}
the above would become
{code}
public void error(final String message, final Object... params) {
    doLog(Enabled.stringVarargs, null, FQCN, Level.ERROR, MsgBuilder.stringVarargs,
            message, null, params);
}
{code}

The idea is to create enums for all {{isEnabled()}} methods, and passing this enum value to
the {{doLog}} method instead of calling the {{isEnabled}} method directly in {{error()}}.

The new {{doLog}} method would look something like this:
{code}
private void doLog(Enabled enabled, Marker marker, String FQCN, Level level, MsgBuilder builder,
        String message, Throwable throwable, Object... params) {
    Message msg = builder.build(message, throwable, params);
    if (enabled.isEnabled(this, level, marker, msg, throwable)) {
        log(marker, FQCN, level, msg, msg.getThrowable());
    }
}
{code}

Each of the 5 enum values for {{Enabled}} has a different implementation of the {{isEnabled}}
method that ignores some parameters, so we end up with the same behaviour as what AbstractLogger
currently does, with a few extra method invocations in between (that Hotspot can easily optimize
out).

A similar enum mechanism can be used to create the {{Message}} object (tentatively named {{MsgBuilder}}).

So the trade-off is we gain a simpler implementation for the existing convenience methods
(6 levels times 14 methods = 84 methods where we go from 3-4 lines to 1 line).

And in return we need to add the new {{doLog}} method above as well as enums for Enabled and
MsgBuilder. The enums can be public static inner classes of AbstractLogger so they can be
re-used in Extended/Custom loggers.

This also means there would be no need to track FQCNs in the class hierarchy. I'm kind of
moving away from any kind of central FQCN tracking. It is much simpler if each AbstractLogger
subclass manages its own FQCN.

Thoughts?


was (Author: remkop@yahoo.com):
I won't have time to work out the details for another 2 weeks or so, so let me draw the big
picture of the refactoring I have in mind.
The goal of the refactoring is to end up with a single {{doLog()}} method that both AbstractLogger
and Extended/Custom Loggers can call.
So for example in AbstractLogger
{code}
public void error(final String message, final Object... params) {
    if (isEnabled(Level.ERROR, null, message, params)) {
        final Message msg = messageFactory.newMessage(message, params);
        log(null, FQCN, Level.ERROR, msg, msg.getThrowable());
    }
}
{code}
the above would become
{code}
public void error(final String message, final Object... params) {
    doLog(Enabled.stringVarargs, null, FQCN, Level.ERROR, MsgBuilder.stringVarargs,
            message, null, params);
}
{code}

The idea is to create enums for all {{isEnabled()}} methods, and passing this enum value to
the {{doLog}} method instead of calling the {{isEnabled}} method directly in {{error()}}.
The new {{doLog}} method would look like this:
{code}
private void doLog(Enabled enabled, Marker marker, String FQCN, Level level, MsgBuilder builder,
        String message, Throwable throwable, Object... params) {
    Message msg = builder.build(message, throwable, params);
    if (enabled.isEnabled(this, level, marker, msg, throwable)) {
        log(marker, FQCN, level, msg, msg.getThrowable());
    }
}
{code}

Each of the 5 enum values for {{Enabled}} has a different implementation of the {{isEnabled}}
method that ignores some parameters, so we end up with the same behaviour as what AbstractLogger
currently does, with a few extra method invocations inbetween (that Hotspot can easily optimize
out).
A similar enum mechanism can be used to create the {{Message}} object.

So the trade-off is we gain a simpler implementation for the existing convenience methods
(6 levels times 14 methods = 84 methods where we go from 3-4 lines to 1 line).

And in return we need to add the new {{doLog}} method above as well as enums for Enabled and
MsgBuilder.
The enums can be protected static so they can be re-used in Extended/Custom loggers.

This also means there would be no need for complex tracking of FQCNs in the class hierarchy.

Thoughts?

> 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
>
>
> *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: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org


Mime
View raw message