-
Notifications
You must be signed in to change notification settings - Fork 341
ThreadLocalScopeManager should be extensible #334
Description
Adding custom behavior for activating and deactivating scope is real pain - one have to copy ThreadLocalScopeManager and ThreadLocalScope into project and modify them.
It is because while activate can be overriden by ScopeManager, the actual deactivation happens in ThreadLocalScope.close() and is conditional. Even wrapping ThreadLocalScope and hooking to close() won't work. Thus, copy-paste it is.
Main use-case here is setting MDC context. Even with 0.32 and exposed spanId and traceId, there is no straightforward way to set those to MDC (unless I'm oblivious to something). While setting always MDC is a little opinionated and can be part of more automated libraries like for Spring (as stated in #202), ot-java should provide extension point for both apps and libraries to do that with ease.
Now, I'll prepare PR, just pick a solution :)
1) Replace calls to ThreadLocalScopeManager.tlsScope.set() in ThreadLocalScope with protected method in ThreadLocalScopeManager:
protected void setThreadScope(ThreadLocalScope scope) {
this.tlsScope.set(scope);
}
This has least impact on current code, but exposes class internals. Question is, whether it is actually an issue.
2) Similar to 1), but add listener to ThreadLocalScopeManager instead of protected setter, to allow composition over inheritance:
public interface Listener {
void onActiveSpanChanged(Span span);
}
final Listener listener;
Whenever ThreadLocalScope calls scopeManager.tlsScope.set(), it also calls scopeManager.listener.onActiveSpanChanged.
3) Create some more sophisticated and complex observer system. Problem with above solutions is, that AutoFinishScopeManager could have exactly same logic. Is it worth adding complex structures to avoid little code duplication?
Also question is, what are actually plans with AutoFinishScopeManager, since it couples app code with specific configuration, and the implementation itself is error-prone IMO (leaking spans, not closing spans).
I prefer solution 2), as it allows very simple use in Java8+. Example for Jaeger and Log4j2:
new JaegerTracer.Builder("sample")
.withScopeManager(new ThreadLocalScopeManager(this::setThreadContext))
.build();
// ...
private void setThreadContext(Span span) {
if (span instanceof JaegerSpan) {
JaegerSpan jaegerSpan = (JaegerSpan) span;
ThreadContext.putAll(Map.of(
"traceId", jaegerSpan.context().getTraceId(),
"spanId", Long.toHexString(jaegerSpan.context().getSpanId())
));
} else {
ThreadContext.removeAll(asList("traceId", "spanId"));
}
}