[apache] [jax-rs2] Instrumentation specific multiple span reporting Issue #297

dray92 · 2017-11-29T03:55:11Z

Apache span interceptors have lighter dependency on threadlocal; newly created span is not pushed
HttpContext is used to propagate the span between interceptors
Similar changes in jax-rs2 instrumentation to reduce dependence on thread local, and instead use jax-rs2 specific javax.ws.rs.client.ClientRequestContext

yurishkuro · 2017-11-29T04:12:18Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

@@ -147,6 +148,10 @@ public SpanContext context() {

  @Override
  public void finish() {
+    if (finished) {


this logic should be in the finishWithDuration method, and inside the synchronized block

codecov · 2017-11-29T04:15:21Z

Codecov Report

Merging #297 into master will increase coverage by 0.09%.
The diff coverage is 75%.

@@             Coverage Diff             @@
##             master    #297      +/-   ##
===========================================
+ Coverage        83%   83.1%   +0.09%     
- Complexity      551     553       +2     
===========================================
  Files            91      91              
  Lines          2165    2160       -5     
  Branches        244     244              
===========================================
- Hits           1797    1795       -2     
+ Misses          267     266       -1     
+ Partials        101      99       -2

Impacted Files	Coverage Δ	Complexity Δ
...aeger/filters/jaxrs2/ClientSpanCreationFilter.java	`100% <ø> (ø)`	`4 <0> (ø)`	⬇️
...ger/httpclient/SpanCreationRequestInterceptor.java	`83.33% <ø> (-0.67%)`	`5 <0> (ø)`
...a/com/uber/jaeger/filters/jaxrs2/ClientFilter.java	`100% <100%> (+13.33%)`	`4 <0> (ø)`	⬇️
...er/httpclient/SpanInjectionRequestInterceptor.java	`75% <100%> (-1.93%)`	`3 <1> (ø)`
...r/jaeger/httpclient/TracingRequestInterceptor.java	`78.57% <100%> (-1.43%)`	`4 <1> (ø)`
...eger/filters/jaxrs2/ClientSpanInjectionFilter.java	`73.68% <50%> (-9.65%)`	`4 <2> (ø)`
...jaeger/reporters/protocols/ThriftUdpTransport.java	`86.44% <0%> (+5.08%)`	`16% <0%> (+2%)`	⬆️

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 9c6d3a5...4e58e60. Read the comment docs.

vprithvi · 2017-11-29T04:21:26Z

Could you split this into two separate PRs by separating the instrumentation fixes from the changes in the Span class?

dray92 · 2017-11-29T04:25:26Z

@vprithvi definitely -- lemme just remove the span class changes

…s depend on thread local, and instead on the implementation specific http context propagation mechanisms Signed-off-by: Debosmit Ray <debo@uber.com>

yurishkuro · 2017-11-30T16:29:03Z

...test/java/com/uber/jaeger/httpclient/JaegerRequestAndResponseInterceptorIntegrationTest.java

@@ -124,7 +130,7 @@ public void testHttpClientTracing() throws Exception {
  private void verifyTracing(Span parentSpan) {
    //Assert that traces are correctly emitted by the client
    List<Span> spans = reporter.getSpans();
-    assertEquals(3, spans.size());
+    assertEquals(2, spans.size());


what is the reason for this change?

Previously, when pop() was getting called, it would make another call to report the span. Hence the reporter was getting an extra span, and the last 2 spans were identical. This change proves that the core issue is fixed.

yurishkuro reviewed Nov 29, 2017

View reviewed changes

dray92 force-pushed the bugfix_multiple_span_finish branch from 2b01739 to 41a0f4e Compare November 29, 2017 04:29

dray92 changed the title ~~[jaeger-client-java] #295 Multiple Span reporting issue~~ [jaeger-client-java] [apache] [jax-rs2] Instrumentation specific multiple span reporting Issue Nov 29, 2017

Changes in apache and jax-rs2 instrumentation to not have interceptor…

4e58e60

…s depend on thread local, and instead on the implementation specific http context propagation mechanisms Signed-off-by: Debosmit Ray <debo@uber.com>

dray92 force-pushed the bugfix_multiple_span_finish branch from 41a0f4e to 4e58e60 Compare November 29, 2017 04:43

yurishkuro changed the title ~~[jaeger-client-java] [apache] [jax-rs2] Instrumentation specific multiple span reporting Issue~~ [apache] [jax-rs2] Instrumentation specific multiple span reporting Issue Nov 30, 2017

yurishkuro reviewed Nov 30, 2017

View reviewed changes

yurishkuro approved these changes Dec 1, 2017

View reviewed changes

yurishkuro merged commit 7250579 into jaegertracing:master Dec 1, 2017

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

[apache] [jax-rs2] Instrumentation specific multiple span reporting Issue #297

[apache] [jax-rs2] Instrumentation specific multiple span reporting Issue #297

dray92 commented Nov 29, 2017 •

edited by yurishkuro

Loading

yurishkuro Nov 29, 2017

codecov bot commented Nov 29, 2017 •

edited

Loading

vprithvi commented Nov 29, 2017

dray92 commented Nov 29, 2017

yurishkuro Nov 30, 2017

dray92 Nov 30, 2017 •

edited

Loading

[apache] [jax-rs2] Instrumentation specific multiple span reporting Issue #297

[apache] [jax-rs2] Instrumentation specific multiple span reporting Issue #297

Conversation

dray92 commented Nov 29, 2017 • edited by yurishkuro Loading

yurishkuro Nov 29, 2017

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2017 • edited Loading

Codecov Report

vprithvi commented Nov 29, 2017

dray92 commented Nov 29, 2017

yurishkuro Nov 30, 2017

Choose a reason for hiding this comment

dray92 Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

dray92 commented Nov 29, 2017 •

edited by yurishkuro

Loading

codecov bot commented Nov 29, 2017 •

edited

Loading

dray92 Nov 30, 2017 •

edited

Loading