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
_______________________________________________
slf4j-dev mailing list
slf4j-dev@qos.ch
http://mailman.qos.ch/mailman/listinfo/slf4j-dev
