-
Notifications
You must be signed in to change notification settings - Fork 344
Added ScopeListener interface to the ThreadLocalScopeManager #336
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not pass the span here too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then it would make sense to call There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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 | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be done earlier imo There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? Calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the second thought, what about renaming methods to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sgtm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed |
||
} | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making it a list or adding a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
There was a problem hiding this comment.
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.