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

Modernize and split log4j-jul #2935

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Modernize and split log4j-jul #2935

merged 1 commit into from
Oct 22, 2024

Conversation

ppkarwasz
Copy link
Contributor

The current log4j-jul artifact contains four different features:

  1. A custom j.u.l.LogManager implementation. This implementation is actually rather generic and can be adapted to any kind of logging backend by implementing an o.a.l.l.j.AbstractLoggerAdapter. The default AbstractLoggerAdapter forwards all logging calls to the Log4j API.
  2. An implementation of o.a.l.l.j.AbstractLoggerAdapter called CoreLoggerAdapter that forwards the JUL Logger.setLevel and similar non-logging API method to log4j-core.
  3. A custom j.u.l.Handler implementation called Log4jBridgeHandler that forwards LogRecords to Log4j API. This handler can be used if the user can not change the implementation of j.u.l.LogManager.
  4. Using the Log4jBridgeHandler is very slow for log statements that are disabled in the Log4j API implementation, but not disabled in the default JUL LogManager implementation. Therefore log4j-jul offers a level propagator that modifies the level of JUL loggers, whenever the level of the corresponding Log4j Core loggers change. This feature of course depends on log4j-core.

This PR:

  • Removes CoreLoggerAdapter and related classes. IMHO users should not use j.u.l.Logger to modify the configuration of the logging backend.
  • Moves the level propagator to a new module log4j-jul-propagator. Note that the level propagator idea comes from SLF4J/Logback where jul-to-slf4j introduces a LevelChangePropagator interface, but the implementation is in Logback.
  • Revamps the o.a.l.l.j.LogManager:
    • It implements JUL methods that were introduced in Java 9.
    • It improves the implementation of methods that accept a sourceClass and sourceMethod parameters, by forwarding these parameters using LogBuilder#withLocation().
    • It fixes location detection, even if JUL Filters are used.

The last part probably could be improved: I don't see why we should support j.u.l.Filters. We could drop the feature in 3.x.

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 am not JUL-savvy enough to provide a meaningful review, but skimmed through the changes and dropped some remarks anyway.

It fixes location detection, even if JUL Filters are used.
The last part probably could be improved: I don't see why we should support j.u.l.Filters. We could drop the feature in 3.x.

Then let's drop it? I guess we can later on add it in a backward compatible manner if needed, right?

src/site/antora/modules/ROOT/pages/log4j-jul.adoc Outdated Show resolved Hide resolved
src/site/antora/modules/ROOT/pages/log4j-jul.adoc Outdated Show resolved Hide resolved
log4j-jul/pom.xml Outdated Show resolved Hide resolved
@ppkarwasz
Copy link
Contributor Author

It fixes location detection, even if JUL Filters are used.
The last part probably could be improved: I don't see why we should support j.u.l.Filters. We could drop the feature in 3.x.

Then let's drop it? I guess we can later on add it in a backward compatible manner if needed, right?

I agree, I will drop the j.u.l.Filter support.

@ppkarwasz
Copy link
Contributor Author

@vy,

I did some further changes to this PR:

  • b416ca5: I encapsulated the existing log4j-jul classes into three packages: spi contains interfaces and classes users can implement, support contains classes that can help implement SPI, internal is of limits and not exported through JPMS.
  • 03a70ff: I removed the support for j.u.l.Filter. IMHO this is not used and can provide users with hard to debug surprises if a legacy library messes with the logging configuration behind their back.

@ppkarwasz ppkarwasz self-assigned this Sep 16, 2024
@ppkarwasz ppkarwasz added this to the 3.x milestone Sep 17, 2024
@ppkarwasz ppkarwasz marked this pull request as draft September 17, 2024 09:22
@ppkarwasz
Copy link
Contributor Author

For backward compatibility with e.g. spring-boot-starter-log4j2, which uses Log4jBridgeHandler I would refactor this PR in the following way:

  • move the level change propagator back to log4j-jul and add a hard dependency on log4j-core. The lack of a level change propagator can really hurt the experience of spring-boot-starter-log4j2 users.
  • move o.a.l.l.j.LogManager out of log4j-jul to a jul-to-log4j artifact that depends on Log4j API only. This will require changes in some applications, which hard code a System.setProperty("java.util.logging.manager", "o.a.l.l.j.LogManager") in their main class (or shell scripts).

Note: a better alternative to a hardcoded o.a.l.l.j.LogManager would be to have a universal j.u.l.LogManager implementation that loads implementation using ServiceLoader like any other modern logging API. I have provided an initial mock-up of such a universal j.u.l.LogManager implementation in ppkarwasz/slf4j/jul-manager and opened qos-ch/slf4j#429 to test for support among Logback users.

Logback users have the most problems with JUL-to-Logback redirection, since they can only use:

  • Slf4jBridgeHandler, which as every Handler (appender) is inefficient,
  • A combination of log4j-jul + log4j-to-slf4j, which is not a direct path.

This splits `log4j-jul` into two artifacts:

- `jul-to-log4j` that contains a `j.u.l.LogManager` implementation, but
  does not depend on Log4j Core.
- `log4j-jul` that contains a `j.u.l.Handler` implementation and depends
  on Log4j Core.

We also update the `j.u.l.LogManager` implementation to:

- implement methods introduced in Java 9,
- remove methods deprecated in Java 9,
- remove the support for `j.u.l.Filter`.
@ppkarwasz ppkarwasz marked this pull request as ready for review October 1, 2024 14:25
@ppkarwasz
Copy link
Contributor Author

I am merging this first and then we can discuss where to put jul-to-log4j (e.g. in a new repo).

@ppkarwasz ppkarwasz merged commit 14f25a4 into main Oct 22, 2024
9 checks passed
@ppkarwasz ppkarwasz deleted the feature/main/split-jul branch October 22, 2024 16:57
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.

2 participants