ThreadLocalScopeManager should be extensible

[mdvorak]

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"));
    }
}

[mdvorak]

Also relates to https://github.com/opentracing-contrib/java-spring-cloud/issues/92

[sjoerdtalsma]

FYI, I have identified a need for a similar concept in my context-propagation library: https://github.com/talsma-ict/context-propagation/blob/develop/context-propagation-java5/src/main/java/nl/talsmasoftware/context/observer/ContextObserver.java

[tylerbenson]

I strongly favor the listener option, but I think there should be a onActivate/onDeactivate or onStart/onStop (or some other matching terminology) pair.

[sjoerdtalsma]

I strongly favor the listener option, but I think there should be a onActivate/onDeactivate or onStart/onStop (or some other matching terminology) pair.

Scope: onActivate / onDeactivate ? Span: onStart / onFinish ?

[carlosalberto]

I'd also vote to add an event listener layer, as it feels to me as the cleanest option ;)

[mdvorak]

@tylerbenson I was thinking about that, problem is, that as long as its single method interface, its easily usable as lambda :) onActivate/onDeactivate is probably cleanest. But again, for primary use-case, it is actually harmful to handle onDeactivate, when scope changes (e.g. for child span), since all MDC/ThreadContext internal structures are immutable, and any change causes clone of the internal HashMap. And for such listener, you cannot distinguish, whether you are leaving outer scope, or just changing it. And calling onDeactivate for only outer scope seems wrong to me. So, single callback looks most versatile, while not the cleanest.

@sjoerdtalsma This is about scopes only. Lifecycle of spans is far more complex and outside power of the ScopeManager. Span activation in the scope does not mean its start. And deactivation does not have to be its end. If you want span lifecycle observe, you will have to open another issue :)

[sjoerdtalsma]

Lifecycle of spans is far more complex and outside power of the ScopeManager.

Agree, I think is misinterpreted the comment from @carlosalberto 😉..

@mdvorak Have you looked at the interface I use for observing? It passes both the activated / deactivated value and the previous / reactivated value. Maybe that could be of use?

🤷‍♂️ shameless plug:

BTW I intent to release a version soon with the above PR in my library. Then you can use my ScopeManager implementation and register a ContextObserver SPI for it.
As an added benefit you could also add mdc-propagation to your classpath and have that automatically propagate into background threads using the same context aware utility classes.

[mdvorak]

@sjoerdtalsma Well, I think your implementation is a bit too complex for the goal I wanted to achieve. I was more thinking about something like this: https://github.com/mdvorak/opentracing-java/commit/734ad19a435798ff82d24b640caf24609d3f17c2 If we had Java8 minimum, it would be even shorter.

Also, from Span.finish() docs:

Future calls to {@link #finish} are defined as noops, and future calls to methods other than {@link #context} lead to undefined behavior.

So separate handling, which avoid calling finish twice, should not be necessary. But you have nice, documented and tested code ;)

[tylerbenson]

@mdvorak interesting insight regarding:

and any change causes clone of the internal HashMap

I was not aware of that. That makes sense then why you'd want to reduce thrashing and limit it to just the changes. I assume in the case where the root scope is being closed, you'd pass in null to indicate such?

[yurishkuro]

My preference is for symmetric onActivate/onClose. The latter can be used, for example, to track how much time the span was "runnable" on a given thread.

[mdvorak]

My preference is for symmetric onActivate/onClose. The latter can be used, for example, to track how much time the span was "runnable" on a given thread.

That is not actually a bad idea, dunno why it did not occur to me. This aproach does not enforce unnecessary MDC modifications, yet it provides strong contract. Passing null on deactivating is a bit ugly. But still great for simple lambda handler :) I'll think about how to best structure it, and cook up a PR.

[mdvorak]

@yurishkuro What about AutoFinishScopeManager? Should this change be added only to ThreadLocalScopeManager, or do you prefer both?

[yurishkuro]

AutoFinishScopeManager

Isn't it deprecated?

[mdvorak]

AutoFinishScopeManager

Isn't it deprecated?

Not in code, neither in master or v0.32.0: https://github.com/opentracing/opentracing-java/blob/master/opentracing-util/src/main/java/io/opentracing/util/AutoFinishScopeManager.java#L21

[yurishkuro]

@carlosalberto do we still need AutoFinishScopeManager.java ?

[carlosalberto]

@yurishkuro It was added as an experimental/esoteric feature, but given the latest changes, guess we can deprecate it (and if any continuation-based framework like Akka needs it, they can either have their own).

Long story short: we should deprecate it, yes.