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

MdcThreadLocalAccessor for SLF4J #191

Closed
chemicL opened this issue Jan 26, 2024 · 4 comments · Fixed by #194
Closed

MdcThreadLocalAccessor for SLF4J #191

chemicL opened this issue Jan 26, 2024 · 4 comments · Fixed by #194
Assignees
Labels
enhancement A general enhancement
Milestone

Comments

@chemicL
Copy link
Collaborator

chemicL commented Jan 26, 2024

As suggested in spring-projects/spring-boot#39299, it feels reasonable to create a new utility module for SLF4J with a ThreadLocalAccessor (TLA) that allows interacting with the MDC during Thread hops. I propose two implementations:

  • global MDC TLA, which copies and overrides the entire contents of MDC -> should only be used when the user is certain no other accessor uses the MDC
  • key-based MDC TLA, which only transports specified entries across Thread boundaries.

@ttddyy, @rstoyanchev @marcingrzejszczak @wilkinsona does this sound ok?

@rstoyanchev
Copy link
Collaborator

What about just an additional package in the same module as an optional compile-only dependency?

@jonatan-ivanov jonatan-ivanov added this to the 1.next milestone Jan 26, 2024
@chemicL
Copy link
Collaborator Author

chemicL commented Jan 29, 2024

@rstoyanchev that's a good idea. I'm going in this direction.

I have a design consideration at this point for the key-based approach. I can think of two options when it comes to the API:

Approach 1: Accept a list of keys of type String

Pros:

  • saves on copying the contents of MDC upon every access because we just select the given keys
  • saves memory and CPU
  • it's easier to provide a List<String> than to provide a Predicate<String>
  • it is very predictable

Cons:

  • inflexible - families of keys can't be selected this way

Approach 2: Accept a Predicate<String> to determine which keys to use in the snapshot

Pros:

  • very flexible, can allow to select a family of keys that for instance share a prefix

Cons:

  • every access requires to copy the MDC map in order to traverse all the keys which is not very performant
  • waste of memory and CPU time
  • when someone explicitly provides a Map for some ContextAccessor that has entries which would have been filtered out during ThreadLocal capture, we either apply the filtering when setting the values or we skip filtering, which can lead to unexpected results

I'd love some feedback before pushing a PR.

@chemicL
Copy link
Collaborator Author

chemicL commented Jan 31, 2024

I think I'll go with Approach 1. If a more flexible approach is required, the users can implement such an accessor based on how the list-based approach is implemented.

Regarding my concern:

when someone explicitly provides a Map for some ContextAccessor that has entries which would have been filtered out during ThreadLocal capture, we either apply the filtering when setting the values or we skip filtering, which can lead to unexpected results

It actually applies to both approaches and I'll filter out keys that don't match both in the restoring and in capturing passes.

@chemicL
Copy link
Collaborator Author

chemicL commented Feb 5, 2024

Resolved by #194

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

Successfully merging a pull request may close this issue.

3 participants