[slf4j-dev] Missing Trowables in EventRecordingLogger

[Wessel van Norel]

Hi,

I was recently doing a code review for a coworker and saw that he was
concatenating strings and not using the varargs method of building the log
message. I asked him why he was doing that and he explained it was because
he also had a Throwable that he wanted to be passed as argument to see the
stacktrace. He was not aware of the change in SLF4J 1.6.0 which allows us
to pass a Throwable as last argument to the varargs.


"Unfortunately", when I was showing him implementations of this, I
encountered the slf4j EventRecordingLogger and this implementation of the
slf4j logger does not comply with this "new" behaviour. However I'm not
sure if this is intentional.

Assuming it's not intentional, I've created a fork and I've made the
required changes. But before I spend "days" on this while it's intentional
I thought: let's write a quick email about this and ask for guidance.

I also saw that in the case of 2 Object arguments, the MessageFormatter
seems to be suboptimal and is creating more objects then strictly required.

For the 1 Object argument case it it's also doing too much, certainly in
case someone calls it with a Throwable. I've now added a runtime check to
it. Since overloading it with a Throwable version will change the way the
method behaves in case the caller does not know if it's being given a
Throwable or an Object. It would be very illogical to call this method with
a Throwable, with the 1.6 way of dealing with Throwables, since what do you
want to format then?

But with the runtime check people won't loose their stacktraces, and
otherwise in case the MessageFormatter is being used in a wrong way people
will see their stacktraces being converted into one line because of a
(minor? it's one instanceof call too many per call to the one Object logger
variant) performance optimization.


I've not yet created the test cases for the EventRecordingLogger, I'll do
that in case it's indeed unintentional that it's loosing the Throwables.
I've added two tests for the changes I made to the MessageFormatter. While
I was at it I also applied some improvements IntelliJ suggested.

You can see my changes on my fork:

https://github.com/qos-ch/slf4j/compare/master...delgurth:eventrecordinglogger?expand=1

I hope you are able to review my changes and shed your light on it.

Kind regards,
Wessel van Norel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.qos.ch/pipermail/slf4j-dev/attachments/20190809/93980064/attachment.html>