Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centralize initialization in Provider class #2374

Merged
merged 4 commits into from
Mar 21, 2024
Merged

Conversation

ppkarwasz
Copy link
Contributor

We move the code responsible for the instantiation of LoggerContextFactory and ThreadContextMap from the static entry points to the logging system (LogManager and ThreadContext) to the Provider class.

The Provider class is instantiated using ServiceLoader, so log4j-core 2.x and 3.x can reimplement the initialization process according to their own rules. E.g. log4j-core 3.x can use the DI to create an instance of LoggerContextFactory and ThreadContextMap.

The following modification were performed:

  • a new system property log4j.provider was introduced,
  • the old log4j2.loggerContextFactory has been deprecated and revised: if set it selects the first provider that uses the given LoggerContextFactory. Therefore it selects now both the context factory and thread context map implementations,
  • private static configuration values were removed from ThreadContextMap implementations, helping test parallelisation,
  • a distinct NoOpThreadContextStack implementation has been introduced.

This PR only includes tests for the first two items. The remaining points should be covered by existing tests.

ppkarwasz added a commit that referenced this pull request Mar 13, 2024
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling we can deliver a solution with less new public methods/members, but... I am curious what you think.

We move the code responsible for the instantiation of
`LoggerContextFactory` and `ThreadContextMap` from the static entry
points to the logging system (`LogManager` and `ThreadContext`) to the
`Provider` class.

The `Provider` class is instantiated using `ServiceLoader`, so
`log4j-core` 2.x and 3.x can reimplement the initialization process
according to their own rules. E.g. `log4j-core` 3.x can use the DI to
create an instance of `LoggerContextFactory` and `ThreadContextMap`.

The following modification were performed:

* a **new** system property `log4j.provider` was introduced,
* the old `log4j2.loggerContextFactory` has been deprecated and revised: if set it
  selects the first provider that uses the given `LoggerContextFactory`.
  Therefore it selects now both the context factory and thread context
  map implementations,
* private static configuration values were removed from
  `ThreadContextMap` implementations, helping test parallelisation,
* a distinct `NoOpThreadContextStack` implementation has been
  introduced.
@ppkarwasz ppkarwasz merged commit 3cfb7b5 into 2.x Mar 21, 2024
6 checks passed
@ppkarwasz ppkarwasz deleted the feature/provider-revamp branch March 21, 2024 21:54
@rgoers
Copy link
Member

rgoers commented Jun 17, 2024

I apologize for taking so long to look at this but while working on moving context related stuff to log4j-context-data I ran into this and it makes things much more difficult as it hard-wires a bunch of classes that should be internal/private into the Provider class. I would request that those changes be undone.

@ppkarwasz
Copy link
Contributor Author

Those internal/private classes are hardwired in Provider, since implementations of Provider have no way to access those classes.

Once I'll promote the StringArrayThreadContextMap to default thread context map, I'll be able to either:

  • remove those private implementations (since the default context map can be used for garbage-free logging)
  • or move them to log4j-core (since no other implementation really profits from them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants