-
Notifications
You must be signed in to change notification settings - Fork 232
Add a ActiveSpanSource backed TraceContext #266
Changes from 4 commits
67eb129
4f8fc78
8277a5b
55d04df
76048cb
67bf0f4
c986e7c
31a7b93
6544e8c
dc4572f
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,89 @@ | ||
/* | ||
* Copyright (c) 2017, Uber Technologies, Inc | ||
* | ||
* 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 com.uber.jaeger.context; | ||
|
||
import io.opentracing.ActiveSpan; | ||
import io.opentracing.ActiveSpanSource; | ||
import io.opentracing.Span; | ||
import io.opentracing.Tracer; | ||
import io.opentracing.util.GlobalTracer; | ||
import io.opentracing.util.ThreadLocalActiveSpan; | ||
|
||
import java.lang.reflect.Field; | ||
|
||
/** | ||
* This is a {@link TraceContext} that relies on the {@link ActiveSpanSource} registered with {@link | ||
* GlobalTracer}. | ||
*/ | ||
public class ActiveSpanSourceTraceContext implements TraceContext { | ||
|
||
/** | ||
* This is a hack to retrieve the span wrapped by the {@link ThreadLocalActiveSpan} implementation | ||
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. let's mention that this is a temporary hack until 0.31 |
||
* to shoehorn into the {@link TraceContext} implementation. This is being done so that | ||
* instrumentation relying on {@link Tracer} is consistent with instrumentation using {@link | ||
* TraceContext}. We expect to remove this when opentracing-api version 0.31 is released. | ||
*/ | ||
private static final Field wrappedSpan; | ||
|
||
static { | ||
try { | ||
wrappedSpan = ThreadLocalActiveSpan.class.getDeclaredField("wrapped"); | ||
wrappedSpan.setAccessible(true); | ||
} catch (NoSuchFieldException e) { | ||
throw new RuntimeException("Unable to access ThreadLocalActiveSpan.wrapped reflectively.", e); | ||
} | ||
} | ||
|
||
public ActiveSpanSourceTraceContext() { | ||
} | ||
|
||
/** Makes the span active. */ | ||
@Override | ||
public void push(Span span) { | ||
GlobalTracer.get().makeActive(span); | ||
} | ||
|
||
/** Deactivates the current active span. */ | ||
@Override | ||
public Span pop() { | ||
ActiveSpan activeSpan = GlobalTracer.get().activeSpan(); | ||
Span span = getSpan(activeSpan); | ||
activeSpan.deactivate(); | ||
return span; | ||
} | ||
|
||
/** Retrieves the current active span. */ | ||
@Override | ||
public Span getCurrentSpan() { | ||
ActiveSpan activeSpan = GlobalTracer.get().activeSpan(); | ||
return getSpan(activeSpan); | ||
} | ||
|
||
@Override | ||
public boolean isEmpty() { | ||
return GlobalTracer.get().activeSpan() == null; | ||
} | ||
|
||
private Span getSpan(ActiveSpan activeSpan) { | ||
Span span; | ||
try { | ||
span = (Span) wrappedSpan.get(activeSpan); | ||
} catch (IllegalAccessException e) { | ||
throw new RuntimeException(e); | ||
} | ||
|
||
return span; | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,18 +14,28 @@ | |
|
||
package com.uber.jaeger.context; | ||
|
||
import io.opentracing.util.GlobalTracer; | ||
|
||
import java.util.concurrent.ExecutorService; | ||
|
||
public class TracingUtils { | ||
private static final TraceContext traceContext = new ThreadLocalTraceContext(); | ||
private static TraceContext traceContext = new ActiveSpanSourceTraceContext(); | ||
|
||
public static TraceContext getTraceContext() { | ||
assertGlobalTracerIsRegistered(); | ||
return traceContext; | ||
} | ||
|
||
public static ExecutorService tracedExecutor(ExecutorService wrappedExecutorService) { | ||
assertGlobalTracerIsRegistered(); | ||
return new TracedExecutorService(wrappedExecutorService, traceContext); | ||
} | ||
|
||
private TracingUtils(){} | ||
private static void assertGlobalTracerIsRegistered() { | ||
if (!GlobalTracer.isRegistered()) { | ||
throw new IllegalStateException("Please register a io.opentracing.util.GlobalTracer."); | ||
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. @yurishkuro I felt it was simpler to use 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 it the GlobalTracer.INSTANCE as we discussed? By using GlobalTracer directly you're introducing tight coupling in the new class, which didn't need to happen. 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 type of |
||
} | ||
} | ||
|
||
private TracingUtils() {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Copyright (c) 2017, The Jaeger 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 com.uber.jaeger.utils; | ||
|
||
import io.opentracing.NoopTracerFactory; | ||
import io.opentracing.util.GlobalTracer; | ||
|
||
import java.lang.reflect.Field; | ||
|
||
public class TestUtils { | ||
public static void resetGlobalTracer() throws NoSuchFieldException, IllegalAccessException { | ||
// Reset opentracing's global tracer | ||
Field field = GlobalTracer.class.getDeclaredField("tracer"); | ||
field.setAccessible(true); | ||
field.set(null, NoopTracerFactory.create()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,10 +94,13 @@ public void filter( | |
// hitting this case means previous filter was not called | ||
return; | ||
} | ||
serverSpan = traceContext.pop(); | ||
serverSpan = traceContext.getCurrentSpan(); | ||
|
||
Tags.HTTP_STATUS.set(serverSpan, containerResponseContext.getStatus()); | ||
serverSpan.finish(); | ||
|
||
// We are relying on the ActiveSpanSource implementation to `close` the span, and | ||
// hence don't need to call `finish`. | ||
traceContext.pop(); | ||
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. please add comments explaining this order (i.e. that we cannot 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. is this the only place in the whole project where we 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. Will do. No, we call it in our wrapped Callable and Runnable |
||
} catch (Exception e) { | ||
log.error("Server Filter Response:", e); | ||
} | ||
|
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.
Are you sure we need a new class instead of changing ThreadLocalTraceContext? If someone is already using ThreadLocalTraceContext directly, then it won't work correctly any longer, so I think it warrants a breaking API change (namely having an argument to the constructor).
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.
We could also simply remove the
ThreadLocalTraceContext
instead of marking it@Deprecated
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.
why create extra churn with classes/filenames/history when we can reuse the same class and re-implement it as needed?
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.
Could you provide justification for sticking with the old name?
The new name is more descriptive.
Javac itself doesn't care about what this class used to be called.
As far as git goes, it uses binary blobs to track file content. I expect it to be able to handle this operation efficiently. Have you seen examples where it hasn't? Should we be optimizing for git history size?