Review request: LOG4J2-3393 JsonTemplateLayout performance improvements

[Volkan Yazıcı]

I will appreciate it if somebody can take a look at #797
. I plan to merge it
next Monday.

Re: Order of property sources

[Ralph Goers]

I would not be in favor of this change. Providers have been designed since day 
one that highest wins and it is documented - 
https://logging.apache.org/log4j/2.x/manual/extending.html#LoggerContextFactory 
.
 

The fact that the user was surprised may be true, but the ordering was 
intentional.  If you include log4j-to-slf4j it means you want to log to SLF4J. 
In that case log4j-core shouldn’t even be present.

Ralph

> On Mar 14, 2022, at 4:37 AM, Piotr P. Karwasz  wrote:
> 
> On Mon, Mar 14, 2022 at 10:54 AM Volkan Yazıcı  wrote:
>> Regarding your remark about providers... `Provider#getPriority()` has the
>> following javadoc: *"Gets the priority (natural ordering) of this Provider"*,
>> hence I would have expected it to work the same lowest-comes-first way, but
>> apparently not – relying on your assessment here. I am also inclined to
>> align them with the lowest-comes-first strategy, though this might have a
>> bigger impact if there are 3rd party providers out there in the wild. Maybe
>> others can weigh in here?
> 
> I can provide some additional evidence supporting the necessity to
> invert the order. At least one user on Stack Overflow was surprised
> that after adding `log4j-core` to his project, the loggers are still
> of type `org.apache.logging.slf4j.SLF4JLogger`:
> 
> https://stackoverflow.com/q/70487959/11748454
> 
> Piotr

Re: Order of property sources

[Tim Perry]

In that case, should log4j log a warning if more than one provider is included?

Tim


> On Mar 15, 2022, at 1:44 PM, Ralph Goers  wrote:
> 
> I would not be in favor of this change. Providers have been designed since 
> day one that highest wins and it is documented - 
> https://logging.apache.org/log4j/2.x/manual/extending.html#LoggerContextFactory
>  
> .
>  
> 
> The fact that the user was surprised may be true, but the ordering was 
> intentional.  If you include log4j-to-slf4j it means you want to log to 
> SLF4J. In that case log4j-core shouldn’t even be present.
> 
> Ralph
> 
>> On Mar 14, 2022, at 4:37 AM, Piotr P. Karwasz  
>> wrote:
>> 
>>> On Mon, Mar 14, 2022 at 10:54 AM Volkan Yazıcı  wrote:
>>> Regarding your remark about providers... `Provider#getPriority()` has the
>>> following javadoc: *"Gets the priority (natural ordering) of this 
>>> Provider"*,
>>> hence I would have expected it to work the same lowest-comes-first way, but
>>> apparently not – relying on your assessment here. I am also inclined to
>>> align them with the lowest-comes-first strategy, though this might have a
>>> bigger impact if there are 3rd party providers out there in the wild. Maybe
>>> others can weigh in here?
>> 
>> I can provide some additional evidence supporting the necessity to
>> invert the order. At least one user on Stack Overflow was surprised
>> that after adding `log4j-core` to his project, the loggers are still
>> of type `org.apache.logging.slf4j.SLF4JLogger`:
>> 
>> https://stackoverflow.com/q/70487959/11748454
>> 
>> Piotr
> 

Re: Order of property sources

[Ralph Goers]

That is actually an interesting question. If you look at the logic, all the 
providers are loaded. They are then looped through and the first one that 
returns a LoggerFactory is used. I really consider this code unfinished. When I 
wrote it I considered supporting multiple logging implementations but I 
couldn’t really figure out how that would work so I just stopped after loading 
the first one. But in theory we could have multiple logger factories and there 
could be an implementation that calls each of the logging implementations. 

But I have never thought of a rational reason for doing it so I’ve just left it 
the way it is.

Ralph

> On Mar 15, 2022, at 2:31 PM, Tim Perry  wrote:
> 
> In that case, should log4j log a warning if more than one provider is 
> included?
> 
> Tim
> 
> 
>> On Mar 15, 2022, at 1:44 PM, Ralph Goers  wrote:
>> 
>> I would not be in favor of this change. Providers have been designed since 
>> day one that highest wins and it is documented - 
>> https://logging.apache.org/log4j/2.x/manual/extending.html#LoggerContextFactory
>>  
>> .
>>  
>> 
>> The fact that the user was surprised may be true, but the ordering was 
>> intentional.  If you include log4j-to-slf4j it means you want to log to 
>> SLF4J. In that case log4j-core shouldn’t even be present.
>> 
>> Ralph
>> 
>>> On Mar 14, 2022, at 4:37 AM, Piotr P. Karwasz  
>>> wrote:
>>> 
 On Mon, Mar 14, 2022 at 10:54 AM Volkan Yazıcı  wrote:
 Regarding your remark about providers... `Provider#getPriority()` has the
 following javadoc: *"Gets the priority (natural ordering) of this 
 Provider"*,
 hence I would have expected it to work the same lowest-comes-first way, but
 apparently not – relying on your assessment here. I am also inclined to
 align them with the lowest-comes-first strategy, though this might have a
 bigger impact if there are 3rd party providers out there in the wild. Maybe
 others can weigh in here?
>>> 
>>> I can provide some additional evidence supporting the necessity to
>>> invert the order. At least one user on Stack Overflow was surprised
>>> that after adding `log4j-core` to his project, the loggers are still
>>> of type `org.apache.logging.slf4j.SLF4JLogger`:
>>> 
>>> https://stackoverflow.com/q/70487959/11748454
>>> 
>>> Piotr
>>