Add a ActiveSpanSource backed TraceContext #266

vprithvi · 2017-10-09T19:50:17Z

use ActiveSpanSource for propagation instead of ThreadLocalTraceContext

Signed-off-by: Prithvi Raj [email protected]

- use `ActiveSpanSource` for propagation instead of `ThreadLocalTraceContext` Signed-off-by: Prithvi Raj <[email protected]>

codecov · 2017-10-09T19:57:17Z

Codecov Report

Merging #266 into master will decrease coverage by <.01%.
The diff coverage is 84.37%.

@@             Coverage Diff              @@
##             master     #266      +/-   ##
============================================
- Coverage      82.3%   82.29%   -0.01%     
- Complexity      530      535       +5     
============================================
  Files            87       87              
  Lines          2006     2022      +16     
  Branches        237      237              
============================================
+ Hits           1651     1664      +13     
- Misses          258      260       +2     
- Partials         97       98       +1

Impacted Files	Coverage Δ	Complexity Δ
...a/com/uber/jaeger/filters/jaxrs2/ServerFilter.java	`95% <100%> (ø)`	`7 <0> (ø)`	⬇️
...r/jaeger/context/ActiveSpanSourceTraceContext.java	`82.6% <82.6%> (ø)`	`8 <8> (?)`
...ain/java/com/uber/jaeger/context/TracingUtils.java	`88.88% <85.71%> (+38.88%)`	`5 <5> (+3)`	⬆️
...jaeger/reporters/protocols/ThriftUdpTransport.java	`77.96% <0%> (-5.09%)`	`14% <0%> (-2%)`
...ger-core/src/main/java/com/uber/jaeger/Tracer.java	`86.84% <0%> (+0.43%)`	`19% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37bd37f...dc4572f. Read the comment docs.

yurishkuro · 2017-10-09T20:17:12Z

jaeger-context/src/main/java/com/uber/jaeger/context/ActiveSpanSourceTraceContext.java

+  private Span getSpan(ActiveSpan activeSpan) {
+    Span wrappedSpan;
+    try {
+      wrappedSpan = (Span) ActiveSpanSourceTraceContext.wrappedSpan.get(activeSpan);


unnecessary qualifier ActiveSpanSourceTraceContext.

yurishkuro · 2017-10-09T20:17:50Z

jaeger-context/src/main/java/com/uber/jaeger/context/ActiveSpanSourceTraceContext.java

+  /**
+   * This is a hack to retrieve the span wrapped by the {@link ThreadLocalActiveSpan} implementation
+   * to shoehorn into the {@link TraceContext} implementation. This is being done so that
+   * instrumentation relying on {@link Tracer} has a is consistent with instrumentation using


typo: "has a is"

yurishkuro · 2017-10-09T20:18:09Z

jaeger-context/src/main/java/com/uber/jaeger/context/ActiveSpanSourceTraceContext.java

+
+  private final Tracer tracer;
+  /**
+   * This is a hack to retrieve the span wrapped by the {@link ThreadLocalActiveSpan} implementation


let's mention that this is a temporary hack until 0.31

yurishkuro · 2017-10-09T20:19:42Z

jaeger-context/src/main/java/com/uber/jaeger/context/ActiveSpanSourceTraceContext.java

+ * This is a {@link TraceContext} that relies on the {@link ActiveSpanSource} registered with {@link
+ * GlobalTracer}.
+ */
+public class ActiveSpanSourceTraceContext implements TraceContext {


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

We could also simply remove the ThreadLocalTraceContext instead of marking it @Deprecated

why create extra churn with classes/filenames/history when we can reuse the same class and re-implement it as needed?

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?

yurishkuro · 2017-10-09T20:21:15Z

jaeger-context/src/main/java/com/uber/jaeger/context/ActiveSpanSourceTraceContext.java

+    }
+  }
+
+  public ActiveSpanSourceTraceContext(Tracer tracer) {


I think you can use a more narrow dependency, ActiveSpanSource instead of Tracer.

instead of depending on ActiveSpanSource, it makes sense to depend directly on ThreadLocalActiveSpanSource

instead of depending on ActiveSpanSource, it makes sense to depend directly on ThreadLocalActiveSpanSource

This is a bit involved because we need to use reflection to get the instance of the ThreadLocalActiveSpanSource from the tracer. I updated to use the ActiveSpanSource interface instead.

you don't need reflection, just a cast: (ThreadLocalActiveSpanSource) tracer.activeSpanSource()

tracer.activeSpanSource isn't public. Further, it is defined in com.uber.jaeger.Tracer in jaeger-core, for which we'd have to add a dependency from jaeger-context instead of depending on opentracing-api.

yurishkuro · 2017-10-09T20:24:33Z

jaeger-context/src/main/java/com/uber/jaeger/context/TracingUtils.java

+    if (!GlobalTracer.isRegistered()) {
+      throw new IllegalStateException("Please register a io.opentracing.util.GlobalTracer");
+    }
+    traceContext = new ActiveSpanSourceTraceContext(GlobalTracer.get());


I am concerned with the race conditions here, as you don't know whether TracingUtils class will be loaded before we have a chance to init global tracer. You may need to change this into a lazy dependency, i.e. instead of passing tracer (or active span source, per the other comment) to the TraceContext impl, you mass a function that returns an active span source, so that it's dereferenced as needed.

I thought this was was fine because GlobalTracer.get() returns an instance that is a new reference to itself. However, I agree that we cannot guarantee that TracingUtil is loaded after the tracer is initialized. I'll move this to a lazy init block.

yurishkuro · 2017-10-09T20:25:50Z

jaeger-jaxrs2/src/main/java/com/uber/jaeger/filters/jaxrs2/ServerFilter.java


      Tags.HTTP_STATUS.set(serverSpan, containerResponseContext.getStatus());
-      serverSpan.finish();
+      traceContext.pop();


please add comments explaining this order (i.e. that we cannot call pop as before since it will deactivate & finish the span).

is this the only place in the whole project where we call pop?

Will do. No, we call it in our wrapped Callable and Runnable

Signed-off-by: Prithvi Raj <[email protected]>

yurishkuro · 2017-10-09T22:34:04Z

jaeger-context/src/main/java/com/uber/jaeger/context/TracingUtils.java

    return new TracedExecutorService(wrappedExecutorService, traceContext);
  }

-  private TracingUtils(){}
+  private static synchronized void initializeTraceContext() {


in my suggestion there was no additional synchronized on the hot path

I apologize, I didn't understand your previous suggestion, could you elaborate?

As such, getTraceContext and tracedExecutor are only used when initializing filters and executors. I expect these methods to be called a handful of times during application startup.

instead of ActiveSpanSourceTraceContext(Tracer) declare ActiveSpanSourceTraceContext(TracerProvider tracerProvider) and then in the implementation every time you need a tracer you retrieve it dynamically viatracerProvider.get(), which will delegate to GlobalTracer.get(). This way you leave a lot more opportunities to the application to init the global tracer before it stumbles on the need to push/pop spans into TraceContext

Signed-off-by: Prithvi Raj <[email protected]>

vprithvi · 2017-10-10T15:31:50Z

jaeger-context/src/main/java/com/uber/jaeger/context/TracingUtils.java

-  private TracingUtils(){}
+  private static void assertGlobalTracerIsRegistered() {
+    if (!GlobalTracer.isRegistered()) {
+      throw new IllegalStateException("Please register a io.opentracing.util.GlobalTracer.");


@yurishkuro I felt it was simpler to use GlobalTracer.get() directly in ActiveSpanSourceTraceContext. It makes tests dirtier, but it's temporary code.
What is your opinion on the assertGlobalTracerIsRegistered check?

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.

The type of GlobalTracer.INSTANCE is GlobalTracer and I didn't see that as being particularly advantageous because it still couples ActiveSpanSourceTraceContext to the implementation of GlobalTracer.

yurishkuro · 2017-10-11T14:04:15Z

Instance implements there Tracer interface, which decouples the new span from GlobalTracer dependency

vprithvi · 2017-10-11T15:00:22Z

Instance implements there Tracer interface, which decouples the new span from GlobalTracer dependency
@yurishkuro My understanding is that we were going to use the GlobalTracer instead of the TracerProvider that you had suggested earlier, and call a getInstance method so that the INSTANCE resolution happens as late as possible. The Tracer interface doesn't provide such a method.

This reverts commit 55d04df. Signed-off-by: Prithvi Raj <[email protected]>

Signed-off-by: Prithvi Raj <[email protected]>

vprithvi · 2017-10-11T20:04:37Z

@yurishkuro Coveralls diffs against the latest successful build on master. It shows unrelated classes because the build on master failed.

yurishkuro · 2017-10-11T20:43:48Z

master build is green now. I restarted this build.

Signed-off-by: Prithvi Raj <[email protected]>

Add a ActiveSpanSource backed TraceContext

67eb129

- use `ActiveSpanSource` for propagation instead of `ThreadLocalTraceContext` Signed-off-by: Prithvi Raj <[email protected]>

vprithvi requested a review from yurishkuro October 9, 2017 19:50

ghost assigned vprithvi Oct 9, 2017

ghost added the review label Oct 9, 2017

yurishkuro reviewed Oct 9, 2017

View reviewed changes

vprithvi added 2 commits October 9, 2017 17:36

Address feedback

4f8fc78

Signed-off-by: Prithvi Raj <[email protected]>

Remove ThreadLocalTraceContext

8277a5b

Signed-off-by: Prithvi Raj <[email protected]>

yurishkuro reviewed Oct 9, 2017

View reviewed changes

Address feedback

55d04df

Signed-off-by: Prithvi Raj <[email protected]>

vprithvi commented Oct 10, 2017

View reviewed changes

vprithvi added 2 commits October 11, 2017 14:12

Revert "Address feedback"

76048cb

This reverts commit 55d04df. Signed-off-by: Prithvi Raj <[email protected]>

Some changes

67bf0f4

Signed-off-by: Prithvi Raj <[email protected]>

vprithvi force-pushed the active-active branch from 05dffa5 to 67bf0f4 Compare October 11, 2017 18:12

vprithvi added 2 commits October 11, 2017 14:34

Add test

c986e7c

Signed-off-by: Prithvi Raj <[email protected]>

Add license

31a7b93

Signed-off-by: Prithvi Raj <[email protected]>

vprithvi added 2 commits October 11, 2017 17:36

Add more tests

6544e8c

Signed-off-by: Prithvi Raj <[email protected]>

Add license

dc4572f

Signed-off-by: Prithvi Raj <[email protected]>

yurishkuro approved these changes Oct 11, 2017

View reviewed changes

vprithvi merged commit 20573d0 into master Oct 11, 2017

ghost removed the review label Oct 11, 2017

vprithvi deleted the active-active branch October 11, 2017 21:56

vprithvi mentioned this pull request Oct 11, 2017

Make TraceContext backed by ActiveSpanSource #257

Closed

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a ActiveSpanSource backed TraceContext #266

Add a ActiveSpanSource backed TraceContext #266

vprithvi commented Oct 9, 2017

codecov bot commented Oct 9, 2017 •

edited

Loading

yurishkuro Oct 9, 2017

yurishkuro Oct 9, 2017

yurishkuro Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 9, 2017

vprithvi Oct 9, 2017

yurishkuro Oct 10, 2017

vprithvi Oct 10, 2017

yurishkuro Oct 11, 2017

vprithvi Oct 11, 2017

yurishkuro commented Oct 11, 2017

vprithvi commented Oct 11, 2017

vprithvi commented Oct 11, 2017

yurishkuro commented Oct 11, 2017

Add a ActiveSpanSource backed TraceContext #266

Add a ActiveSpanSource backed TraceContext #266

Conversation

vprithvi commented Oct 9, 2017

codecov bot commented Oct 9, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Oct 11, 2017

vprithvi commented Oct 11, 2017

vprithvi commented Oct 11, 2017

yurishkuro commented Oct 11, 2017

codecov bot commented Oct 9, 2017 •

edited

Loading