Re: [log4j] Overhauling API initialization in Log4j 3.x

[Ralph Goers]

I don’t really have much to say about the confluence page. It is simply 
documenting how things work. I should add that I believe the static block in 
LogManager has been there since the very first commit. However, things have 
gotten much more complicated, especially due to the addition of the garbage 
free code.

I don’t have any real issues with what you are proposing in the second 
paragraph. 

My feeling is that log4j-api should not contain any code that can only be used 
by log4j modules. Everything in it should be primarily targeted at users or 
things that can be used by any implementation of the API.  

As for log4j-spi, we would have to discuss what the purpose of that is. If it 
is stuff that is available to any logging implementation that would be good 
from the standpoint of making it obvious that it isn’t for end-user 
consumption. But we already have an spi package in log4j-api so I wonder if 
that is actually necessary. We just haven’t done a very good job of putting 
packages and classes under the spi package. I think any classes that are 
designed to only be used by the log4j implementation belong in log4j-core. But 
even there we need to do a better job of identifying what is available to users 
and what is internal to log4j.

So the devil is in the details. I don’t think you can just make a general 
proposal without discussing at least some of the classes involved and what you 
are trying to achieve with them.

Ralph


> On Nov 5, 2022, at 2:32 PM, Matt Sicker  wrote:
> 
> After an attempt at soloing the idea, I’ve reverted my changes and am instead 
> documenting the current system along with adding some proposals in the 
> following Confluence doc [0]. I’ve analyzed the initialization ordering of 
> the API (without going into detail of how log4j-core initializes as this 
> differs fairly significantly in 2.x and 3.x at this point). Right now, some 
> of the main differences between 2.x and master is that most of the documented 
> static fields from initialization are currently refactored into Lazy 
> static fields, though that only defers initialization until slightly later.
> 
> I’d like to work on a proper SPI here so that I can get back to considering 
> updates to log4j-core and dependency injection. In general, my idea is to add 
> a getInstance(Class) method to the API provider class which would be used 
> to get the LoggerContextFactory along with initial instances for 
> ThreadContextMap and ThreadContextFactory. This could become a natural place 
> to also return a PropertyEnvironment instance (the interface that 
> PropertiesUtil now implements) to further avoid use of static state.
> 
> Let me know what you think of the idea. Do we need a separate log4j-spi 
> module? Other ideas?
> 
> [0]: https://cwiki.apache.org/confluence/display/LOGGING/API+Initialization
> —
> Matt Sicker
> 

Re: [log4j] Overhauling API initialization in Log4j 3.x

[Piotr P. Karwasz]

Hi Matt,

On Sat, 5 Nov 2022 at 22:32, Matt Sicker  wrote:
> After an attempt at soloing the idea, I’ve reverted my changes and am instead 
> documenting the current system along with adding some proposals in the 
> following Confluence doc [0]. I’ve analyzed the initialization ordering of 
> the API (without going into detail of how log4j-core initializes as this 
> differs fairly significantly in 2.x and 3.x at this point). Right now, some 
> of the main differences between 2.x and master is that most of the documented 
> static fields from initialization are currently refactored into Lazy 
> static fields, though that only defers initialization until slightly later.

Thanks for analyzing and writing down this quite complex initialization process.

My main concerns with the way this works now are:

1. Logging of errors in the pre-StatusLogger phase is inconsistent.
Some classes use `LowLevelLogUtil`, some ignore errors, while the
usage of StatusLogger I introduced in `ServiceLoaderUtil` is brittle:
it requires to call `loadServices` with `verbose = false` to disable
the usage of `StatusLogger`. I think that the ideal solution would be
to remove enough static initialization from `StatusLogger` and
`AbstractLogger` (i.e. move it to constructors) so that
LowLevelLogUtil can create a `StatusLogger` with a hardcoded
configuration. Later in the initialization process we can replace it
with the real `StatusLogger`.

2. There are still a couple of singleton classes that cache the result
of `PropertiesUtil.getProperty` in private static fields. I think
these calls can be easily moved to the constructor of the class. E.g.
`StatusLogger` could easily have a constructor that does not require
`PropertiesUtil` at all.

3. The way `PropertiesUtil` caches values could be simplified (cf.
discussion in https://issues.apache.org/jira/browse/LOG4J2-3621): it
caches all values from environment variables and Java system
properties at startup. Most of the time none of those values configure
Log4j2. On the other hand missing values are not cached, so most of
the time a lookup for `log4j2.configurationFile` will find no value in
the cache, perform a lookup in all available property sources and
return `null`.

Most of these changes can also be done in `release-2.x` without
breaking public and protected members.

Piotr

Re: [log4j] Overhauling API initialization in Log4j 3.x

[Matt Sicker]

On the LowLevelLogUtil front, I’ve updated that to use StatusLogger now once 
StatusLogger has initialized. The code could probably be backported to 2.x.

As for how PropertiesUtil caches values, simplifications there would be great!
—
Matt Sicker

> On Nov 6, 2022, at 03:37, Piotr P. Karwasz  wrote:
> 
> Hi Matt,
> 
> On Sat, 5 Nov 2022 at 22:32, Matt Sicker  wrote:
>> After an attempt at soloing the idea, I’ve reverted my changes and am 
>> instead documenting the current system along with adding some proposals in 
>> the following Confluence doc [0]. I’ve analyzed the initialization ordering 
>> of the API (without going into detail of how log4j-core initializes as this 
>> differs fairly significantly in 2.x and 3.x at this point). Right now, some 
>> of the main differences between 2.x and master is that most of the 
>> documented static fields from initialization are currently refactored into 
>> Lazy static fields, though that only defers initialization until slightly 
>> later.
> 
> Thanks for analyzing and writing down this quite complex initialization 
> process.
> 
> My main concerns with the way this works now are:
> 
> 1. Logging of errors in the pre-StatusLogger phase is inconsistent.
> Some classes use `LowLevelLogUtil`, some ignore errors, while the
> usage of StatusLogger I introduced in `ServiceLoaderUtil` is brittle:
> it requires to call `loadServices` with `verbose = false` to disable
> the usage of `StatusLogger`. I think that the ideal solution would be
> to remove enough static initialization from `StatusLogger` and
> `AbstractLogger` (i.e. move it to constructors) so that
> LowLevelLogUtil can create a `StatusLogger` with a hardcoded
> configuration. Later in the initialization process we can replace it
> with the real `StatusLogger`.
> 
> 2. There are still a couple of singleton classes that cache the result
> of `PropertiesUtil.getProperty` in private static fields. I think
> these calls can be easily moved to the constructor of the class. E.g.
> `StatusLogger` could easily have a constructor that does not require
> `PropertiesUtil` at all.
> 
> 3. The way `PropertiesUtil` caches values could be simplified (cf.
> discussion in https://issues.apache.org/jira/browse/LOG4J2-3621): it
> caches all values from environment variables and Java system
> properties at startup. Most of the time none of those values configure
> Log4j2. On the other hand missing values are not cached, so most of
> the time a lookup for `log4j2.configurationFile` will find no value in
> the cache, perform a lookup in all available property sources and
> return `null`.
> 
> Most of these changes can also be done in `release-2.x` without
> breaking public and protected members.
> 
> Piotr

Re: [log4j] Overhauling API initialization in Log4j 3.x

[Ralph Goers]

Note that PropertiesUtil will need some work in master to implement 
https://cwiki.apache.org/confluence/display/LOGGING/Properties+Enhancement 
.

Ralph

> On Nov 6, 2022, at 6:37 PM, Matt Sicker  wrote:
> 
> On the LowLevelLogUtil front, I’ve updated that to use StatusLogger now once 
> StatusLogger has initialized. The code could probably be backported to 2.x.
> 
> As for how PropertiesUtil caches values, simplifications there would be great!
> —
> Matt Sicker
> 
>> On Nov 6, 2022, at 03:37, Piotr P. Karwasz  wrote:
>> 
>> Hi Matt,
>> 
>> On Sat, 5 Nov 2022 at 22:32, Matt Sicker  wrote:
>>> After an attempt at soloing the idea, I’ve reverted my changes and am 
>>> instead documenting the current system along with adding some proposals in 
>>> the following Confluence doc [0]. I’ve analyzed the initialization ordering 
>>> of the API (without going into detail of how log4j-core initializes as this 
>>> differs fairly significantly in 2.x and 3.x at this point). Right now, some 
>>> of the main differences between 2.x and master is that most of the 
>>> documented static fields from initialization are currently refactored into 
>>> Lazy static fields, though that only defers initialization until 
>>> slightly later.
>> 
>> Thanks for analyzing and writing down this quite complex initialization 
>> process.
>> 
>> My main concerns with the way this works now are:
>> 
>> 1. Logging of errors in the pre-StatusLogger phase is inconsistent.
>> Some classes use `LowLevelLogUtil`, some ignore errors, while the
>> usage of StatusLogger I introduced in `ServiceLoaderUtil` is brittle:
>> it requires to call `loadServices` with `verbose = false` to disable
>> the usage of `StatusLogger`. I think that the ideal solution would be
>> to remove enough static initialization from `StatusLogger` and
>> `AbstractLogger` (i.e. move it to constructors) so that
>> LowLevelLogUtil can create a `StatusLogger` with a hardcoded
>> configuration. Later in the initialization process we can replace it
>> with the real `StatusLogger`.
>> 
>> 2. There are still a couple of singleton classes that cache the result
>> of `PropertiesUtil.getProperty` in private static fields. I think
>> these calls can be easily moved to the constructor of the class. E.g.
>> `StatusLogger` could easily have a constructor that does not require
>> `PropertiesUtil` at all.
>> 
>> 3. The way `PropertiesUtil` caches values could be simplified (cf.
>> discussion in https://issues.apache.org/jira/browse/LOG4J2-3621): it
>> caches all values from environment variables and Java system
>> properties at startup. Most of the time none of those values configure
>> Log4j2. On the other hand missing values are not cached, so most of
>> the time a lookup for `log4j2.configurationFile` will find no value in
>> the cache, perform a lookup in all available property sources and
>> return `null`.
>> 
>> Most of these changes can also be done in `release-2.x` without
>> breaking public and protected members.
>> 
>> Piotr
>