[logback-dev] RFC 5424 and Structured Data support.

[Joern Huxhorn]

On 29.01.2010, at 04:43, Ralph Goers wrote:

>
> On Jan 28, 2010, at 3:06 AM, Joern Huxhorn wrote:
>
>> Hi Ralph,
>>
>> On 28.01.2010, at 08:42, Ralph Goers wrote:
>>
>>> In reading your response I'm not sure if you are just describing  
>>> how you originally thought this should be done or critiquing what  
>>> I've done. In your analysis below I really don't see any comments  
>>> about what is wrong but rather, how you had anticipated it would  
>>> be done. I've tried to answer the points in your email as best I  
>>> can, but the main issue is that it seemed to me that your proposal  
>>> was for a new version of SLF4J whereas I am trying to make the  
>>> changes without the need for a major release or breaking any  
>>> compatibility.
>>
>> I assumed you tried to do the Logback counterpart of my SLF4J  
>> redesign proposal - which wouldn't break compatibility but  
>> introduces an additional artifact (tentatively called slf4j-n-api).
>> Instead of depending on slf4j-api, Logback classic would instead  
>> depend on slf4j-n-api (which, in turn, introduces the slf4j-api  
>> dependency for compatibility reasons).
>
> I tried to implement your proposal to some degree. The most  
> important part to me was introducing the Message concept. For  
> minimal impact on users I did not want to introduce or change any  
> dependencies, other than requiring version changes.
>
>>
>>>
>>> On Jan 25, 2010, at 3:40 AM, Joern Huxhorn wrote:
>>>
>>>> Ok, I've had time to take a look now.
>>>>
>>>> My plan was that the logback-classic Logger would simply extend  
>>>> org.slf4j.n.helper.AbstractLoggerBase.
>>>
>>> I saw that in your original note. That class doesn't currently  
>>> exist. I assume you would have created it. As I recall I thought  
>>> about that but decided against it. Logback has a lot of logic in  
>>> its Logger class and implementing AbstractLoggerBase in SLF4J  
>>> doesn't save much as you might think.  In addition, all the  
>>> current Loggers implement the interface. Creating a new base class  
>>> that includes the Message API and then having Logback extend that  
>>> creates serious problems
>>
>> But it does exist:
>> http://github.com/huxi/slf4j/blob/slf4j-redesign/slf4j-n-api/src/main/java/org/slf4j/n/helpers/AbstractLoggerBase.java
>>
>> Is it possible that you did not look at the slf4j-redesign branch  
>> but master?
>
> Sorry. It has been quite a while since I wrote the code. Yes, I  
> looked at your code. IIRC the reason I didn't use it was that I  
> don't like calling the isEnabled methods that way. The log method is  
> going to result it Logback calling filterAndLog which results in  
> filtering being performed twice for each call. If the log method  
> doesn't call filterAndLog then the behavior of the log method would  
> be different than how Logback currently behaves. Of course, not  
> calling isEnabled would result in creating ParameterizedMessages  
> when no logging will occur. My implementation doesn't have this  
> problem.

Good point.
I've changed AbstractLoggerBase a bit.
It now directly implements log(level, marker, message, throwable) and  
defines
protected abstract void performLogging(level, marker, message,  
throwable)
An implementation is expected to not call isEnabled before performing  
additional filtering and logging.

What I don't know right now:
Is it possible to reenable logging that would be disabled due to level  
by using turbo-filters?
This wouldn't work anymore using my AbstractLoggerBase.
(which doesn't mean that the rest of the concept doesn't work)

Calling isEnabled was indeed meant to prevent creation of messages  
that won't be logged.

This could probably be prevented by another method like
protected abstract boolean isEnabled(level, marker, messagePattern,  
arguments)
that implemented the filtering logic.

I've never used TurboFilters so I tend to forget about their powers.

One problem of your implementation that I've found in the meantime is  
that you are not using the Throwable that might have been detected by  
ParameterizedMessage.
The logic looks like this:
   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);
     }
   }

This is necessary to use both varargs and Throwables at the same time.

Assuming that TurboFilters can act on the optional Throwables, the  
Message needs to be created before filtering.
Assuming the filters can NOT override a level, checking for level and  
marker using isEnabled(level, marker) makes sense before filtering to  
prevent the Message creation.

  I really don't know.

>
>>
>> The Logback classic Logger isn't extending anything at the moment  
>> and the AbstractLoggerBase is simply implementing everything that  
>> will likely stay the same for all Logger implementations.
>> It also supports proper deserialization of Logger instances already.
>>
>> It's not necessary to extend it but it's certainly easier to do so  
>> and doing so wouldn't introduce any problems in case of the Logback  
>> classic Logger. It would simply remove a lot of boilerplate code  
>> from the Logger implementation.
>
> That would be true if my comment above wasn't true.
>
>>
>>> b) no new SLF4J version is required because c) backward  
>>> compatibility must be maintained.
>>
>> That's the whole point. By leaving the original API untouched and  
>> adding an additional one we'd have the possibility to change the  
>> API without constraints.
>> My constraining goal was compiletime compatibility, but not binary  
>> compatibility.
>>
>> Binary compatibility simply can't be achieved without sacrificing  
>> JDK<1.5 compatibility in SLF4J API - and I think that would be a  
>> bad idea.
>> So I aimed for the next-best thing: compiletime compatibility  
>> requiring only a change of the Logger and LoggerFactory import.
>
> ??? My code should be maintaining binary compatibility. Applications  
> written to older versions of SLF4J can drop in the new jars with no  
> change. New code can use the new API. They can coexist happily.

With my concept it stays binary compatible for the old API, i.e.  
org.slf4j.Logger and org.slf4j.LoggerFactory.
Any code built against those would simply work but couldn't use the  
Message logging and varargs since the original SLF4J API is frozen and  
JDK14.

If you want Varargs (incl. Throwable evaluation) & Message you'd use  
org.slf4j.n.Logger and org.slf4j.n.LoggerFactory.

The n-API is Java 5, the original one isn't.

>>
>>
>>> If I was not working with those constraints I certainly would have  
>>> made a few different choices, but then I'm sure I would have had a  
>>> snowballs chance in hell of getting it adopted since it would  
>>> break so much stuff.
>>
>> It wouldn't break anything. I tested it.
>
> I won't believe that until I see a fork of Logback that depends on  
> your SLF4J implementation. It was very difficult to get Logback to  
> accept the Message and deal with it correctly.

Well, you did the hardest part of the work already, i.e. the Message  
handling throughout the rest of the API.

I'd honestly like to have some input from Ceki because I don't want to  
invest too much time if all of this ends in a "nope".

>>
>> If you can find any breakage please let me know but I'm quite  
>> confident that I didn't miss any point this time around.
>>
>> For example:
>> during the evolution of SLF4J bug #31 I suggested that n.Logger  
>> should extend Logger.
>> This would have resulted in breakage since it wouldn't have been  
>> clear, during deserialization, what is expected by the  
>> deserializing code. readResolve() needs to execute  
>> LoggerFactory.getLogger(loggerName). How should we decide about  
>> wrapping or not without creating kludgy code? Wrapping it each and  
>> every time if n.Logger wasn't implemented by the returned instance?  
>> Nope, this would have been quite bad.
>
> If you notice there really is no extra overhead since in my fork  
> Logback's getLogger() always returns a MessageLogger. It isn't  
> really wrapped. No kludgy code is needed. Since MessageLogger  
> extends Logger anyone calling LoggerFactory.getLogger() will work  
> just fine.  The wrapper is only needed if a Logger implementation  
> returns a Logger. Then MessageLoggerFactory will wrap it.

What MessageLoggerFactory? Now I'm lost. ;)

Joern.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://qos.ch/pipermail/logback-dev/attachments/20100129/41729990/attachment.html>