[slf4j-dev] [Bug 31] Varargs for Logger methods

[bugzilla-daemon at pixie.qos.ch]

http://bugzilla.slf4j.org/show_bug.cgi?id=31

--- Comment #90 from Joern Huxhorn <joern at huxhorn.de> 2011-09-07 11:32:28 CEST ---
(In reply to comment #89)
> A new proposal (and code):
> 
> 1. Release 2.0 targeting Java 5

Agreed, but it must be possible to use 1.6.x and 2.0 side-by-side.

> Those stuck on 1.4 aren't looking for the latest and greatest features.
> SLF4J 1.6.2 should be sufficient.
> 
> 
> 2. Add varargs with Google Collections/Guava style methods
> 
> log*(String format, String arg1, ..., String arg6)
> log*(String format, String arg1, ..., String arg6, String... others)
> 
> There is a performance penalty for creating the implicit array required for
> varargs.  A quick benchmark shows this to be about 1 nanosecond.  Small, but
> why not eliminate it for nearly all use cases (6 or fewer args). As a bonus
> with this approach, the existing one and two arg methods will fit in naturally
> with this style API.  The log*(format, Object[] arg) methods should be retained
> for compatibility.
> 
> This will require many new methods, but it is not as bad as it sounds.  See
> below.

I can't help but feel dirty defining an interface like this...

Even taking into account the PDF linked in comment #82, I think "optimizing" an
interface like this is pretty much premature optimization. "Premature" since
the next JVM (which is constantly enhanced and optimized) could suddenly
implement optimizations that could result in nearly identical or even
same/better performance.

You can never get rid of those additional methods, ever, without having this
very same discussion and having another compatibility break.

I'm aware that this is a rather purist point of view but I also think that I
have some very valid points...

> 4. Simplify the logger adapter contract
> 
> It seems that compatibility challenges may be largely due to the front end API
> (used by application writers) is tightly bound to the adapter API (used by
> logging platform developers).
> 
> The minimum set of methods required for an adapter is two (three if
> LocationAwareLogger is implemented):
> 
> protected abstract boolean isEnabled(Marker marker, Level level);
> protected abstract void log(Marker marker, Level level, String msg, Throwable
> t);

All maybeLog(...)-methods in NamedLoggerBase are calling the same
private void log(Marker marker, Level level, String msg, Throwable t)
method, already formatting the message and dropping any info about the argument
array and message pattern that was used to call it.

This is rather bad for several reasons:
- not every logging framework/appender requires the rather expensive formatting
to take place at this point or at all. Both my Logback SocketAppender and
Encoder are using the raw message pattern and the arguments transformed into
String instead of the formatted message. This reduces the amount of time
required during the actual logging and moves the formatting into the code that
is used for logging event consumption, resulting in increased performance of
the logged application.
- We filter on the unformatted message pattern regularly since it is much more
straightforward than using the formatted message and creating a potentially
very complex RegEx to catch (or rather: ignore) a specific logging call.
- since losing those abilities is not an option, Logback can't use
NamedLoggerBase and would therefore be required to reimplement all new methods
on it's own. It also wouldn't be able to extend AbstractLogger for the same
reason.

> An AbstractLogger class would implement all application facing API methods.
> Recommending (or requiring) log adapters to extend AbstractLogger and implement
> the two methods above would effectively unbind the front end and adapter APIs.
> New Logger methods could be added.  Formatting would no longer be handled by
> adapters.  Adapter lines of code and risk of bugs would be greatly reduced.

The fact that (optional) formatting is happening in the adapters is not an
issue but a feature.
Message formatting is an implementation detail and different frameworks can
handle it in different ways (e.g. postpone it as in my Logback example).

> Loggers extending AbstractLogger would of course require the 2.0 API at
> runtime, with the long term benefit of simplified code and future
> compatibility.

I see what you are trying to achieve but it won't work, at least not in the way
you've implemented it right now.

Please check out comment #61, comment #69, comment #78 and comment #86

I'd really like to see the Logger extended with methods that can handle
arbitrary Message implementations in addition to the current
messagePattern+arguments style.
Something like this has been frequently requested, i.e. being able to log with
arbitrary attached objects that can still be accessed in the (Logback)
appender.

The way to achieve this at the moment is serializing them to a String in some
way, putting them into the MDC with a name known by the appender and
deserializing them in the appender by parsing in that String retrieved from the
MDC. This, obviously, is less than ideal concerning performance.

Cheers,
Joern.

-- 
Configure bugmail: http://bugzilla.slf4j.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.