Log correlation remake suggestions #4
Conversation
|
While |
There was a problem hiding this comment.
Thanks for proposing this.
If you don't mind, I will only pick some of the proposed changes, but I won't merge this as a whole.
The CorrelationIdMapAdapter is awesome, but it's very log4j-specific, and that's where I would put it. I'd definitely not treat it as the default implementation holding global constants or affecting the abstract implementation this way.
As we discussed, I will apply the framework-specific instrumentation points, but my intent was to only change the instrumentations and see the tests pass, not to do all these adjustments.
| if (callDepth.isNestedCallAndIncrement()) { | ||
| // avoid re-entrancy and stack overflow errors | ||
| // may happen when bootstrapping an instrumentation that also gets triggered during the bootstrap | ||
| // for example, adding correlation ids to the thread context when executing logger.debug | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Is that an issue related to us instrumenting our own logging?
If that so, the proper handling is to avoid instrumenting it. I did it through getClassLoaderMatcher.
I am not opposed to this safety check, but it should be treated as such - log an error before returning null.
There was a problem hiding this comment.
IMHO, we'll also want to instrument our logging so that we also have correlation IDs in the agent logs.
There was a problem hiding this comment.
Having correlation IDs within the agent logs is a great idea, but I think the proper way to achieve that is through direct addition to the MDC. We can use an ActivationListener for that.
Instrumenting classes loaded by the agent class loader can open a can of worms. It would create a plugin CL for it and treat the agent CL as the target CL. This deviates from anything our complicated class loading hierarchy is designed for, so I really think we should avoid that.
++
Right, the constants are better off in a different class. |
Implements suggestions from elastic#2428 (comment)