uber / tchannel-java

A Java implementation of the TChannel protocol.
MIT License
134 stars 67 forks source link

Add OpenTracingContext implementation #196

Closed yborovikov closed 6 years ago

yborovikov commented 6 years ago

This PR adds an io.opentracing.ScopeManager-based implementation of TracingContext.

codecov-io commented 6 years ago

Codecov Report

Merging #196 into master will increase coverage by 0.07%. The diff coverage is 84.21%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #196      +/-   ##
============================================
+ Coverage     71.41%   71.48%   +0.07%     
  Complexity       10       10              
============================================
  Files            86       87       +1     
  Lines          2637     2654      +17     
  Branches        313      318       +5     
============================================
+ Hits           1883     1897      +14     
  Misses          537      537              
- Partials        217      220       +3
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/com/uber/tchannel/api/TChannel.java 65.32% <0%> (-1.08%) 0 <0> (ø)
.../com/uber/tchannel/tracing/OpenTracingContext.java 88.89% <88.89%> (ø) 0 <0> (?)

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 b044085...b1a4c67. Read the comment docs.

yurishkuro commented 6 years ago

My main concern is with TracingContext.popSpan() being implemented via scope.close(). The latter is not equivalent to popping the span, because depending on how the scope was created it may also finish the span on close.

One option is to remove TracingContext altogether and just use tracer.scopeManager() as intended. But that would be a breaking change (although pulling opentracing 0.31 was already a breaking change).

Another option is to continue accepting TracingContext as optional (and deprecated) and wrap it into an adapter implementing ScopeManager, so that the rest of tchannel code is just using the scope manager.

yborovikov commented 6 years ago

I agree - TracingContext has outlived its welcome.

As for side-effects of Scope.close() - these probably should be fixed in opentracing itself. By either removing the behavior (not sure what the rationale was, to begin with - maybe you know the context) or by introducing some sort of a counter that would track activation/deactivation events (which is not likely to work properly even in ideal conditions).

Good thing is, though, that tchannel only pops scopes/spans that it itself pushed - and OpenTracingContxt.push() implementation doesn't set the finishSpanOnClose flag. So we're safe from that perspective (unless, of course, the context got corrupted - but this is a totally different and much sadder story).

yurishkuro commented 6 years ago

Good thing is, though, that tchannel only pops spans that it itself pushed

ah, that's true. In this case we should be fine.

As for side-effects of Scope.close() - these probably should be fixed in opentracing itself.

That behavior is by design. In fact, the large change from ActiveSpanManager to ScopeManager in 0.31 was specifically done to remove reference counting that was implemented in 0.30, as it caused more unexpected behavior.

:shipit: