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

Copy MDC context in workspace runtime (start stop) related async tasks #8585

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

sunix
Copy link
Contributor

@sunix sunix commented Feb 2, 2018

What does this PR do?

When MDC is used in Che log configuration, we figured out that some information were not passed through workspace start and stop tasks. These tasks were executed asynchronously to different thread pools.
This PR is fixing the issue by copying the context. It uses kind of the same mechanism that has been used for thread local in the same portion of code.

What issues does this PR fix or reference?

redhat-developer/rh-che#495

@skabashnyuk
Copy link
Contributor

I would say we need to solve this task a bit more general. My proposal is to create two classes

  1. MDCContext analog of EnvironmentContext. Where MDC is stored in thread local variable + static method to ThreadLocalPropagateContext.addThreadLocal(currentMDC);
  2. MDCFilter to setup MDC in MDCContext. This filter can be configured for master and agent.

@sunix
Copy link
Contributor Author

sunix commented Feb 2, 2018

@skabashnyuk can you explain "more general" ? I don't see which improvements will bring your suggestions and feel like it is adding more complexity.

@benoitf benoitf added kind/bug Outline of a bug - must adhere to the bug report template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Feb 5, 2018
@skabashnyuk
Copy link
Contributor

skabashnyuk commented Feb 5, 2018

As far as I understand the problem that MDC context is lost when one thread delegates some work to another with help of ThreadLocalPropagateContext. And you are fixed it in one particular place WorkspaceRuntimes.
My proposal was to solving this issue in any place we are using ThreadLocalPropagateContext not only in WorkspaceRuntimes with the build-in functionality of ThreadLocalPropagateContext.

@garagatyi
Copy link

+1 to use it in all the places ThreadLocalPropagateContext is used as @skabashnyuk suggested

@sunix
Copy link
Contributor Author

sunix commented Feb 5, 2018

@skabashnyuk OK thanks, understand works for me

@sunix sunix force-pushed the copy-mdc-workspace-start-stop branch from bf69744 to e7a85a4 Compare February 6, 2018 15:06
…ropagateContext.wrap method

Signed-off-by: Sun Tan <sutan@redhat.com>
@sunix sunix force-pushed the copy-mdc-workspace-start-stop branch from e7a85a4 to 0f6fe41 Compare February 6, 2018 15:07
@sunix
Copy link
Contributor Author

sunix commented Feb 6, 2018

@skabashnyuk @garagatyi I've updated the PR as requested, let me know if it works for you

Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

Hello.
Close but not exactly what I thought.
The idea was to consider ThreadLocalPropagateContext as a framework that is propagating thread-local variables to any child Threads. I was expecting to see
something very similar to EnvironmentContext(EnvironmentContextFilter). But in your case, you are modifying
framework itself by adding a direct dependency on slf4j.

But I saw recommendation https://logback.qos.ch/manual/mdc.html#managedThreads and don't mind if you merge it as is.
BTW. I haven't seen in our filters a place where we reset MDC. Are you using MDCInsertingServletFilter in some place?

@sunix
Copy link
Contributor Author

sunix commented Feb 7, 2018

@skabashnyuk Yes, I will keep like that because MDC is using a mechanism similar to thread local but which is not thread local so it won't make sense (and may have strange behaviour) to mix the two of them. Could you approve this PR ?
About cleaning, I will create another pull request as it seems we forgot to clean MDC (for request id and identity id) and information could be passed if a thread is reused. Will do something similar to https://github.com/qos-ch/logback/blob/master/logback-classic/src/main/java/ch/qos/logback/classic/helpers/MDCInsertingServletFilter.java#L75
MDCInsertingServletFilter only clean its own values. We don't use this filter anyway yet.

@sunix sunix merged commit ccd6878 into eclipse-che:master Feb 7, 2018
@benoitf benoitf added this to the 6.1.0 milestone Feb 7, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants