-
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 all 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 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); | ||
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 think the method name should reflect whether the activation happens before or after invoking this method. I'd propose 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 agree on this. It's clearer having 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. good call |
||
|
||
/** | ||
* Called when outermost scope was deactivated. | ||
*/ | ||
void onClosed(); | ||
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 still think it needs to accept the span represented by the scope being closed. |
||
} |
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.onActivated(wrapped); | ||
} | ||
|
||
@Override | ||
|
@@ -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(); | ||
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. the previous comments got lost - the onClosed must be called regardless of the previous span presence. 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. And thats exactly what i don't want to do, see discussion in #334.
wastes resources. Consumer cannot know, whether onActivated is coming right after 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. BTW thats why I originally proposed single method interface, with possible null as argument - its ugly, but efficient and versatile. 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. 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 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. 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). Only model, that would actually fulfill both use-cases IMO, is ugly but flexible:
Or, I would revisit protected method and simply inheritance - do everyone whatever they want.
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. 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. Moved discussion to bottom, it will get lost here on another push. |
||
} | ||
} | ||
|
||
@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.