Added ScopeListener interface to the ThreadLocalScopeManager #336

mdvorak · 2019-03-15T08:54:54Z

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

…ager

yurishkuro · 2019-03-15T13:54:56Z

opentracing-util/src/main/java/io/opentracing/util/ScopeListener.java

+    /**
+     * Called when outermost scope was deactivated.
+     */
+    void onClose();


Why not pass the span here too?

Then it would make sense to call onActivate with previous span as well (optionally null). I'd think it is better to keep the interface as simple as possible. But its no problem to implement it of course :)

I am not sure why previous span is relevant - if we're on an executor thread that span may be completely unrelated to the one being activated.

Yes, but previous span is the one being deactivated - same situation as onClose. So, if you don't need deactivated span in onActivate, you should not need it in onClose either.

yurishkuro · 2019-03-15T13:58:21Z

opentracing-util/src/main/java/io/opentracing/util/ThreadLocalScope.java

+            scopeManager.listener.onActivate(toRestore.wrapped);
+        } else {
+            scopeManager.tlsScope.remove();
+            scopeManager.listener.onClose();


Should be done earlier imo

Are you sure? Listener code will then execute in the context of the previous span, not the one passed in as argument. Same applies to onClose.

again, the "previous span" is not really relevant. This function closes the current scope, so it should invoke listener.close(currentScope.span). Then it can do the housekeeping of the stack and re-activation of the previous span.

Ok, maybe we misunderstood each other. Were you talking about onClose or onActivate?
onClose can be called before scopeManager.tlsScope.remove(); and it makes sense. I'll update that.

Calling onActivate before scopeManager.tlsScope.set(toRestore); does not make sense - onActivate would run still with active previous span.

On the second thought, what about renaming methods to onActivated and onClosed, and really call them both after change has happened? After all, this is listener to change, not interceptor. I would naturally expect, that state transition has already happened, when I'm receiving the call.

carlosalberto · 2019-03-15T17:55:02Z

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 · 2019-03-15T17:55:51Z

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

yurishkuro · 2019-03-15T18:17:45Z

Multiple listeners are already possible via composition.

yurishkuro · 2019-03-15T18:18:21Z

opentracing-util/src/main/java/io/opentracing/util/ScopeListener.java

+    /**
+     * Called when outermost scope was deactivated.
+     */
+    void onClosed();


I still think it needs to accept the span represented by the scope being closed.

yurishkuro · 2019-03-15T18:19:17Z

opentracing-util/src/main/java/io/opentracing/util/ThreadLocalScope.java

+            scopeManager.listener.onActivated(toRestore.wrapped);
+        } else {
+            scopeManager.tlsScope.remove();
+            scopeManager.listener.onClosed();


the previous comments got lost - the onClosed must be called regardless of the previous span presence.

And thats exactly what i don't want to do, see discussion in #334.
Most common use-case is setting MDC (or ThreadContext for log4j) - and that uses copy-on-write immutable maps.
So, having to do following sequence:

onActivated(1) - set MDC (copy internal map)

onClose(1) - delete MDC (copy internal map) - useless

onActivated(2) - set MDC (copy internal map)

onClose(2) - delete MDC (copy internal map) - useless

onActivated(1) - set MDC (copy internal map)

onClose(1) - delete MDC (copy internal map) - needed

wastes resources. Consumer cannot know, whether onActivated is coming right after onClose.
Thats why originally wanted only protected method for simple extension, or single method interface, so consumer can decide, what is best. This API should be simplistic, if you need something complex, you can copy whole class.

BTW thats why I originally proposed single method interface, with possible null as argument - its ugly, but efficient and versatile.

You're making assumption that MDC is the only use case. If you don't want to clear MDC from onClosed(), you don't have to, it does not negate the usefulness of the callback for other use cases. For example, see census-instrumentation/opencensus-specs#247

You have to clear it in onClosed(), because you can't leave it set on last deactivation (updated example above, sorry I left it out, see its 6) step).
Not saying its only use case, but its use case that should be supported. Give users freedom to do what they need, don't force them to follow exact and strict pattern.

Only model, that would actually fulfill both use-cases IMO, is ugly but flexible:

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

Or, I would revisit protected method and simply inheritance - do everyone whatever they want.

class ThreadLocalScopeManager { protected void setThreadScope(@Nullable ThreadLocalScope scope) { this.tlsScope.set(scope); } }

this can serve as "around advice". And it would make the class extensible in many ways, which I don't think is a bad thing.

Moved discussion to bottom, it will get lost here on another push.

carlosalberto · 2019-03-15T18:40:20Z

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

yurishkuro · 2019-03-15T19:23:46Z

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

carlosalberto · 2019-03-15T20:23:29Z

@yurishkuro Fair enough, sounds great to me ;)

thegreystone · 2019-03-17T23:04:45Z

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 · 2019-03-18T07:16:09Z

@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?

felixbarny · 2019-03-18T09:33:18Z

opentracing-util/src/main/java/io/opentracing/util/ScopeListener.java

+     *
+     * @param span Activated span. Never null.
+     */
+    void onActivated(Span span);


I think the method name should reflect whether the activation happens before or after invoking this method. I'd propose beforeActivated and afterClosed

I agree on this. It's clearer having a before or after prefix.

felixbarny · 2019-03-18T09:35:55Z

opentracing-util/src/main/java/io/opentracing/util/ThreadLocalScopeManager.java

    final ThreadLocal<ThreadLocalScope> tlsScope = new ThreadLocal<ThreadLocalScope>();
+    final ScopeListener listener;


Consider making it a list or adding a CompoundScopeListener, which maintains a list of ScopeListeners

composite listener can be another PR, imo, if someone actually needs it.

mdvorak · 2019-03-18T09:55:33Z

Moving discussion to bottom.

We've identified 2 requirements:

Perform action with span at activation and deactivation, while providing said scope in an argument, for easy handling, as needed in Extend span APIs to support callbacks and access to spancontext census-instrumentation/opencensus-specs#247
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.

carlosalberto · 2019-03-18T15:09:18Z

opentracing-util/src/main/java/io/opentracing/util/NoopScopeListener.java

+
+import io.opentracing.Span;
+
+public interface NoopScopeListener extends ScopeListener {


I'm wondering if we really need to expose this one as public.

yurishkuro · 2019-03-18T15:20:12Z

@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 · 2019-03-18T15:23:42Z

@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 · 2019-03-18T15:58:34Z

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 · 2019-03-18T16:14:24Z

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 · 2019-03-18T16:30:12Z

@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 · 2019-03-18T16:52:03Z

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 · 2019-03-18T18:50:10Z

@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 · 2019-03-18T19:00:03Z

@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 · 2019-03-18T19:06:05Z

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

The JFR use case is the closest to my heart.

mdvorak · 2019-03-18T20:59:17Z

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 · 2019-03-19T01:48:45Z

@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 · 2019-03-19T07:09:24Z

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 · 2019-03-19T13:47:40Z

@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 · 2019-03-19T14:44:27Z

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 · 2019-03-19T15:32:29Z

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

carlosalberto · 2019-03-20T15:38:20Z

Hey @felixbarny

This is the Issue: opentracing/specification#114

dschulten · 2019-12-24T07:52:25Z

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 · 2019-12-24T09:11:31Z

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

…

On Tue, 24 Dec 2019, 08:52 Dietrich Schulten, ***@***.***> 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 <#336?email_source=notifications&email_token=AADI7HO4VLGIIUKFOWASGQTQ2G5TXA5CNFSM4G6X6CB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHSXDIA#issuecomment-568684960>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADI7HJ7OVCG6KD7QJJ4AWTQ2G5TXANCNFSM4G6X6CBQ> .

dschulten · 2019-12-25T17:14:11Z

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 · 2019-12-25T18:34:55Z

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.

Michal Dvorak added 2 commits March 13, 2019 09:46

Fixes #334 - Add ScopeListener extension point to ThreadLocalScopeMan…

17b0dd0

…ager

Closes #334 - Tests and docs for ScopeListener

5ae192f

mdvorak changed the title ~~#334 - Added ScopeListener interface to the ThreadLocalScopeManager~~ Added ScopeListener interface to the ThreadLocalScopeManager Mar 15, 2019

mdvorak mentioned this pull request Mar 15, 2019

0.32.0 Release #314

Open

mdvorak marked this pull request as ready for review March 15, 2019 08:58

yurishkuro suggested changes Mar 15, 2019

View reviewed changes

Renamed ScopeListener methods to past tense

4fa85ad

yurishkuro reviewed Mar 15, 2019

View reviewed changes

felixbarny reviewed Mar 18, 2019

View reviewed changes

carlosalberto reviewed Mar 18, 2019

View reviewed changes

thegreystone mentioned this pull request Mar 18, 2019

Something like Brave span/scope decorators should be part of opentracing-java #324

Open

tylerbenson mentioned this pull request May 16, 2019

SDK: Lifecycle [Listener|Decorator|Interceptor] for [Scope|Span] open-telemetry/opentelemetry-java#295

Closed

		final ThreadLocal<ThreadLocalScope> tlsScope = new ThreadLocal<ThreadLocalScope>();
		final ScopeListener listener;


		import io.opentracing.Span;

		public interface NoopScopeListener extends ScopeListener {

Added ScopeListener interface to the ThreadLocalScopeManager #336

Are you sure you want to change the base?

Added ScopeListener interface to the ThreadLocalScopeManager #336

Conversation

mdvorak commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Mar 15, 2019

carlosalberto commented Mar 15, 2019

yurishkuro commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdvorak Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdvorak Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Mar 15, 2019

yurishkuro commented Mar 15, 2019

carlosalberto commented Mar 15, 2019

thegreystone commented Mar 17, 2019

sjoerdtalsma commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdvorak commented Mar 18, 2019

Choose a reason for hiding this comment

yurishkuro commented Mar 18, 2019

mdvorak commented Mar 18, 2019

tylerbenson commented Mar 18, 2019

felixbarny commented Mar 18, 2019

mdvorak commented Mar 18, 2019 • edited Loading

felixbarny commented Mar 18, 2019

thegreystone commented Mar 18, 2019

thegreystone commented Mar 18, 2019

thegreystone commented Mar 18, 2019

mdvorak commented Mar 18, 2019

carlosalberto commented Mar 19, 2019

sjoerdtalsma commented Mar 19, 2019

felixbarny commented Mar 19, 2019

carlosalberto commented Mar 19, 2019

felixbarny commented Mar 19, 2019

carlosalberto commented Mar 20, 2019

dschulten commented Dec 24, 2019

whiskeysierra commented Dec 24, 2019 via email

dschulten commented Dec 25, 2019 • edited Loading

mdvorak commented Dec 25, 2019

mdvorak Mar 15, 2019 •

edited

Loading

mdvorak Mar 15, 2019 •

edited

Loading

mdvorak commented Mar 18, 2019 •

edited

Loading

dschulten commented Dec 25, 2019 •

edited

Loading