Added ScopeListener interface to the ThreadLocalScopeManager

[mdvorak]

• Added new interface ScopeListener
• Modified ThreadLocalScopeManager
• Added NoopScopeListener to be used as default
• Modified tests

Is there anything else missing?

Also, I've created signature of onActivate, thaking directly span:

void onActivate(Span span);

Because consumers should not actually need the scope, only its span - but it is a little weird, having Span in the interface called ScopeListener. What do you think? I have already prepared commit to change it to void onActivate(Scope scope);

[carlosalberto]

Because consumers should not actually need the scope, only its span - but it is a little weird, having Span in the interface called ScopeListener

I think this is fine, aligning with the recent changes on 0.32 around exposing active Span, not the Scope object itself.

[carlosalberto]

I'm wondering if supporting multiple listeners could be useful ;)

[yurishkuro]

Multiple listeners are already possible via composition.

[carlosalberto]

@yurishkuro sure, but was wondering if we should support this out-of-the-box ;) (not an important thing, just curious about the possibility)

[yurishkuro]

We can always add it later, it's a utility class that would dispatch to 2+ underlying listeners. Nobody asked for it yet.

[carlosalberto]

@yurishkuro Fair enough, sounds great to me ;)

[thegreystone]

So, with the ScopeListener, I would need to track the association between an onActivate and a close of a scope myself, most likely with a thread local. For example, if I start a JFR event in a scope, I would have to add it in a thread local, to close the right one once close is hit.

Brave, for example, uses the concept of span and scope decorators. Upon activation, the scope decorator would expect a new scope to be returned (see ScopeDecorator#decorateScope(...)), into which the event could be pushed and closed when the scope itself is closed. No thread locals required.

[sjoerdtalsma]

@thegreystone I like the decorator idea and I think it has come up before.
However, as I understand it, each tracer can currently assume that all spans are 'their own'. How do you propose to allow each tracer operate on decorated spans, expose an undecorate or unwrap method of sorts? Could it be an idea work this out in a separate PR?

[mdvorak]

Moving discussion to bottom.

We've identified 2 requirements:

1. Perform action with span at activation and deactivation, while providing said scope in an argument, for easy handling, as needed in census-instrumentation/opencensus-specs#247
2. Ability to handle change of span (both activation and deactivation of nested scope) in single operation, for MDC update (avoid unnecessary modifications) - this might apply to other handlers as well

Only possible solution I can think of is a bit ugly but universal

interface ScopeListener {
  void onSpanChanged(@Nullable Span activeSpan, @Nullable Span deactivatedSpan);
}

I can't think of use case, where I would need handler before action. If so, you can always replace whole ScopeManager with your own.. Which should still be normal option for anyone requesting special behavior. I just wanted simple way to add configuring MDC for logging, without need to copy-paste scope manager into every app.

[yurishkuro]

@mdvorak I don't think we have an agreement on (2) to have a single operation. Using lambda or 2-method class is, imo, a very minor thing compared to the cleanliness and intuitiveness of the API.

[mdvorak]

@mdvorak I don't think we have an agreement on (2) to have a single operation. Using lambda or 2-method class is, imo, a very minor thing compared to the cleanliness and intuitiveness of the API.

Sorry I mentioned lambda originally, but read carefully previous post. Problem for two-method handler is an implementation, where deactivation always called before activation creates performance (or other) problems. You force implementation to have two separate handlers, when it actually needs to handle both at one step.

[tylerbenson]

Here's a suggestion... Have the onSpanChanged method be the primary interface, but include an adapter that converts it to the style that @yurishkuro wants. Given the constraints proposed by @mdvorak I don't think it could go the other way.

What does the prior art look like? How does this compare with Brave/Census/etc?

[felixbarny]

What about adding an argument for the previously active/now active span.

• void beforeActivate(Span spanToActivate, @Nullable Span previouslyActive);
• void afterDeactivate(Span deactivatedSpan, @Nullable Span nowActive);

@thegreystone This should eliminate the need for maintaining a separate ThreadLocal, right? The advantage compared to Scope wrappers is that this does not allocate additional memory when creating scopes.

@mdvorak would that also solve your MDC use case? You will only have to remove the MDC context if nowActive is null (when you deactivate the first span of that thread).

[mdvorak]

@felixbarny Yes, it would be possible to create efficient handler, but I still find it very complicated to do very simple thing. Also, relying on behavior, where that when afterDeactivate is called with nowActive!=null can be ignored, and I can expect beforeActivate right away seems error prone. From API point of view, I would be relying on specific implementation behavior.

as for 1) I already proposed to provide both previous and next spans, so I agree with you on first point.

[felixbarny]

Also, relying on behavior, where that when afterDeactivate is called with nowActive!=null can be ignored

How is this different when having a single method?

From API point of view, I would be relying on specific implementation behavior.

Not sure what is implementation specific here - the behavior is clearly specified and does not depend on the implementation.

[thegreystone]

@thegreystone This should eliminate the need for maintaining a separate ThreadLocal, right? The advantage compared to Scope wrappers is that this does not allocate additional memory when creating scopes.

Not sure how this would help. I would still need to propagate the event from beforeActivate to afterDeactivate somehow, and the only way I would know how is to use a thread local (in the scope case; in the SpanListener case all bets are off).

[thegreystone]

@thegreystone I like the decorator idea and I think it has come up before. However, as I understand it, each tracer can currently assume that all spans are 'their own'. How do you propose to allow each tracer operate on decorated spans, expose an undecorate or unwrap method of sorts?

True, doing this in a general fashion requires a bit of extra consideration. Brave simply returns implementations of their own scope interface, and assume they are the only player in town.

Could it be an idea work this out in a separate PR?

If we have decorators, we probably do not need listeners. And if we have listeners, it will seem a bit excessive to have decorators too. It may be worth it to consider the options. Anyone from Brave involved in OpenTracing?

[thegreystone]

Here are some Brave decorators: https://github.com/openzipkin/brave/tree/master/context

The JFR use case is the closest to my heart.

[mdvorak]

Scope decorators are great, and that was my first idea, except with current implementation it does not work reliably, because of this if:

public class ThreadLocalScope implements Scope {
    @Override
    public void close() {
        if (scopeManager.tlsScope.get() != this) {
            // This shouldn't happen if users call methods in the expected order. Bail out.
            return;
        }

decorator can never know, whether scope was actually closed or not.

[carlosalberto]

@mdvorak Besides what others already commented/asked, I suggest you cooking a small case under testbed -if you have enough cycles- to show why you need a specific event design (and why the simple event @yurishkuro has in mind does not suit your needs). I know you posted a small explanation regarding MDC, but having actual code could throw additional light into it ;)

[sjoerdtalsma]

decorator can never know, whether scope was actually closed or not

Don't let that influence the listener vs. decorator discussion please. If that's a 'defect' of the current scope manager implementation, we could address that separately from the decorator issue. We could update scope manager to handle out-of-order closing in a predictable manner for example.

[felixbarny]

@thegreystone This should eliminate the need for maintaining a separate ThreadLocal, right? The advantage compared to Scope wrappers is that this does not allocate additional memory when creating scopes.

Not sure how this would help. I would still need to propagate the event from beforeActivate to afterDeactivate somehow, and the only way I would know how is to use a thread local (in the scope case; in the SpanListener case all bets are off).

I misunderstood what the problem is. Looking at brave's JfrScopeDecorator I understand now. You have to have a reference to the Event instance which has been created on activation when the deactivation happens.

I see two designs to achieve that:

Brave-like scope wrappers

• Pros:
  • Prooven design
  • Simplicity
• Cons:
  • Additional allocations for every scope, proportional to the number of scope wrappers
  • The type of the scope changes, so we will likely need a unwrap method

Context object

Introducing a span context object which allows to attach custom key/value pairs to it. You can add custom context objects in the before callback and retrieve them from the map in the after callback.

ScopeContext

• void add(String key, Object value) (lazily allocates the underlying map)
• <T> T get(String key)

ScopeListener

• void beforeActivate(Span spanToActivate, ScopeContext context);

• void afterDeactivate(Span deactivatedSpan, ScopeContext context);

• Pros

  • The type of the scope does not change as it is not wrapped
  • Even though we have to allocate ScopeContext objects, we only have to allocate once, no matter how many ScopeListeners are registered

• Cons

  • Allocation of the ScopeContext object for each scope event
  • Adding things to a map causes allocations (the Map itself and Map.Entry objects)
  • More complicated to work with

Example

public class ExampleScopeListener implements ScopeListener {
    @Override
    public void beforeActivate(Span spanToActivate, ScopeContext context) {
        context.add("foo", "bar");
    }

    @Override
    public void afterDeactivate(Span deactivatedSpan, ScopeContext context) {
        String foo = context.get("foo");
    }
}

[carlosalberto]

Introducing a span context object which allows to attach custom key/value pairs to it. You can add custom context objects in the before callback and retrieve them from the map in the after callback.

Btw, ScopeContext has been discussed a little bit in the past, so it's probably a good time to re-take it.

[felixbarny]

Can you link to that discussion or summarize some of the discussed use cases?

[carlosalberto]

Hey @felixbarny

This is the Issue: https://github.com/opentracing/specification/issues/114

[dschulten]

Since this has been dormant for a while, I wanted to mention that there is an opentracing-contrib module which can do sth quite similar: https://github.com/opentracing-contrib/java-api-extensions

[whiskeysierra]

Not really. You can't observe Scopes with that.

On Tue, 24 Dec 2019, 08:52 Dietrich Schulten, notifications@github.com wrote:

Since this has been dormant for a while, I wanted to mention that there is an opentracing-contrib module which can do sth quite similar: https://github.com/opentracing-contrib/java-api-extensions

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-java/pull/336?email_source=notifications&email_token=AADI7HO4VLGIIUKFOWASGQTQ2G5TXA5CNFSM4G6X6CB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHSXDIA#issuecomment-568684960, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADI7HJ7OVCG6KD7QJJ4AWTQ2G5TXANCNFSM4G6X6CBQ .

[dschulten]

Not really. You can't observe Scopes with that.

That is true, you observe when a Span starts and finishes, and you can track when the root span finishes (e.g. when traceId==spanId). Which is why I said it is similar. At least one can cover the MDC use case with it.

Out of curiosity: What is the compelling reason why one needs to observe a ThreadLocalScope?

[mdvorak]

I believe I discussed that somewhere - Span is completely different from Scope: Scope is technical, Span is logical One span can "span" across different threads, especially in reactive app. Scope is activated and deactivated on every thread change, since it is thread-based. So, when you want to observe tracing output, Span is your thing. If you need to extend tracing library, you might need Scopes.