Skip to content

Delegate ScopedContext functionality to interface #2445

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

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

ppkarwasz
Copy link
Contributor

To provide more configurability for the ScopedContext service, this PR moves its implementation details to log4j-core and replaces it with a ScopedContextProvider interface.

In Log4j API only a NO-OP version of the provider is present, but each implementation of the API can provide its own.

To provide more configurability for the `ScopedContext` service, this PR
moves its implementation details to `log4j-core` and replaces it with a
`ScopedContextProvider` interface.

In Log4j API only a NO-OP version of the provider is present, but each
implementation of the API can provide its own.
@ppkarwasz ppkarwasz mentioned this pull request Apr 4, 2024
@ppkarwasz ppkarwasz requested a review from rgoers April 4, 2024 22:08
@rgoers
Copy link
Member

rgoers commented Apr 4, 2024

I took a look at this again. I missed some things the first time through it. I like most of this but will want to change it a bit.

@ppkarwasz
Copy link
Contributor Author

I took a look at this again. I missed some things the first time through it. I like most of this but will want to change it a bit.

Feel free to directly modify this branch.

@rgoers rgoers merged commit e28affa into ScopedContext Apr 5, 2024
@rgoers rgoers deleted the ScopedContext-replace-with-interface branch April 5, 2024 06:22
@rgoers
Copy link
Member

rgoers commented Apr 5, 2024

Rats. I didn't mean to but I merged my changes to this branch by mistake. They are now in the ScopedContext branch as well.
Oh - I just notice you said it was OK to modify this branch.

I have a question though. I just noticed that you made the default ThreadContext provider a no-op. How did that manage to pass unit tests? Why does it need to be a no-op. For one, SimpleLogger includes the MDC in the output so I would think anyone using it would expect it to work.

@ppkarwasz
Copy link
Contributor Author

I have a question though. I just noticed that you made the default ThreadContext provider a no-op. How did that manage to pass unit tests? Why does it need to be a no-op. For one, SimpleLogger includes the MDC in the output so I would think anyone using it would expect it to work.

I removed ScopedContext from SimpleLogger: IMHO it isn't a simple logger any more if it contains complex data.

Since I was mostly concerned with ScopedContext, I disabled the ResourceLoggerTest temporarily (shouldn't ResourceLogger be in another PR anyway?). I moved the ScopedContextTest to log4j-core, since this is where a non-trivial implementation resides. In log4j-api-test I left an abstract class that should be used to test all the implementations of ScopedContextProvider.

@rgoers
Copy link
Member

rgoers commented Apr 5, 2024

I disagree with your premise. SimpleLogger has always supported logging the ThreadContext. Including the ScopedContext as well makes sense since they can be interchangeably used. You adding a Noop implementation of ThreadContext essentially breaks backward compatibility. We need a minimal implementation in the API.

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