On Sep 17, 2011, at 3:26 PM, Joern Huxhorn wrote: > > 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-unsubscr...@logging.apache.org >>>>> For additional commands, e-mail: log4j-dev-h...@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); > }
Fixes would be required for all classes extending AbstractLogger that implement: protected abstract void log(Marker marker, String fqcn, Level level, Message data, Throwable t); and protected abstract boolean isEnabled(Level level, Marker marker, Message data, Throwable t); Alternately, the numerous: public void trace|debug|..(Marker? marker, Message msg, Throwable? t); of AbstractLogger could include this logic. Granted, redundant code could be avoided with "private void resolveThrowableAndLog(...)" in AbstractLogger that is called by the numerous methods. Third party implementations of Logger that do not extend AbstractLogger would need to do this as well. Appenders, Filters, etc. probably don't need this check if resolving throwables is handled by all front end methods. But, given that messages may have a throwable, the contract that non-front-end code isn't required to resolve throwables is not clearly communicated by the method signatures. Further... today ParameterizedMessages are allowed to define a throwable. Tomorrow there may be other types such as JavaFormattedMessage. So, the above code does not remove the requirement for a ThrowableMessage type. It should be "instance of ThrowableMessage". > > 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. > Despite implementation challenges above, the core of the problem I see is not with the getThrowable() method. It is with the dilemma faced by application developers deciding if providing a Throwable in a Message object is sufficient, or if it should also be provided as a log method argument, or maybe only as a log method argument. Including your above notes in the javadocs for info(msg, throwable) type methods would help resolve this. But considering a throwable as a part of the message data and removing what would then be "extra" methods eliminates the dilemma altogether. For a JavaFormattedMessage class that also handles a trailing throwable, all users would face the above dilemma, as would users that want to instantiate ParameterizedMessage directly. > Joern. > John --------------------------------------------------------------------- To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org
