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

Add Logback's throwable-consuming semantics as an option #2381

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

ppkarwasz
Copy link
Contributor

The semantics used to determine the Throwable attached to a log event changed in Logback 1.1.0 (in the fix to LOGBACK-873).

When there are not enough arguments to the log call to fill both the throwable field and the placeholders contained in the message pattern, Logback 1.1.0 and later prefer to fill the throwable field and leave some placeholders empty. Log4j Core does the opposite choice: it first fills all the placeholders and only fills the throwable field if there is something left.

This change allows log4j-slf4j-impl and log4j-slf4j2-impl users to switch between the two behaviors by setting the property log4j2.messageFactory to:

org.apache.logging.slf4j.message.ThrowableConsumingMessageFactory

In this PR two choices have been made:

  • the message factory is only included in log4j-slf4j-impl and log4j-slf4j2-impl, since it should be mostly required by Logback users during a transition period. Note that Logback's and Log4j's semantics to deal with throwables agree whenever enough arguments are provided. In a second step users can modify their code using Add recipe to clone an exception argument openrewrite/rewrite-logging-frameworks#141, so that it behaves in the same way in Logback and Log4j Core out-of-the-box. However I can also create a new log4j-message artifact for this.
  • no garbage-free version of this factory is provided. SLF4J creates temporary arrays whenever more than 2 arguments are provided. I believe that an additional creation of a ParameterizeMessage instance does not make a difference.

Closes #2363

The semantics used to determine the `Throwable` attached to a log event
changed in Logback 1.1.0 (in the fix to [LOGBACK-873](https://jira.qos.ch/browse/LOGBACK-873)).

When there are not enough arguments to the log call to fill both the
throwable field and the placeholders contained in the message pattern,
Logback 1.1.0 and later prefer to fill the throwable field and leave
some placeholders empty. Log4j Core does the opposite choice: it first
fills all the placeholders and only fills the throwable field if there
is something left.

This change allows `log4j-slf4j-impl` and `log4j-slf4j2-impl` users to
switch between the two behaviors by setting the property
`log4j2.messageFactory` to:

```
org.apache.logging.slf4j.message.ThrowableConsumingMessageFactory
```
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.

LGTM, thanks for taking care of this @ppkarwasz! 💯
I have two nits:

  1. Would you mind explicitly stating in all newly added Java source files something like the following, please?
This class should be identical to its clone in {@code log4j-slf4j2-impl}.
Any changes made here should be reflected to the other one, and vice versa.
  1. Would you mind documenting this in the following files, please?
    1. migrate-from-logback.adoc
    2. installation.adoc#impl-logback

@ppkarwasz
Copy link
Contributor Author

@vy,

1. Would you mind explicitly stating in **all** newly added Java source files something like the following, please?
This class should be identical to its clone in {@code log4j-slf4j2-impl}.
Any changes made here should be reflected to the other one, and vice versa.

I don't think this is necessary. Every time one of log4j-slf4j-impl or log4j-slf4j2-impl is modified, the other one needs to be modified too.

2. Would you mind documenting this in the following files, please?
   
   1. `migrate-from-logback.adoc`
   2. `installation.adoc#impl-logback`

I added some documentation to migrate-from-logback.adoc in f7a6320.
The ThrowableConsumingMessageFactory is only useful for people that migrate from Logback, so I didn't add it to installation.adoc.

@ppkarwasz ppkarwasz merged commit c977798 into 2.x Jul 2, 2024
8 of 9 checks passed
@ppkarwasz ppkarwasz deleted the fix/2363_add_logback_throwable_consuming_semantics branch July 2, 2024 18:55
vy pushed a commit that referenced this pull request Jul 5, 2024
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.

Add Logback's throwable-consuming semantics as an option
2 participants