logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joern Huxhorn <jhuxh...@googlemail.com>
Subject Re: log4j2 parameterized msg with throwable
Date Sat, 17 Sep 2011 19:26:11 GMT

On 17.09.2011, at 21:02, John Vasileff wrote:

> 
> On Sep 17, 2011, at 2:40 PM, Joern Huxhorn wrote:
> 
>> 
>> On 17.09.2011, at 18:47, John Vasileff wrote:
>> 
>>> 
>>> On Sep 17, 2011, at 8:53 AM, Joern Huxhorn wrote:
>>> 
>>>> Adding getThrowable() would also reintroduce the issue that such a Message
could not be serialized securely since deserialization would require the exceptions class
on the classpath. In my ParameterizedMessage over at https://github.com/huxi/slf4j/blob/slf4j-redesign/slf4j-n-api/src/main/java/org/slf4j/n/messages/ParameterizedMessage.java
the Throwable is transient so it is deliberately lost while serializing, i.e. it must be taken
care of by the framework immediately after the message is created.
>>>> 
>>>> It is only contained in ParameterizedMessage at all simply because a method
can only return a single value. And the create-method is also responsible for resolving the
Throwable at the last position of arguments if it is not used up by a corresponding {}.
>>>> 
>>>> Yes, it would be "cleaner" to split that into a parseresult object that contains
both the message and an additional, optional Throwable. I just didn't implement it that way
to reduce the amount of garbage (that parseresult object would get garbage-collected immediately
after evaluation).
>>>> 
>>>> So adding Throwable to ParameterizedMessage was just a performance optimization.
>>>> 
>>>> The serialization issue is btw also the reason for *not* having Object[]
getParameters() but String[] getParameters() instead. The String representation is the serialization-safe
version of the parameters.
>>>> 
>>>> But all of this is, in my books, an implementation detail of the Message
implementation with getFormattedMessage() being the only "public" API of Message.
>>>> 
>>>> Joern.
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>>> 
>>> 
>>> Interesting point on serialization.  When would you see serialization happening?
 Is this primarily for appenders?
>>> 
>> 
>> SocketAppender is using serialization. Since I'm the author of Lilith ( http://lilith.huxhorn.de/
) I tend to focus on stuff like that.
>> 
>> This is also the reason for the differentiation between the Message instance and
the (laziliy) formatted  message string. A SocketAppender does not have any need for a formatted
message. It is perfectly valid to skip the formatting entirely and simply transmit the message
pattern and the message parameters (as Strings) to safe some CPU in the logged application.
>> 
>> Directly converting parameters into Strings as soon as we know that an event really
needs to be created is crucial to make sure that the logging framework is not lying as is
happening, for example, in this case: http://techblog.appnexus.com/2011/webkit-chrome-safari-console-log-bug/
>> A lying logging framework is really, really bad...
>> 
>> Joern.
> 
> So, the two most significant points are:
> 
> 1) ambiguity with info(Message msg, Throwable throwable) where for all practical purposes
Message will at least sometimes also have a throwable.  Minor additional point that having
log methods with explicit Throwables adds several methods to the Logger interface.
> 
> 2) issue with serializing appenders having to special case 'transient Throwable' in message
objects (i.e. the appender would have to call getThrowable(), convert to String, and store
separately from the message object.)
> 
> My opinion is that Logger is by far the most important interface and design decisions
should give preference to application developer facing interfaces.  Thoughts?


The code simply looks like this:
public void log(Level level, String messagePattern, Object... args) {
	if (!isEnabled(level)) return;
	ParameterizedMessage message = ParameterizedMessage.create(messagePattern, args);
	performLogging(level, null, message, message.getThrowable());
}

public void log(Level level, Marker marker, String messagePattern, Object... args) {
	if (!isEnabled(level, marker)) return;
	ParameterizedMessage message = ParameterizedMessage.create(messagePattern, args);
	performLogging(level, marker, message, message.getThrowable());
}

public void log(Level level, Message message) {
	if (!isEnabled(level)) return;
	if (message instanceof ParameterizedMessage) {
		performLogging(level, null, message, ((ParameterizedMessage) message).getThrowable());
	} else {
		performLogging(level, null, message, null);
	}
}

public void log(Level level, Message message, Throwable throwable) {
	if (!isEnabled(level)) return;
	performLogging(level, null, message, throwable);
}

public void log(Level level, Marker marker, Message message) {
	if (!isEnabled(level, marker)) return;
	if (message instanceof ParameterizedMessage) {
		performLogging(level, marker, message, ((ParameterizedMessage) message).getThrowable());
	} else {
		performLogging(level, marker, message, null);
	}
}

public void log(Level level, Marker marker, Message message, Throwable throwable) {
	if (!isEnabled(level, marker)) return;
	performLogging(level, marker, message, throwable);
}

If a method with just a Message but no Throwable is called then this Message is checked if
it is a ParameterizedMessage. Otherwise null is used for the Throwable. If, however, a method
with an explicit Throwable is used then this Throwable is used for logging.
The whole point of having the Throwable in ParameterizedMessage is simply because create can
only return a single value without creating additional overhead caused by wrappers. The Throwable
is not meant to be part of the Message, it just happens to be like that directly after creation
- but not after deserialization! It's the job of the logging framework to handle the Throwable
in a sensible way.

Perhaps making getThrowable() package-private would make this more clear but I'd have to move
classes around in a non-logical way just to achieve this. Do not want.

Joern.



---------------------------------------------------------------------
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