Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.uber.jaeger.context.TracingUtils;
import io.opentracing.Span;
import io.opentracing.Tracer;
import io.opentracing.propagation.Format;
import io.opentracing.tag.Tags;
import java.io.IOException;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -66,9 +65,6 @@ public void process(HttpRequest httpRequest, HttpContext httpContext)
}

Span clientSpan = clientSpanBuilder.startManual();
// putting newly created span on the trace context so that
// other interceptors in the chain
parentContext.push(clientSpan);

RequestLine requestLine = httpRequest.getRequestLine();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,14 @@

package com.uber.jaeger.httpclient;

import com.uber.jaeger.context.TraceContext;
import com.uber.jaeger.context.TracingUtils;
import io.opentracing.Span;
import io.opentracing.Tracer;
import io.opentracing.propagation.Format;
import io.opentracing.tag.Tags;
import java.io.IOException;
import lombok.extern.slf4j.Slf4j;
import org.apache.http.HttpException;
import org.apache.http.HttpHost;
import org.apache.http.HttpRequest;
import org.apache.http.HttpRequestInterceptor;
import org.apache.http.RequestLine;
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.http.protocol.HttpContext;

/**
Expand All @@ -58,9 +52,7 @@ public SpanInjectionRequestInterceptor(Tracer tracer) {
public void process(HttpRequest httpRequest, HttpContext httpContext)
throws HttpException, IOException {
try {
TraceContext parentContext = TracingUtils.getTraceContext();

Span clientSpan = parentContext.pop();
Span clientSpan = (Span) httpContext.getAttribute(Constants.CURRENT_SPAN_CONTEXT_KEY);

tracer.inject(
clientSpan.context(), Format.Builtin.HTTP_HEADERS, new ClientRequestCarrier(httpRequest));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,14 @@

package com.uber.jaeger.httpclient;

import com.uber.jaeger.context.TraceContext;
import com.uber.jaeger.context.TracingUtils;
import io.opentracing.Span;
import io.opentracing.Tracer;
import io.opentracing.propagation.Format;
import io.opentracing.tag.Tags;
import java.io.IOException;
import lombok.extern.slf4j.Slf4j;
import org.apache.http.HttpException;
import org.apache.http.HttpHost;
import org.apache.http.HttpRequest;
import org.apache.http.HttpRequestInterceptor;
import org.apache.http.RequestLine;
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.http.protocol.HttpContext;

/**
Expand All @@ -40,12 +34,10 @@
*/
@Slf4j
public class TracingRequestInterceptor implements HttpRequestInterceptor {
private final Tracer tracer;
private final SpanCreationRequestInterceptor spanCreationInterceptor;
private final SpanInjectionRequestInterceptor spanInjectionInterceptor;

public TracingRequestInterceptor(Tracer tracer) {
this.tracer = tracer;
this.spanCreationInterceptor = new SpanCreationRequestInterceptor(tracer);
this.spanInjectionInterceptor = new SpanInjectionRequestInterceptor(tracer);
}
Expand All @@ -54,9 +46,9 @@ public TracingRequestInterceptor(Tracer tracer) {
public void process(HttpRequest httpRequest, HttpContext httpContext)
throws HttpException, IOException {
try {
this.spanCreationInterceptor.process(httpRequest, httpContext);
spanCreationInterceptor.process(httpRequest, httpContext);
onSpanStarted(TracingUtils.getTraceContext().getCurrentSpan(), httpRequest, httpContext);
this.spanInjectionInterceptor.process(httpRequest, httpContext);
spanInjectionInterceptor.process(httpRequest, httpContext);
} catch (Exception e) {
log.error("Could not start client tracing span.", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ public void testAsyncHttpClientTracing() throws Exception {

client.start();

// Verify that parent span is on top of the stack _before_ request is made
assertEquals(parentSpan, TracingUtils.getTraceContext().getCurrentSpan());

//Make a request to the async client and wait for response
client
.execute(
Expand All @@ -105,6 +108,9 @@ public void cancelled() {}
})
.get();

// Verify that parent span is on top of the stack _after_ request is made
assertEquals(parentSpan, TracingUtils.getTraceContext().getCurrentSpan());

verifyTracing(parentSpan);
}

Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for this change?

Copy link
Contributor Author

@dray92 dray92 Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Span span = spans.get(1);
assertEquals("GET", span.getOperationName());
assertEquals(parentSpan.context().getSpanId(), span.context().getParentId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,12 @@
@ConstrainedTo(RuntimeType.CLIENT)
@Slf4j
public class ClientFilter implements ClientRequestFilter, ClientResponseFilter {
private final Tracer tracer;
private final TraceContext traceContext;

private final ClientSpanCreationFilter spanCreationFilter;
private final ClientSpanInjectionFilter spanInjectionFilter;

public ClientFilter(Tracer tracer, TraceContext traceContext) {
this.tracer = tracer;
this.traceContext = traceContext;

this.spanCreationFilter = new ClientSpanCreationFilter(tracer, traceContext);
this.spanInjectionFilter = new ClientSpanInjectionFilter(tracer, traceContext);
this.spanInjectionFilter = new ClientSpanInjectionFilter(tracer);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,5 @@ public void filter(ClientRequestContext clientRequestContext) throws IOException
Tags.SPAN_KIND.set(clientSpan, Tags.SPAN_KIND_CLIENT);
Tags.HTTP_URL.set(clientSpan, clientRequestContext.getUri().toString());
Tags.PEER_HOSTNAME.set(clientSpan, clientRequestContext.getUri().getHost());

traceContext.push(clientSpan);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,22 @@
@Slf4j
public class ClientSpanInjectionFilter implements ClientRequestFilter, ClientResponseFilter {
private final Tracer tracer;
private final TraceContext traceContext;

/**
* @deprecated use {@link ClientSpanInjectionFilter(Tracer)}
*/
@Deprecated
public ClientSpanInjectionFilter(Tracer tracer, TraceContext traceContext) {
this(tracer);
}

public ClientSpanInjectionFilter(Tracer tracer) {
this.tracer = tracer;
this.traceContext = traceContext;
}

@Override
public void filter(ClientRequestContext clientRequestContext) throws IOException {
Span clientSpan = traceContext.getCurrentSpan();
Span clientSpan = (Span) clientRequestContext.getProperty(Constants.CURRENT_SPAN_CONTEXT_KEY);

tracer.inject(
clientSpan.context(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import java.util.Map;
import javax.ws.rs.client.ClientRequestContext;
import javax.ws.rs.client.ClientResponseContext;
import javax.ws.rs.core.MultivaluedHashMap;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -73,7 +72,6 @@ public void testFilter() throws Exception {
String method = "GET";
when(clientRequestContext.getMethod()).thenReturn(method);
when(clientRequestContext.getUri()).thenReturn(new URI("http://localhost/path"));
when(clientRequestContext.getHeaders()).thenReturn(new MultivaluedHashMap<String, Object>());

when(clientResponseContext.getStatus()).thenReturn(200);

Expand Down