Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Added ScopeListener interface to the ThreadLocalScopeManager #336

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2016-2019 The OpenTracing Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package io.opentracing.util;

import io.opentracing.Span;

public interface NoopScopeListener extends ScopeListener {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

NoopScopeListener INSTANCE = new NoopScopeListenerImpl();
}

/**
* A noop (i.e., cheap-as-possible) implementation of a ScopeListener.
*/
class NoopScopeListenerImpl implements NoopScopeListener {

@Override
public void onActivated(Span span) {
}

@Override
public void onClosed() {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright 2016-2019 The OpenTracing Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package io.opentracing.util;

import io.opentracing.Scope;
import io.opentracing.ScopeManager;
import io.opentracing.Span;

/**
* Listener that can react on changes of currently active {@link Span}.
* <p>
* The {@link #onActivated} method will be called, whenever scope changes - that can be both
* as result of a {@link ScopeManager#activate(Span, boolean)} call or when {@link Scope#close()}
* is closed on a nested scope.
* <p>
* {@link #onClosed} is called when closing outermost scope - meaning no scope is currently active.
*
* @see ThreadLocalScopeManager
*/
public interface ScopeListener {

/**
* Called whenever a scope was activated (changed).
*
* @param span Activated span. Never null.
*/
void onActivated(Span span);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call


/**
* Called when outermost scope was deactivated.
*/
void onClosed();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public class ThreadLocalScope implements Scope {
this.finishOnClose = finishOnClose;
this.toRestore = scopeManager.tlsScope.get();
scopeManager.tlsScope.set(this);
scopeManager.listener.onActivated(wrapped);
}

@Override
Expand All @@ -48,7 +49,13 @@ public void close() {
wrapped.finish();
}

scopeManager.tlsScope.set(toRestore);
if (toRestore != null) {
scopeManager.tlsScope.set(toRestore);
scopeManager.listener.onActivated(toRestore.wrapped);
} else {
scopeManager.tlsScope.remove();
scopeManager.listener.onClosed();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@mdvorak mdvorak Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. onActivated(1) - set MDC (copy internal map)
  2. onClose(1) - delete MDC (copy internal map) - useless
  3. onActivated(2) - set MDC (copy internal map)
  4. onClose(2) - delete MDC (copy internal map) - useless
  5. onActivated(1) - set MDC (copy internal map)
  6. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@mdvorak mdvorak Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,33 @@

/**
* A simple {@link ScopeManager} implementation built on top of Java's thread-local storage primitive.
* <p>
* Optionally supports {@link ScopeListener}, to perform additional actions when scope is changed for given thread.
* Listener methods are always called synchronously on the same thread, right after activation (meaning {@link #active()}
* already returns new a scope).
*
* @see ThreadLocalScope
*/
public class ThreadLocalScopeManager implements ScopeManager {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Default constructor for {@link ThreadLocalScopeManager}, without any listener.
*/
public ThreadLocalScopeManager() {
this(null);
}

/**
* Constructs {@link ThreadLocalScopeManager} with custom {@link ScopeListener}.
*
* @param listener Listener to register. When null, noop listener will be used.
*/
public ThreadLocalScopeManager(ScopeListener listener) {
this.listener = listener != null ? listener : NoopScopeListener.INSTANCE;
}

@Override
public Scope activate(Span span, boolean finishOnClose) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@

public class ThreadLocalScopeTest {
private ThreadLocalScopeManager scopeManager;
private ScopeListener scopeListener;

@Before
public void before() throws Exception {
scopeManager = new ThreadLocalScopeManager();
scopeListener = mock(ScopeListener.class);
scopeManager = new ThreadLocalScopeManager(scopeListener);
}

@Test
Expand Down Expand Up @@ -63,6 +65,11 @@ public void implicitSpanStack() throws Exception {
verify(backgroundSpan, times(1)).finish();
verify(foregroundSpan, times(1)).finish();

// Verify listener calls
verify(scopeListener, times(2)).onActivated(backgroundSpan);
verify(scopeListener, times(1)).onActivated(foregroundSpan);
verify(scopeListener, times(1)).onClosed();

// And now nothing is active.
Scope missingSpan = scopeManager.active();
assertNull(missingSpan);
Expand All @@ -71,11 +78,16 @@ public void implicitSpanStack() throws Exception {
@Test
public void testDeactivateWhenDifferentSpanIsActive() {
Span span = mock(Span.class);
Span nestedSpan = mock(Span.class);

Scope active = scopeManager.activate(span, false);
scopeManager.activate(mock(Span.class), false);
scopeManager.activate(nestedSpan, false);
active.close();

verify(span, times(0)).finish();

verify(scopeListener, times(1)).onActivated(span);
verify(scopeListener, times(1)).onActivated(nestedSpan);
verify(scopeListener, times(0)).onClosed();
}
}