Skip to content

Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) #3372

New issue

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

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

Already on GitHub? # to your account

Merged
merged 8 commits into from
May 25, 2025

Conversation

JWT007
Copy link
Contributor

@JWT007 JWT007 commented Jan 8, 2025

In LoggerConfig.RootLogger.Builder:

  • Deprecated method with typo in name 'withtFilter'.
  • Added correct method 'withFilter'.

@ppkarwasz
Copy link
Contributor

@JWT007,

Can you add a changelog entry (of type added)?

Copy link

github-actions bot commented Jan 8, 2025

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

We should use setFilter instead of withFilter.

I also noticed that none of our tests use the LoggerConfig builder and they call the deprecated LoggerConfig#createLogger method instead. Could you fix this?

@JWT007 JWT007 requested a review from ppkarwasz February 16, 2025 19:34
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

LGTM

@ppkarwasz ppkarwasz added this to the 2.25.0 milestone Apr 13, 2025
@ppkarwasz
Copy link
Contributor

@JWT007,

Your two last commits are not signed and linked to the @jethomas-tsi account. Could you resign them, so I can merge the PR?

git --force-rebase --gpg-sign origin/2.x
git push -f

BTW: if you wait for the required checks to pass, feel free to merge this yourself.

JWT007 added 4 commits May 25, 2025 14:51
…der (apache#3369)

Deprecated LoggerConfig.RootLogger.Builder#withtFilter(Filter).

Added LoggerConfig.RoottLogger.Builder#withFilter(Filtter)
+ properly delegated from deprecated method to new method
+ updated affected package-info versions for baseline
* switched tests to use Builders instead of createLogger
* changed new LoggerConig.RootLogger.Builder#withFilter to LoggerConfig.RootLogger.Builder#setFilter
* deprecated LoggerConfig.Builder.withFilter and added LoggerConig.Builder.setFilter
* moved changelog to correct .2.x.x directory
@ppkarwasz ppkarwasz force-pushed the bugfix/LOG4J-3369 branch from 78cc906 to d496f03 Compare May 25, 2025 12:53
ppkarwasz added 4 commits May 25, 2025 14:57
The `o.a.l.l.core.async` package was modified indirectly: `AsyncLoggerConfig` inherited the changes to `LoggerConfig`.
@ppkarwasz ppkarwasz enabled auto-merge (squash) May 25, 2025 13:16
@ppkarwasz ppkarwasz merged commit 46db3f9 into apache:2.x May 25, 2025
7 checks passed
@github-project-automation github-project-automation bot moved this from To triage to Done in Log4j bug tracker May 25, 2025
@norrisjeremy
Copy link

We should use setFilter instead of withFilter.

I also noticed that none of our tests use the LoggerConfig builder and they call the deprecated LoggerConfig#createLogger method instead. Could you fix this?

@ppkarwasz
What is the justification for deprecating withFilter()?
Nothing else in this builder class is configured with a setter, but rather they are all withers, so this seems to introduce an API inconsistency.

@ppkarwasz
Copy link
Contributor

@norrisjeremy,

The overall goal is to transition all withers to setters in version 3.x, as described in #1206. Since the builders are mutable, the purpose is to use a naming pattern more common for mutable objects.

@norrisjeremy
Copy link

@norrisjeremy,

The overall goal is to transition all withers to setters in version 3.x, as described in #1206. Since the builders are mutable, the purpose is to use a naming pattern more common for mutable objects.

@ppkarwasz,

  1. So why weren't all the other wither methods replaced with setters and marked as deprecated too?
  2. And why wasn't Replace withers with setters in master #1206 mentioned at all in this PR in the first place?
  3. And why wasn't this change documented in the Deprecated section of the Release Notes?

From a user's perspective, this change seems poorly implemented and documented, which is extremely frustrating.

@ppkarwasz
Copy link
Contributor

ppkarwasz commented Jun 17, 2025

Hi @norrisjeremy,

Thanks for your questions—these are all excellent points, and I really appreciate you taking the time to raise them.

  1. So why weren't all the other wither methods replaced with setters and marked as deprecated too?

You're absolutely right to highlight the inconsistency. The focus of the PR was to correct a specific issue with withtFilter (with an additional t), and the introduction of setFilter was a side-effect driven by the fact that withFilter is already deprecated in 3.x. So it made sense to align it preemptively here.

Replacing all wither methods in the same Log4j Core plugin—or across all plugins—would indeed be more consistent. But that would have significantly expanded the scope of this PR. Since there are around 300 plugins, we felt it was better to handle that broader change separately. To address this, I’ve opened issue #3750 to track the work of adding setters and deprecating all remaining withers. If you're interested, contributions are always welcome!

  1. And why wasn't issue #1206 referenced in the PR?**

It was actually mentioned in a resolved conversation here: #3372 (comment)

  1. Why wasn’t this change documented in the Deprecated section of the Release Notes?

That’s a fair point. When preparing release notes, we tend to prioritize high-impact changes for the majority of users, most of whom use Log4j via configuration files. The Log4j Core internal API is mostly used by Log4j Plugin developers and integrators. As a result, API-level changes like this one can get less visibility, especially if they're part of a narrowly scoped fix.

That said, you're absolutely right: this deprecation should have been classified as deprecated, not just changed. Feel free to submit a PR correcting that in the release notes—those are always welcome, even after the release, and can help future users.

From a user's perspective, this change seems poorly implemented and documented, which is extremely frustrating.

I hear you, and I’m sorry the process came across that way. We always aim to provide a stable and predictable experience, and clearly we can improve how we communicate these kinds of changes.

Out of curiosity—what kind of impact did this deprecation have on your application? Was it mainly the compiler warnings, or something else? Understanding that might help us improve how we handle and communicate such changes going forward.

Thanks again for your feedback—it’s valuable and helps us improve the project for everyone.

@norrisjeremy
Copy link

norrisjeremy commented Jun 17, 2025

Hi @ppkarwasz,

Thank you for your detailed response.

To answer your question about the impact: we deliberately run compiler warnings & deprecations set to errors for our internal apps, so that they immediately cause a compilation failure, in order for our team to notice and deal with API deprecations immediately and avoid having them pile up over time.

So we immediately noticed the deprecation when a Dependabot PR for one of our internal apps failed to build yesterday.
Upon seeing the deprecation warning, we first turned to the release notes for the new 2.25.0 release, but did not note any mention of the withFilter() method being deprecated.

And then when trying to load up the latest Javadocs, noticed that they had not yet been published on the website.
(And in fact updated Javadocs are still not published, as all the pages linked here lead to 404 page not found errors...)

This then led us to having to go dumpster diving through Github, to determine how to handle the deprecation, as well as why it had occurred.

On top of this, this is the second time in past year that this API method has been touched.
In the 2.24.0 release, the original withtFilter() was marked as deprecated in lieu of the new withFilter() method (see #788).

So all told, this is just frustrating: it results in lost time & productivity for what is amounting to whack-a-mole operation, chasing the whims of upstream library developers: deprecating and changing withers to setters in a Builder class just doesn't seem to offer tangible benefit to any existing users.

Thanks,
Jeremy

@ppkarwasz
Copy link
Contributor

Hi @norrisjeremy,

To answer your question about the impact: we deliberately run compiler warnings & deprecations set to errors for our internal apps, so that they immediately cause a compilation failure, in order for our team to notice and deal with API deprecations immediately and avoid having them pile up over time.

That’s a great policy—seriously! 👏 It’s not something we see often; more typically, teams tend to tolerate deprecated methods and configuration properties until something breaks. Because of that, we don't usually factor the impact of deprecations into our decision-making. So, thank you for calling attention to your approach—it’s helpful context for us going forward.

And then when trying to load up the latest Javadocs, noticed that they had not yet been published on the website. (And in fact updated Javadocs are still not published, as all the pages linked here lead to 404 page not found errors...)

That was ironically caused by a recent change in the Maven Javadoc Plugin (apache/maven-javadoc-plugin#1163). While it improved consistency, it also broke our site build 😉. This has now been fixed—thanks for bringing it to our attention!

On top of this, this is the second time in past year that this API method has been touched. In the 2.24.0 release, the original withtFilter() was marked as deprecated in lieu of the new withFilter() method (see #788).

Yes, good catch. That change was made in 21ad7be, back when we still allowed Commit-then-Review for “trivial” changes. That likely contributed to us forgetting about the "review" part of the process. We've since moved to a strict Review-then-Commit policy for all changes, which should help avoid similar oversights in the future.

So all told, this is just frustrating: it results in lost time & productivity for what is amounting to whack-a-mole operation, chasing the whims of upstream library developers: deprecating and changing withers to setters in a Builder class just doesn't seem to offer tangible benefit to any existing users.

Totally understandable. This change reflects an effort to standardize the API and address inconsistencies that date back to the early development of Log4j 2. That said, I agree that the change has only an aesthetic effect. I don't want to leave things in the middle, so I’ve scheduled #3750 for the 2.26.0 milestone, but we will deprecate all remaining "withers" in a single go.

As mentioned, we assume most users rely on configuration files—which are unaffected by these changes. But for those constructing configurations programmatically, we recommend using ConfigurationBuilder. It's a more stable and future-proof approach, unlike directly using plugin builders, which can be more volatile.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants