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

[Joern Huxhorn]

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

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

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 way, only the following 3 methods would need implementation:
>> Threshold getThreshold()
>> boolean isEnabled(Level level, Marker marker)
>> void log(Level level, Marker marker, Message message, Throwable  
>> throwable)
>
> That isn't really true. Again, look at the Logger class. While the  
> public methods like info(), debug() etc, could be factored out there  
> are a lot of other methods that can't be in an abstract class.

Those would stay in the actual Logger implementation, unchanged, as  
before.

>
>>
>> The log-method above would just call filterAndLog(String localFQCN,  
>> Marker marker, Level level, Message msg, Throwable t).
>>
>> The implementation of old and new SLF4J-API implementations must be  
>> separated so it's possible to either call org.slf4j.n.LoggerFactory  
>> (for org.slf4j.n.Logger)  or org.slf4j.LoggerFactory (for  
>> org.slf4j.Logger) while deserializing a Logger, i.e. the Logback  
>> Logger implementation should *not* implement org.slf4j.Logger  
>> anymore.
>
> There are some issues with this. LoggerFactory returns a Logger. To  
> maintain compatibility this interface cannot be changed. Instead I  
> created MessageLoggerFactory which returns a MessageLogger which  
> extends LocationAwareLogger. Logback's Logger now implements  
> MessageLogger instead of LocationAwareLogger. Other applications,  
> such as Sling, which implement the Logger interface will not be  
> impacted by this at all. I don't see how using  
> org.slf4j.message.MessageLoggerFactory is much different than using  
> org.slf4j.new.LoggerFactory, except I believe my name is a bit  
> clearer.
>

The original Logger interface is unchanged and would stay that way. No  
other implementation would be affected at all since they would be  
automatically wrapped the other way around, i.e. there's a  
n.ILoggerFactory implementation already implemented that is used  
automatically if no explicit implementation is available.
It wraps an original Logger into the n.Logger interface.

Both scenarios are tested in
http://github.com/huxi/slf4j/tree/slf4j-redesign/slf4j-n-api-new-integration-test/
and
http://github.com/huxi/slf4j/tree/slf4j-redesign/slf4j-n-api-old-integration-test/

> Logback doesn't implement Logger directly in either Ceki's fork or  
> mine. Currently, it implements LocationAwareLogger. In mine,  
> MessageLogger.

There's an additional (optional) interface for that, too:
http://github.com/huxi/slf4j/blob/slf4j-redesign/slf4j-n-api/src/main/java/org/slf4j/n/spi/LocationAwareLogger.java

>
>>
>> Instead, org.slf4j.impl.StaticLoggerBinder should return  
>> org.slf4j.n.helpers.UsingNewLoggerFactory which is implemented like  
>> this:
>> Logger getLogger(String name) {
>>   return org.slf4j.n.LoggerFactory.getLogger(name).getOldLogger();
>> }
>
> Why? I left the existing getLogger alone. Instead the MessageLogger  
> is a LoggerWrapper, so the existing API is untouched.

Because Logger and n.Logger are compiletime- but not binary-compatible.

Assuming that more and more modules would switch to the n API it means  
that more and more Loggers are used without an additional wrapper.

The current LoggerFactory-related code would be moved to  
org.slf4j.n.impl.StaticLoggerBinder and an org.slf4j.n.ILoggerFactory  
implementation with very minimal changes.

>
>>
>> Does this help as an explanation so far?
>
> Not really.  I'm working under the premise that a) SLF4J is not  
> being upgraded to Java 5

Yes, only an additional, optional module would be added. The original  
SLF4J API would not require Java 5.

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

All modules using the original API could still do so while other that  
use JDK>=1.5 could opt-in using the "modern" API by simply changing  
the import.

I've put a lot of thought into that whole concept.

One benefit of this approach is that it does not require the user to  
learn anything new. If he's not interested in the new API he'll be  
perfectly fine ignoring it entirely.
If, on the other hand, he has the need and/or motivation for the new  
Message/Varargs logging then a very simple upgrade path is in existence.
It can't get any simpler than just adding a .n to the imports (given  
the constraint that the original API must be left unchanged).

The .n is just a suggestion, though.
I really don't care how that package should be named or if everything  
is put into a new namespace like org.lf4j.

What I tried to achieve is that the class-names don't have to be  
changed, just the import/package-name.

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

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.

Cheers,
Joern.

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