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 2 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 onActivate(Span span) {
}

@Override
public void onClose() {
}
}
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 #onActivate(Span)} 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 #onClose()} 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 onActivate(Span span);

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

Choose a reason for hiding this comment

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

Why not pass the span here too?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
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.onActivate(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.onActivate(toRestore.wrapped);
} else {
scopeManager.tlsScope.remove();
scopeManager.listener.onClose();
Copy link
Member

Choose a reason for hiding this comment

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

Should be done earlier imo

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

Copy link
Author

Choose a reason for hiding this comment

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

Renamed

}
}

@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)).onActivate(backgroundSpan);
verify(scopeListener, times(1)).onActivate(foregroundSpan);
verify(scopeListener, times(1)).onClose();

// 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)).onActivate(span);
verify(scopeListener, times(1)).onActivate(nestedSpan);
verify(scopeListener, times(0)).onClose();
}
}