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

Should ContextManager.initializeNewContext(T) always overwrite existing context? #202

Open
Marcono1234 opened this issue Feb 6, 2021 · 6 comments

Comments

@Marcono1234
Copy link
Contributor

It appears current implementations of nl.talsmasoftware.context.ContextManager.initializeNewContext(T), most notably the SLF4J MDC one, overwrite the existing context.

However, I think there might be cases where it is desirable to only append the context to the existing one. For example assume you have an Executor for whose threads an MDC entry is added to allow identifying in the log files which worker thread performed which work. When a work task is then executed the MDC of the work task should only be added on top of the existing MDC and when finished these changes should be reverted.

@sjoerdtalsma
Copy link
Contributor

I don't know, quite often in the case of thread pools the pre-existence of other values has not really been an issue I guess.
I think this design choice was made out of simplicity, I often choose the most simple solution for any problem, which normally makes a lot of sense.

Could you describe your use-case so there is a little more background into why appending to the existing MDC context in this case is a better choice?

I'm not disagreeing with you, I'm just interested what made you experience this as an issue.

Merging non-singleton context values instead of overwriting them seems a reasonable approach, although more complex than replacing it.

@Marcono1234
Copy link
Contributor Author

Could you describe your use-case so there is a little more background into why appending to the existing MDC context in this case is a better choice?

Sorry, I don't have any specific use-case in mind, and I guess since this is a versatile library you cannot really predict all the ways how it will be used. Nevertheless I think this library should not be so "destructive", replacing the previous context before restoring it again. However, on the other hand there might be cases where this is desired (though this indicates that the previous context might not have been cleared up properly?).

The hypothetical use-case, as described above a little bit, is that you have context information for both worker threads and the work tasks they perform. If you are using a logging framework / API such as SLF4J or Log4j2 you might want to log per worker thread, and per work task. Assuming the user is using this library correctly, i.e. they always close the context once a work task is done, the existing context within a worker thread represents the context of that thread and should be available while the work task is running.

@sjoerdtalsma
Copy link
Contributor

sjoerdtalsma commented Mar 1, 2021

The hypothetical use-case, as described above a little bit, is that you have context information for both worker threads and the work tasks they perform.

This may be a valid use-case. I tend to think of worker threads mostly from the 'interchangeable' threadpool where pre-existing context is normally either blank or unimportant. In the use-case of pre-initialized threads, merging the context into the new thread may be a preferred mechanism.

I have been troubled by MDC propagation anyway, as normally you would also want to exclude certain explicitly thread-bound values from propagating (e.g. some logical thread-name is probably bad to propagate to a worker-thread).

The MDC being a map of all kind of values makes this a difficult functional problem.

@sjoerdtalsma
Copy link
Contributor

Perhaps we could implement a merge, but also introduce either a blacklist or whitelist feature to allow some kind of filter on exactly which MDC keys to propagate to worker threads.

@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Mar 1, 2021

e.g. some logical thread-name is probably bad to propagate to a worker-thread

Perhaps we could implement a merge, but also introduce either a blacklist or whitelist feature to allow some kind of filter on exactly which MDC keys to propagate to worker threads.

Sounds reasonable, but how / where should such a filter be defined? I am not sure if there are any standard names (same applies for Log4j 2 ThreadContext). Putting this logic to ContextManagers.createContextSnapshot() is probably not an option because such a filter would only be relevant to a limited number of context types.
Maybe a static method on Slf4jMdc could be added to allow setting a custom filter (with the default filter allowing everything). This filter would then be consulted when getActiveContext() is called.
Though should that filter then be global, or only for the calling thread?

@sjoerdtalsma
Copy link
Contributor

Good questions.
I think I'm going to let this one lie for a while and wait if others have a need for such a feature. Implementing something guessing the answers to such functional questions doesn't seem to be very prudent.
Perhaps others have good ideas as well.

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

No branches or pull requests

2 participants