Adds support for AutoFinishScope #12
Conversation
|
|
||
| public TracedCallable(Callable<V> delegate, Tracer tracer) { | ||
| this.delegate = delegate; | ||
| this.tracer = tracer; | ||
| this.span = tracer.activeSpan(); | ||
|
|
||
| if (tracer.scopeManager() instanceof AutoFinishScopeManager && tracer.scopeManager().active() != null) |
There was a problem hiding this comment.
Could you add braces in the if/else constructs here and following classes.
|
|
||
| public TracedRunnable(Runnable delegate, Tracer tracer) { | ||
| this.delegate = delegate; | ||
| this.tracer = tracer; | ||
| this.span = tracer.activeSpan(); | ||
|
|
||
| if (tracer.scopeManager() instanceof AutoFinishScopeManager && tracer.scopeManager().active() != null) |
There was a problem hiding this comment.
Indentation problem with this if/else construct.
| } | ||
|
|
||
| @Override | ||
| public void run() { | ||
| Scope scope = span == null ? null : tracer.scopeManager().activate(span, false); | ||
| try { | ||
| Scope scope = null; |
There was a problem hiding this comment.
Indentation problem with this code block.
| import org.junit.Before; | ||
| import org.junit.Test; | ||
|
|
||
| import java.util.concurrent.*; |
There was a problem hiding this comment.
Can you expand the imports.
| /** | ||
| * @author Jose Montoya | ||
| */ | ||
| public class TracedAutoFinishTest { |
There was a problem hiding this comment.
You should derive from the AbstractConcurrentTest and make use TestRunnable/Callable and latch mechanism, rather than use awaitility.
There was a problem hiding this comment.
Was going to, but I needed a Thread.sleep within the runnable/callable to make sure that I could check the current thread's span and scope were close before the runnable returned. I didn't want to modify the existing TestRunnable, even though the original tests should still pass. If you're OK with me making those changes then I'll go ahead and do that.
There was a problem hiding this comment.
You could introduce a semaphore in the abstract test runnable/callable impls to allow the test to block their execution.
|
Since your You will have to take care of some gotcha's to make this work:
|
| <maven.compiler.source>1.6</maven.compiler.source> | ||
| <maven.compiler.target>1.6</maven.compiler.target> | ||
| <maven.compiler.source>1.7</maven.compiler.source> | ||
| <maven.compiler.target>1.7</maven.compiler.target> |
There was a problem hiding this comment.
Why the java version bump?
There was a problem hiding this comment.
Try with resources in a test. I suppose that does not merit the change. I'll revert these.
| @@ -99,7 +106,6 @@ | |||
| <dependency> | |||
| <groupId>io.opentracing</groupId> | |||
| <artifactId>opentracing-util</artifactId> | |||
| <scope>test</scope> | |||
There was a problem hiding this comment.
Please make this provided and optional=true to avoid unnecessary transitive dependencies
There was a problem hiding this comment.
It would result in java.lang.NoClassDefFoundError.
There was a problem hiding this comment.
It would result in
java.lang.NoClassDefFoundError.
Currently, yes.
However, as I explained in #12 (comment), the util usage is entirely optional (based on an instanceof check, which can only happen if the application already added the util lib to the classpath).
Therefore, catching the NoClassDefFoundError and interpreting it as "probably isn't an instance of AutoFinishScopeManager" is valid logic in my opinion.
Libraries should not declare force transitive dependencies if they're optional.
There was a problem hiding this comment.
That can be quite expensive to have it in runnable/callable.
There was a problem hiding this comment.
if (tracer.scopeManager() instanceof AutoFinishScopeManager && tracer.scopeManager().active() != null)
cont = ((AutoFinishScope) tracer.scopeManager().active()).capture();
else
cont = null;Could be replaced by:
if (hasAutoFinishScopeManager(tracer) && tracer.scopeManager().active() != null) {
cont = ((io.opentracing.util.AutoFinishScope) tracer.scopeManager().active()).capture();
}
...
private static boolean hasAutoFinishScopeManager(Tracer tracer) {
try {
return tracer.scopeManager() instanceof io.opentracing.util.AutoFinishScopeManager;
} catch (NoClassDefFoundError utilityJarNotLoaded) {
LOGGER.finest("The opentracing-util library is not available, therefore the ScopeManager cannot be the AutoFinishScopeManager.");
return false;
}
}Just make sure not to import io.opentrating.util classes..
I agree this borders on being too far-fetched, just pointing out the possibilities.
There was a problem hiding this comment.
A better way would be to wrap all auto-finish related coded in its own (package protected) class and only access that if the hasAutoFinish.. test returned true.
There was a problem hiding this comment.
That can be quite expensive to have it in runnable/callable.
Ok, then create a boolean constant and populate it in a static code block when the class is loaded..
For inspiration, you could look at PriorityComparator (optional use of javax.annotation.Priority) or TracerResolver (optional use of io.opentracing.util.GlobalTracer)
There was a problem hiding this comment.
If we have consensus on the last proposal, static boolean. Then I don't mind adding that.
|
What's the status of this PR? |
Adds support for AutoFinishScope by checking the type of ScopeManager and using a AutoFinishScope.Continuation when appropriate. Includes some tests.