[slf4j-dev] Missing Trowables in EventRecordingLogger

[Ceki Gülcü]

Hi Wessel,

Comments below.

On 09/08/2019 01:09, Wessel van Norel wrote:
> 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.

It's not 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.

Good idea.

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

Right.

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

Very good. In general, it is preferable if the more meaningful changes 
are separate from more cosmetic ones even if both are welcome.

> You can see my changes on my fork:
> 
> https://github.com/qos-ch/slf4j/compare/master...delgurth:eventrecordinglogger?expand=1

I have made some comments there.

In any case, I think the unit test as well the change to the 
org.slf4j.event.EventRecodingLogger.recordEvent method are warranted. I 
propose to discuss changes to MessageFormatter class separately.

Also can you please create a Jira ticket describing the bug in
EventRecodingLogger.recordEvent?

Many thanks,

--
Ceki