From 9060ecb17f608bbe00d05272d3dae281473dd9e1 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Sun, 7 May 2017 17:10:39 +0800 Subject: [PATCH] Ensures current span is always closed prior to finishing (#393) This changes logic in http handlers such that a span is always closed prior to being finished. This has two advantages: one is that we can test that instrumentation do not leak span contexts. Another is that log context synchronization will not add trace IDs of a closed span. --- .../java/brave/http/HttpClientHandler.java | 61 ++++++++------ .../java/brave/http/HttpServerHandler.java | 61 ++++++++------ .../brave/grpc/TracingClientInterceptor.java | 2 +- .../src/main/java/brave/http/ITHttp.java | 61 ++++++++++++++ .../main/java/brave/http/ITHttpClient.java | 82 ++++--------------- .../main/java/brave/http/ITHttpServer.java | 54 ++---------- .../TracingHttpAsyncClientBuilder.java | 10 +-- .../httpclient/TracingHttpClientBuilder.java | 17 ++-- .../brave/jaxrs2/TracingClientFilter.java | 10 +-- .../brave/jaxrs2/TracingContainerFilter.java | 8 +- .../brave/okhttp3/TracingCallFactory.java | 13 ++- .../brave/servlet/HttpServletAdapter.java | 52 ++++++++++++ .../brave/servlet/HttpServletHandler.java | 62 -------------- .../java/brave/servlet/ServletRuntime.java | 16 ++-- .../java/brave/servlet/TracingFilter.java | 21 +++-- .../java/brave/sparkjava/SparkTracing.java | 17 ++-- .../TracingClientHttpRequestInterceptor.java | 13 ++- .../webmvc/TracingHandlerInterceptor.java | 27 +++--- 18 files changed, 297 insertions(+), 290 deletions(-) create mode 100644 instrumentation/http-tests/src/main/java/brave/http/ITHttp.java create mode 100644 instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java delete mode 100644 instrumentation/servlet/src/main/java/brave/servlet/HttpServletHandler.java diff --git a/brave/src/main/java/brave/http/HttpClientHandler.java b/brave/src/main/java/brave/http/HttpClientHandler.java index 60f93b6ad3..a7a908b682 100644 --- a/brave/src/main/java/brave/http/HttpClientHandler.java +++ b/brave/src/main/java/brave/http/HttpClientHandler.java @@ -1,46 +1,61 @@ package brave.http; import brave.Span; -import zipkin.Constants; +import brave.internal.Nullable; +import brave.propagation.TraceContext; + +/** + * This standardizes a way to instrument http clients, particularly in a way that encourages use of + * portable customizations via {@link HttpClientParser}. + * + * @param the native http request type of the client. + * @param the native http response type of the client. + */ +public final class HttpClientHandler { + public static HttpClientHandler create(HttpAdapter adapter, + HttpClientParser parser) { + return new HttpClientHandler<>(adapter, parser); + } -public class HttpClientHandler { final HttpAdapter adapter; final HttpClientParser parser; - public HttpClientHandler(HttpAdapter adapter, HttpClientParser parser) { + HttpClientHandler(HttpAdapter adapter, HttpClientParser parser) { this.adapter = adapter; this.parser = parser; } - public Req handleSend(Req request, Span span) { - if (span.isNoop()) return request; + /** + * Starts the client span after assigning it a name and tags. + * + *

This is typically called after a span is {@link TraceContext.Injector#inject(TraceContext, + * Object) injected} onto the request, and before request is processed by the actual library. + */ + public void handleSend(Req request, Span span) { + if (span.isNoop()) return; // all of the parsing here occur before a timestamp is recorded on the span span.kind(Span.Kind.CLIENT).name(parser.spanName(adapter, request)); parser.requestTags(adapter, request, span); span.start(); - return request; - } - - public Resp handleReceive(Resp response, Span span) { - if (span.isNoop()) return response; - - try { - parser.responseTags(adapter, response, span); - } finally { - span.finish(); - } - return response; } - public T handleError(T throwable, Span span) { - if (span.isNoop()) return throwable; + /** + * Finishes the client span after assigning it tags according to the response or error. + * + *

This is typically called once the response headers are received, and after the span is + * {@link brave.Tracer.SpanInScope#close() no longer in scope}. + */ + public void handleReceive(@Nullable Resp response, @Nullable Throwable error, Span span) { + if (span.isNoop()) return; try { - String message = throwable.getMessage(); - if (message == null) message = throwable.getClass().getSimpleName(); - span.tag(Constants.ERROR, message); - return throwable; + if (error != null) { + String message = error.getMessage(); + if (message == null) message = error.getClass().getSimpleName(); + span.tag(zipkin.Constants.ERROR, message); + } + if (response != null) parser.responseTags(adapter, response, span); } finally { span.finish(); } diff --git a/brave/src/main/java/brave/http/HttpServerHandler.java b/brave/src/main/java/brave/http/HttpServerHandler.java index 1c897f9446..be5d661cd8 100644 --- a/brave/src/main/java/brave/http/HttpServerHandler.java +++ b/brave/src/main/java/brave/http/HttpServerHandler.java @@ -1,46 +1,61 @@ package brave.http; import brave.Span; -import zipkin.Constants; +import brave.internal.Nullable; +import brave.propagation.TraceContext; + +/** + * This standardizes a way to instrument http servers, particularly in a way that encourages use of + * portable customizations via {@link HttpServerParser}. + * + * @param the native http request type of the server. + * @param the native http response type of the server. + */ +public final class HttpServerHandler { + public static HttpServerHandler create(HttpAdapter adapter, + HttpServerParser parser) { + return new HttpServerHandler<>(adapter, parser); + } -public class HttpServerHandler { final HttpAdapter adapter; final HttpServerParser parser; - public HttpServerHandler(HttpAdapter adapter, HttpServerParser parser) { + HttpServerHandler(HttpAdapter adapter, HttpServerParser parser) { this.adapter = adapter; this.parser = parser; } - public Req handleReceive(Req request, Span span) { - if (span.isNoop()) return request; + /** + * Starts the server span after assigning it a name and tags. + * + *

This is typically called after a span is {@link brave.Tracer#nextSpan(TraceContext.Extractor, + * Object) extracted} from the request and before the request is processed by the actual library. + */ + public void handleReceive(Req request, Span span) { + if (span.isNoop()) return; // all of the parsing here occur before a timestamp is recorded on the span span.kind(Span.Kind.SERVER).name(parser.spanName(adapter, request)); parser.requestTags(adapter, request, span); span.start(); - return request; - } - - public Resp handleSend(Resp response, Span span) { - if (span.isNoop()) return response; - - try { - parser.responseTags(adapter, response, span); - } finally { - span.finish(); - } - return response; } - public T handleError(T throwable, Span span) { - if (span.isNoop()) return throwable; + /** + * Finishes the server span after assigning it tags according to the response or error. + * + *

This is typically called once the response headers are sent, and after the span is + * {@link brave.Tracer.SpanInScope#close() no longer in scope}. + */ + public void handleSend(@Nullable Resp response, @Nullable Throwable error, Span span) { + if (span.isNoop()) return; try { - String message = throwable.getMessage(); - if (message == null) message = throwable.getClass().getSimpleName(); - span.tag(Constants.ERROR, message); - return throwable; + if (error != null) { + String message = error.getMessage(); + if (message == null) message = error.getClass().getSimpleName(); + span.tag(zipkin.Constants.ERROR, message); + } + if (response != null) parser.responseTags(adapter, response, span); } finally { span.finish(); } diff --git a/instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java b/instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java index 7de1be867b..a3b62a63f5 100644 --- a/instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java +++ b/instrumentation/grpc/src/main/java/brave/grpc/TracingClientInterceptor.java @@ -47,8 +47,8 @@ public ClientCall interceptCall( return new SimpleForwardingClientCall(next.newCall(method, callOptions)) { @Override public void start(Listener responseListener, Metadata headers) { - span.kind(Span.Kind.CLIENT).name(method.getFullMethodName()).start(); injector.inject(span.context(), headers); + span.kind(Span.Kind.CLIENT).name(method.getFullMethodName()).start(); try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) { super.start(new SimpleForwardingClientCallListener(responseListener) { @Override public void onClose(Status status, Metadata trailers) { diff --git a/instrumentation/http-tests/src/main/java/brave/http/ITHttp.java b/instrumentation/http-tests/src/main/java/brave/http/ITHttp.java new file mode 100644 index 0000000000..f5caf491fd --- /dev/null +++ b/instrumentation/http-tests/src/main/java/brave/http/ITHttp.java @@ -0,0 +1,61 @@ +package brave.http; + +import brave.Tracing; +import brave.internal.StrictCurrentTraceContext; +import brave.propagation.CurrentTraceContext; +import brave.propagation.TraceContext; +import brave.sampler.Sampler; +import java.util.Collections; +import java.util.List; +import okhttp3.mockwebserver.MockWebServer; +import org.junit.Rule; +import org.junit.rules.ExpectedException; +import zipkin.Endpoint; +import zipkin.Span; +import zipkin.internal.Util; +import zipkin.storage.InMemoryStorage; + +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + +public abstract class ITHttp { + @Rule public ExpectedException thrown = ExpectedException.none(); + @Rule public MockWebServer server = new MockWebServer(); + + Endpoint local = Endpoint.builder().serviceName("local").ipv4(127 << 24 | 1).port(100).build(); + InMemoryStorage storage = new InMemoryStorage(); + + protected CurrentTraceContext currentTraceContext = new StrictCurrentTraceContext(); + protected HttpTracing httpTracing; + + Tracing.Builder tracingBuilder(Sampler sampler) { + return Tracing.newBuilder() + .reporter(s -> { + // make sure the context was cleared prior to finish.. no leaks! + TraceContext current = httpTracing.tracing().currentTraceContext().get(); + if (current != null) { + assertThat(current.spanId()) + .isNotEqualTo(s.id); + } + storage.spanConsumer().accept(asList(s)); + }) + .currentTraceContext(currentTraceContext) + .localEndpoint(local) + .sampler(sampler); + } + + List collectedSpans() { + List> result = storage.spanStore().getRawTraces(); + if (result.isEmpty()) return Collections.emptyList(); + assertThat(result).hasSize(1); + return result.get(0); + } + + void assertReportedTagsInclude(String key, String... values) { + assertThat(collectedSpans()) + .flatExtracting(s -> s.binaryAnnotations) + .filteredOn(b -> b.key.equals(key)) + .extracting(b -> new String(b.value, Util.UTF_8)) + .containsExactly(values); + } +} diff --git a/instrumentation/http-tests/src/main/java/brave/http/ITHttpClient.java b/instrumentation/http-tests/src/main/java/brave/http/ITHttpClient.java index db6ac89070..de578b289e 100644 --- a/instrumentation/http-tests/src/main/java/brave/http/ITHttpClient.java +++ b/instrumentation/http-tests/src/main/java/brave/http/ITHttpClient.java @@ -2,43 +2,26 @@ import brave.Tracer; import brave.Tracer.SpanInScope; -import brave.Tracing; import brave.internal.HexCodec; -import brave.internal.StrictCurrentTraceContext; -import brave.propagation.CurrentTraceContext; import brave.sampler.Sampler; import java.io.IOException; import java.util.List; import java.util.concurrent.TimeUnit; import okhttp3.mockwebserver.MockResponse; -import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; import okhttp3.mockwebserver.SocketPolicy; import org.junit.After; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; -import zipkin.BinaryAnnotation; import zipkin.Constants; import zipkin.Endpoint; import zipkin.Span; import zipkin.TraceKeys; -import zipkin.internal.Util; -import zipkin.storage.InMemoryStorage; import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; -public abstract class ITHttpClient { - @Rule public ExpectedException thrown = ExpectedException.none(); - @Rule public MockWebServer server = new MockWebServer(); - - Endpoint local = Endpoint.builder().serviceName("local").ipv4(127 << 24 | 1).port(100).build(); - InMemoryStorage storage = new InMemoryStorage(); - - protected CurrentTraceContext currentTraceContext = new StrictCurrentTraceContext(); - protected HttpTracing httpTracing; +public abstract class ITHttpClient extends ITHttp { protected C client; @Before public void setup() { @@ -175,19 +158,19 @@ public void reportsServerAddress() throws Exception { String uri = "/foo?z=2&yAA=1"; close(); -httpTracing = httpTracing.toBuilder() - .clientParser(new HttpClientParser() { - @Override public String spanName(HttpAdapter adapter, Req req) { - return adapter.method(req).toLowerCase() + " " + adapter.path(req); - } - - @Override - public void requestTags(HttpAdapter adapter, Req req, brave.Span span) { - span.tag(TraceKeys.HTTP_URL, adapter.url(req)); // just the path is logged by default - } - }) - .serverName("remote-service") - .build(); + httpTracing = httpTracing.toBuilder() + .clientParser(new HttpClientParser() { + @Override public String spanName(HttpAdapter adapter, Req req) { + return adapter.method(req).toLowerCase() + " " + adapter.path(req); + } + + @Override + public void requestTags(HttpAdapter adapter, Req req, brave.Span span) { + span.tag(TraceKeys.HTTP_URL, adapter.url(req)); // just the path is logged by default + } + }) + .serverName("remote-service") + .build(); client = newClient(server.getPort()); server.enqueue(new MockResponse()); @@ -204,11 +187,7 @@ public void requestTags(HttpAdapter adapter, Req req, brave.Span s .extracting(b -> b.endpoint.serviceName) .containsExactly("remote-service"); - assertThat(collectedSpans) - .flatExtracting(s -> s.binaryAnnotations) - .filteredOn(b -> b.key.equals(TraceKeys.HTTP_URL)) - .extracting(b -> new String(b.value, Util.UTF_8)) - .containsExactly(url(uri)); + assertReportedTagsInclude(TraceKeys.HTTP_URL, url(uri)); } @Test public void addsStatusCodeWhenNotOk() throws Exception { @@ -220,9 +199,7 @@ public void requestTags(HttpAdapter adapter, Req req, brave.Span s // some clients think 404 is an error } - assertThat(collectedSpans()) - .flatExtracting(s -> s.binaryAnnotations) - .contains(BinaryAnnotation.create(TraceKeys.HTTP_STATUS_CODE, "404", local)); + assertReportedTagsInclude(TraceKeys.HTTP_STATUS_CODE, "404"); } @Test public void redirect() throws Exception { @@ -240,12 +217,7 @@ public void requestTags(HttpAdapter adapter, Req req, brave.Span s parent.finish(); } - assertThat(collectedSpans()) - .hasSize(3) // parent and two HTTP spans - .flatExtracting(s -> s.binaryAnnotations) - .filteredOn(b -> b.key.equals(TraceKeys.HTTP_PATH)) - .extracting(b -> new String(b.value, Util.UTF_8)) - .containsExactly("/foo", "/bar"); + assertReportedTagsInclude(TraceKeys.HTTP_PATH, "/foo", "/bar"); } @Test public void post() throws Exception { @@ -290,28 +262,10 @@ public void requestTags(HttpAdapter adapter, Req req, brave.Span s server.enqueue(new MockResponse()); get(client, path); - assertThat(collectedSpans()) - .flatExtracting(s -> s.binaryAnnotations) - .filteredOn(b -> b.key.equals(TraceKeys.HTTP_PATH)) - .extracting(b -> new String(b.value, Util.UTF_8)) - .containsExactly("/foo"); + assertReportedTagsInclude(TraceKeys.HTTP_PATH, "/foo"); } protected String url(String pathIncludingQuery) { return "http://127.0.0.1:" + server.getPort() + pathIncludingQuery; } - - Tracing.Builder tracingBuilder(Sampler sampler) { - return Tracing.newBuilder() - .reporter(s -> storage.spanConsumer().accept(asList(s))) - .currentTraceContext(currentTraceContext) - .localEndpoint(local) - .sampler(sampler); - } - - List collectedSpans() { - List> result = storage.spanStore().getRawTraces(); - assertThat(result).hasSize(1); - return result.get(0); - } } diff --git a/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java b/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java index 47da0553c0..bd3b95558b 100644 --- a/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java +++ b/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java @@ -1,42 +1,23 @@ package brave.http; -import brave.Tracing; import brave.internal.HexCodec; -import brave.internal.StrictCurrentTraceContext; -import brave.propagation.CurrentTraceContext; import brave.sampler.Sampler; -import java.util.Collections; import java.util.List; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; import org.junit.AssumptionViolatedException; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; -import zipkin.BinaryAnnotation; import zipkin.Constants; -import zipkin.Endpoint; import zipkin.Span; import zipkin.TraceKeys; -import zipkin.internal.Util; -import zipkin.storage.InMemoryStorage; -import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.groups.Tuple.tuple; -public abstract class ITHttpServer { - - @Rule public ExpectedException thrown = ExpectedException.none(); - public OkHttpClient client = new OkHttpClient(); - - Endpoint local = Endpoint.builder().serviceName("local").ipv4(127 << 24 | 1).port(100).build(); - InMemoryStorage storage = new InMemoryStorage(); - - protected CurrentTraceContext currentTraceContext = new StrictCurrentTraceContext(); - protected HttpTracing httpTracing; +public abstract class ITHttpServer extends ITHttp { + OkHttpClient client = new OkHttpClient(); @Before public void setup() throws Exception { @@ -228,11 +209,7 @@ public void requestTags(HttpAdapter adapter, Req req, brave.Span s .extracting(s -> s.name) .containsExactly("get /foo"); - assertThat(collectedSpans) - .flatExtracting(s -> s.binaryAnnotations) - .filteredOn(b -> b.key.equals(TraceKeys.HTTP_URL)) - .extracting(b -> new String(b.value, Util.UTF_8)) - .containsExactly(url(uri)); + assertReportedTagsInclude(TraceKeys.HTTP_URL, url(uri)); } @Test @@ -245,9 +222,7 @@ public void addsStatusCode_badRequest() throws Exception { // some servers think 400 is an error } - assertThat(collectedSpans()) - .flatExtracting(s -> s.binaryAnnotations) - .contains(BinaryAnnotation.create(TraceKeys.HTTP_STATUS_CODE, "400", local)); + assertReportedTagsInclude(TraceKeys.HTTP_STATUS_CODE, "400"); } @Test @@ -305,25 +280,6 @@ public void httpPathTagExcludesQueryParams() throws Exception { assertThat(response.isSuccessful()).isTrue(); } - assertThat(collectedSpans()) - .flatExtracting(s -> s.binaryAnnotations) - .filteredOn(b -> b.key.equals(TraceKeys.HTTP_PATH)) - .extracting(b -> new String(b.value, Util.UTF_8)) - .containsExactly("/foo"); - } - - Tracing.Builder tracingBuilder(Sampler sampler) { - return Tracing.newBuilder() - .reporter(s -> storage.spanConsumer().accept(asList(s))) - .currentTraceContext(currentTraceContext) - .localEndpoint(local) - .sampler(sampler); - } - - protected List collectedSpans() { - List> result = storage.spanStore().getRawTraces(); - if (result.isEmpty()) return Collections.emptyList(); - assertThat(result).hasSize(1); - return result.get(0); + assertReportedTagsInclude(TraceKeys.HTTP_PATH, "/foo"); } } diff --git a/instrumentation/httpasyncclient/src/main/java/brave/httpasyncclient/TracingHttpAsyncClientBuilder.java b/instrumentation/httpasyncclient/src/main/java/brave/httpasyncclient/TracingHttpAsyncClientBuilder.java index 7f1e7c67c6..fee4b62d3d 100644 --- a/instrumentation/httpasyncclient/src/main/java/brave/httpasyncclient/TracingHttpAsyncClientBuilder.java +++ b/instrumentation/httpasyncclient/src/main/java/brave/httpasyncclient/TracingHttpAsyncClientBuilder.java @@ -49,7 +49,7 @@ public static HttpAsyncClientBuilder create(HttpTracing httpTracing) { if (httpTracing == null) throw new NullPointerException("httpTracing == null"); this.tracer = httpTracing.tracing().tracer(); this.remoteServiceName = httpTracing.serverName(); - this.handler = new HttpClientHandler<>(new HttpAdapter(), httpTracing.clientParser()); + this.handler = HttpClientHandler.create(new HttpAdapter(), httpTracing.clientParser()); this.injector = httpTracing.tracing().propagation().injector(HttpMessage::setHeader); } @@ -57,6 +57,7 @@ public static HttpAsyncClientBuilder create(HttpTracing httpTracing) { super.addInterceptorFirst((HttpRequestInterceptor) (request, context) -> { TraceContext parent = (TraceContext) context.getAttribute(TraceContext.class.getName()); Span span = parent == null ? tracer.newTrace() : tracer.newChild(parent); + injector.inject(span.context(), request); HttpHost host = HttpClientContext.adapt(context).getTargetHost(); if (!span.isNoop() && host != null) { @@ -66,7 +67,6 @@ public static HttpAsyncClientBuilder create(HttpTracing httpTracing) { handler.handleSend(request, span); } - injector.inject(span.context(), request); context.setAttribute(Span.class.getName(), span); context.setAttribute(SpanInScope.class.getName(), tracer.withSpanInScope(span)); }); @@ -79,7 +79,7 @@ public static HttpAsyncClientBuilder create(HttpTracing httpTracing) { }); super.addInterceptorLast((HttpResponseInterceptor) (response, context) -> { Span span = (Span) context.getAttribute(Span.class.getName()); - handler.handleReceive(response, span); + handler.handleReceive(response, null, span); }); return new TracingHttpAsyncClient(super.build()); } @@ -186,8 +186,8 @@ final class TracingAsyncRequestProducer implements HttpAsyncRequestProducer { @Override public void failed(Exception ex) { Span currentSpan = (Span) context.getAttribute(Span.class.getName()); if (currentSpan != null) { - handler.handleError(ex, currentSpan); context.removeAttribute(Span.class.getName()); + handler.handleReceive(null, ex, currentSpan); } requestProducer.failed(ex); } @@ -228,8 +228,8 @@ final class TracingAsyncResponseConsumer implements HttpAsyncResponseConsumer @Override public void failed(Exception ex) { Span currentSpan = (Span) context.getAttribute(Span.class.getName()); if (currentSpan != null) { - handler.handleError(ex, currentSpan); context.removeAttribute(Span.class.getName()); + handler.handleReceive(null, ex, currentSpan); } responseConsumer.failed(ex); } diff --git a/instrumentation/httpclient/src/main/java/brave/httpclient/TracingHttpClientBuilder.java b/instrumentation/httpclient/src/main/java/brave/httpclient/TracingHttpClientBuilder.java index dbfcceb32a..dd6028cb3a 100644 --- a/instrumentation/httpclient/src/main/java/brave/httpclient/TracingHttpClientBuilder.java +++ b/instrumentation/httpclient/src/main/java/brave/httpclient/TracingHttpClientBuilder.java @@ -39,7 +39,7 @@ public static HttpClientBuilder create(HttpTracing httpTracing) { if (httpTracing == null) throw new NullPointerException("HttpTracing == null"); this.tracer = httpTracing.tracing().tracer(); this.remoteServiceName = httpTracing.serverName(); - this.handler = new HttpClientHandler<>(new HttpAdapter(), httpTracing.clientParser()); + this.handler = HttpClientHandler.create(new HttpAdapter(), httpTracing.clientParser()); this.injector = httpTracing.tracing().propagation().injector(HttpMessage::setHeader); } @@ -70,7 +70,11 @@ public static HttpClientBuilder create(HttpTracing httpTracing) { @Override protected ClientExecChain decorateMainExec(ClientExecChain exec) { return (route, request, context, execAware) -> { Span span = tracer.currentSpan(); + CloseableHttpResponse response = null; + Throwable error = null; try { + injector.inject(span.context(), request); + HttpHost host = HttpClientContext.adapt(context).getTargetHost(); if (!span.isNoop() && host != null) { parseServerAddress(host, span); @@ -78,18 +82,13 @@ public static HttpClientBuilder create(HttpTracing httpTracing) { } else { handler.handleSend(request, span); } - - injector.inject(span.context(), request); - - CloseableHttpResponse response = exec.execute(route, request, context, execAware); - - handler.handleReceive(response, span); - return response; + return response = exec.execute(route, request, context, execAware); } catch (IOException | HttpException | RuntimeException e) { - handler.handleError(e, span); + error = e; throw e; } finally { context.getAttribute(SpanInScope.class.getName(), SpanInScope.class).close(); + handler.handleReceive(response, error, span); } }; } diff --git a/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingClientFilter.java b/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingClientFilter.java index 8e3093f987..9dd01ca135 100644 --- a/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingClientFilter.java +++ b/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingClientFilter.java @@ -41,7 +41,7 @@ final class TracingClientFilter implements ClientRequestFilter, ClientResponseFi if (httpTracing == null) throw new NullPointerException("HttpTracing == null"); tracer = httpTracing.tracing().tracer(); remoteServiceName = httpTracing.serverName(); - handler = new HttpClientHandler<>(new HttpAdapter(), httpTracing.clientParser()); + handler = HttpClientHandler.create(new HttpAdapter(), httpTracing.clientParser()); injector = httpTracing.tracing().propagation().injector(MultivaluedMap::putSingle); } @@ -65,10 +65,10 @@ void parseServerAddress(ClientRequestContext request, Span span) { @Override public void filter(ClientRequestContext request, ClientResponseContext response) { - SpanInScope spanInScope = (SpanInScope) request.getProperty(SpanInScope.class.getName()); - if (spanInScope == null) return; - handler.handleReceive(response, tracer.currentSpan()); - spanInScope.close(); + Span span = tracer.currentSpan(); + if (span == null) return; + ((Tracer.SpanInScope) request.getProperty(Tracer.SpanInScope.class.getName())).close(); + handler.handleReceive(response, null, span); } static final class HttpAdapter diff --git a/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingContainerFilter.java b/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingContainerFilter.java index 03adbe8bb6..a55566e9fb 100644 --- a/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingContainerFilter.java +++ b/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/TracingContainerFilter.java @@ -37,7 +37,7 @@ @Inject TracingContainerFilter(HttpTracing httpTracing) { tracer = httpTracing.tracing().tracer(); - handler = new HttpServerHandler<>(new HttpAdapter(), httpTracing.serverParser()); + handler = HttpServerHandler.create(new HttpAdapter(), httpTracing.serverParser()); extractor = httpTracing.tracing().propagation() .extractor(ContainerRequestContext::getHeaderString); } @@ -57,14 +57,14 @@ } } - private Span startSpan(ContainerRequestContext context) { + Span startSpan(ContainerRequestContext context) { Span span = tracer.nextSpan(extractor, context); parseClientAddress(context, span); handler.handleReceive(context, span); return span; } - void parseClientAddress(ContainerRequestContext context, Span span) { + static void parseClientAddress(ContainerRequestContext context, Span span) { if (span.isNoop()) return; Endpoint.Builder builder = Endpoint.builder().serviceName(""); if (builder.parseIp(context.getHeaderString("X-Forwarded-For"))) { @@ -90,7 +90,7 @@ public void filter(ContainerRequestContext request, ContainerResponseContext res if (statusInfo.getFamily() == Response.Status.Family.SERVER_ERROR) { span.tag(Constants.ERROR, statusInfo.getReasonPhrase()); } - handler.handleSend(response, span); + handler.handleSend(response, null, span); } /** diff --git a/instrumentation/okhttp3/src/main/java/brave/okhttp3/TracingCallFactory.java b/instrumentation/okhttp3/src/main/java/brave/okhttp3/TracingCallFactory.java index 115c2575ab..30469a2196 100644 --- a/instrumentation/okhttp3/src/main/java/brave/okhttp3/TracingCallFactory.java +++ b/instrumentation/okhttp3/src/main/java/brave/okhttp3/TracingCallFactory.java @@ -42,7 +42,7 @@ public static Call.Factory create(HttpTracing httpTracing, OkHttpClient ok) { if (ok == null) throw new NullPointerException("OkHttpClient == null"); tracer = httpTracing.tracing().tracer(); remoteServiceName = httpTracing.serverName(); - handler = new HttpClientHandler<>(new HttpAdapter(), httpTracing.clientParser()); + handler = HttpClientHandler.create(new HttpAdapter(), httpTracing.clientParser()); injector = httpTracing.tracing().propagation().injector(Request.Builder::addHeader); this.ok = ok; } @@ -97,14 +97,19 @@ class TracingNetworkInterceptor implements Interceptor { Span span = tracer.nextSpan(); parseServerAddress(chain.connection(), span); Request request = chain.request(); - handler.handleSend(request, span); Request.Builder requestBuilder = request.newBuilder(); injector.inject(span.context(), requestBuilder); + handler.handleSend(request, span); + + Response response = null; + Throwable error = null; try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) { - return handler.handleReceive(chain.proceed(requestBuilder.build()), span); + return response = chain.proceed(requestBuilder.build()); } catch (IOException | RuntimeException e) { - handler.handleError(e, span); + error = e; throw e; + } finally { + handler.handleReceive(response, error, span); } } } diff --git a/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java b/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java new file mode 100644 index 0000000000..0d0bc9aa39 --- /dev/null +++ b/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java @@ -0,0 +1,52 @@ +package brave.servlet; + +import brave.Span; +import brave.http.HttpAdapter; +import brave.http.HttpServerHandler; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import zipkin.Endpoint; + +/** This can also parse the remote IP of the client. */ +// public for others like sparkjava to use +public final class HttpServletAdapter extends HttpAdapter { + final ServletRuntime servlet = ServletRuntime.get(); + + /** + * Utility for parsing the remote address, via the "X-Forwarded-For" header, falling back to the + * {@linkplain HttpServletRequest#getRemoteAddr() remote address}. + * + *

Typically parsed before {@link HttpServerHandler#handleReceive(Object, Span)} is called. + */ + public static void parseClientAddress(HttpServletRequest request, Span span) { + if (span.isNoop()) return; + Endpoint.Builder builder = Endpoint.builder().serviceName(""); + boolean parsed = builder.parseIp(request.getHeader("X-Forwarded-For")); + if (!parsed) parsed = builder.parseIp(request.getRemoteAddr()); + if (parsed) span.remoteEndpoint(builder.port(request.getRemotePort()).build()); + } + + @Override public String method(HttpServletRequest request) { + return request.getMethod(); + } + + @Override public String path(HttpServletRequest request) { + return request.getRequestURI(); + } + + @Override public String url(HttpServletRequest request) { + StringBuffer url = request.getRequestURL(); + if (request.getQueryString() != null && !request.getQueryString().isEmpty()) { + url.append('?').append(request.getQueryString()); + } + return url.toString(); + } + + @Override public String requestHeader(HttpServletRequest request, String name) { + return request.getHeader(name); + } + + @Override public Integer statusCode(HttpServletResponse response) { + return servlet.status(response); + } +} \ No newline at end of file diff --git a/instrumentation/servlet/src/main/java/brave/servlet/HttpServletHandler.java b/instrumentation/servlet/src/main/java/brave/servlet/HttpServletHandler.java deleted file mode 100644 index 278a18939b..0000000000 --- a/instrumentation/servlet/src/main/java/brave/servlet/HttpServletHandler.java +++ /dev/null @@ -1,62 +0,0 @@ -package brave.servlet; - -import brave.Span; -import brave.http.HttpAdapter; -import brave.http.HttpServerHandler; -import brave.http.HttpServerParser; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import zipkin.Endpoint; - -/** - * This is a generic handler which parses the remote IP of the client by default. - */ -public final class HttpServletHandler // public for others like sparkjava to use - extends HttpServerHandler { - - public HttpServletHandler(HttpServerParser parser) { - super(new HttpServletAdapter(), parser); - } - - @Override public HttpServletRequest handleReceive(HttpServletRequest request, Span span) { - if (span.isNoop()) return request; - parseClientAddress(request, span); - return super.handleReceive(request, span); - } - - void parseClientAddress(HttpServletRequest request, Span span) { - Endpoint.Builder builder = Endpoint.builder().serviceName(""); - boolean parsed = builder.parseIp(request.getHeader("X-Forwarded-For")); - if (!parsed) parsed = builder.parseIp(request.getRemoteAddr()); - if (parsed) span.remoteEndpoint(builder.port(request.getRemotePort()).build()); - } - - static final class HttpServletAdapter - extends HttpAdapter { - final ServletRuntime servlet = ServletRuntime.get(); - - @Override public String method(HttpServletRequest request) { - return request.getMethod(); - } - - @Override public String path(HttpServletRequest request) { - return request.getRequestURI(); - } - - @Override public String url(HttpServletRequest request) { - StringBuffer url = request.getRequestURL(); - if (request.getQueryString() != null && !request.getQueryString().isEmpty()) { - url.append('?').append(request.getQueryString()); - } - return url.toString(); - } - - @Override public String requestHeader(HttpServletRequest request, String name) { - return request.getHeader(name); - } - - @Override public Integer statusCode(HttpServletResponse response) { - return servlet.status(response); - } - } -} diff --git a/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java b/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java index 3ab2060b51..0ade449eff 100644 --- a/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java +++ b/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java @@ -1,6 +1,7 @@ package brave.servlet; import brave.Span; +import brave.http.HttpServerHandler; import brave.internal.Nullable; import java.io.IOException; import javax.servlet.AsyncEvent; @@ -28,7 +29,8 @@ HttpServletResponse httpResponse(ServletResponse response) { abstract boolean isAsync(HttpServletRequest request); - abstract void handleAsync(HttpServletHandler handler, HttpServletRequest request, Span span); + abstract void handleAsync(HttpServerHandler handler, + HttpServletRequest request, Span span); ServletRuntime() { } @@ -63,20 +65,21 @@ private static final class Servlet3 extends ServletRuntime { return response.getStatus(); } - @Override void handleAsync(HttpServletHandler handler, HttpServletRequest request, Span span) { + @Override void handleAsync(HttpServerHandler handler, + HttpServletRequest request, Span span) { if (span.isNoop()) return; // don't add overhead when we aren't httpTracing request.getAsyncContext().addListener(new AsyncListener() { @Override public void onComplete(AsyncEvent e) throws IOException { - handler.handleSend((HttpServletResponse) e.getSuppliedResponse(), span); + handler.handleSend((HttpServletResponse) e.getSuppliedResponse(), null, span); } @Override public void onTimeout(AsyncEvent e) throws IOException { span.tag(ERROR, String.format("Timed out after %sms", e.getAsyncContext().getTimeout())); - handler.handleSend((HttpServletResponse) e.getSuppliedResponse(), span); + handler.handleSend((HttpServletResponse) e.getSuppliedResponse(), null, span); } @Override public void onError(AsyncEvent e) throws IOException { - handler.handleError(e.getThrowable(), span); + handler.handleSend(null, e.getThrowable(), span); } @Override public void onStartAsync(AsyncEvent e) throws IOException { @@ -94,7 +97,8 @@ private static final class Servlet25 extends ServletRuntime { return false; } - @Override void handleAsync(HttpServletHandler handler, HttpServletRequest request, Span span) { + @Override void handleAsync(HttpServerHandler handler, + HttpServletRequest request, Span span) { assert false : "this should never be called in Servlet 2.5"; } diff --git a/instrumentation/servlet/src/main/java/brave/servlet/TracingFilter.java b/instrumentation/servlet/src/main/java/brave/servlet/TracingFilter.java index d9f1878d4b..458906b3e8 100644 --- a/instrumentation/servlet/src/main/java/brave/servlet/TracingFilter.java +++ b/instrumentation/servlet/src/main/java/brave/servlet/TracingFilter.java @@ -3,6 +3,7 @@ import brave.Span; import brave.Tracer; import brave.Tracing; +import brave.http.HttpServerHandler; import brave.http.HttpTracing; import brave.propagation.TraceContext; import java.io.IOException; @@ -26,12 +27,12 @@ public static Filter create(HttpTracing httpTracing) { final ServletRuntime servlet = ServletRuntime.get(); final Tracer tracer; - final HttpServletHandler handler; + final HttpServerHandler handler; final TraceContext.Extractor extractor; TracingFilter(HttpTracing httpTracing) { tracer = httpTracing.tracing().tracer(); - handler = new HttpServletHandler(httpTracing.serverParser()); + handler = HttpServerHandler.create(new HttpServletAdapter(), httpTracing.serverParser()); extractor = httpTracing.tracing().propagation().extractor(HttpServletRequest::getHeader); } @@ -42,19 +43,21 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha HttpServletResponse httpResponse = servlet.httpResponse(response); Span span = tracer.nextSpan(extractor, httpRequest); - try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) { - handler.handleReceive(httpRequest, span); // start the span + HttpServletAdapter.parseClientAddress(httpRequest, span); + handler.handleReceive(httpRequest, span); // start the span + Throwable error = null; + try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) { chain.doFilter(httpRequest, httpResponse); // any downstream filters see Tracer.currentSpan - + } catch (IOException | ServletException | RuntimeException e) { + error = e; + throw e; + } finally { if (servlet.isAsync(httpRequest)) { // we don't have the actual response, handle later servlet.handleAsync(handler, httpRequest, span); } else { // we have a synchronous response, so we can finish the span - handler.handleSend(httpResponse, span); + handler.handleSend(httpResponse, error, span); } - } catch (IOException | ServletException | RuntimeException e) { - handler.handleError(e, span); - throw e; } } diff --git a/instrumentation/sparkjava/src/main/java/brave/sparkjava/SparkTracing.java b/instrumentation/sparkjava/src/main/java/brave/sparkjava/SparkTracing.java index b9804998b4..a029ee1f25 100644 --- a/instrumentation/sparkjava/src/main/java/brave/sparkjava/SparkTracing.java +++ b/instrumentation/sparkjava/src/main/java/brave/sparkjava/SparkTracing.java @@ -3,9 +3,12 @@ import brave.Span; import brave.Tracer; import brave.Tracing; +import brave.http.HttpServerHandler; import brave.http.HttpTracing; import brave.propagation.TraceContext; -import brave.servlet.HttpServletHandler; +import brave.servlet.HttpServletAdapter; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import spark.ExceptionHandler; import spark.Filter; import spark.Request; @@ -20,18 +23,19 @@ public static SparkTracing create(HttpTracing httpTracing) { } final Tracer tracer; - final HttpServletHandler handler; + final HttpServerHandler handler; final TraceContext.Extractor extractor; SparkTracing(HttpTracing httpTracing) { tracer = httpTracing.tracing().tracer(); - handler = new HttpServletHandler(httpTracing.serverParser()); + handler = HttpServerHandler.create(new HttpServletAdapter(), httpTracing.serverParser()); extractor = httpTracing.tracing().propagation().extractor(Request::headers); } public Filter before() { return (request, response) -> { Span span = tracer.nextSpan(extractor, request); + HttpServletAdapter.parseClientAddress(request.raw(), span); handler.handleReceive(request.raw(), span); request.attribute(Tracer.SpanInScope.class.getName(), tracer.withSpanInScope(span)); }; @@ -42,14 +46,17 @@ public Filter afterAfter() { Span span = tracer.currentSpan(); if (span == null) return; ((Tracer.SpanInScope) request.attribute(Tracer.SpanInScope.class.getName())).close(); - handler.handleSend(response.raw(), span); + handler.handleSend(response.raw(), null, span); }; } public ExceptionHandler exception(ExceptionHandler delegate) { return (exception, request, response) -> { Span span = tracer.currentSpan(); - if (span != null) handler.handleError(exception, span); + if (span != null) { + ((Tracer.SpanInScope) request.attribute(Tracer.SpanInScope.class.getName())).close(); + handler.handleSend(null, exception, span); + } delegate.handle(exception, request, response); }; } diff --git a/instrumentation/spring-web/src/main/java/brave/spring/web/TracingClientHttpRequestInterceptor.java b/instrumentation/spring-web/src/main/java/brave/spring/web/TracingClientHttpRequestInterceptor.java index 6a70025e3b..d7398869dc 100644 --- a/instrumentation/spring-web/src/main/java/brave/spring/web/TracingClientHttpRequestInterceptor.java +++ b/instrumentation/spring-web/src/main/java/brave/spring/web/TracingClientHttpRequestInterceptor.java @@ -32,7 +32,7 @@ public static ClientHttpRequestInterceptor create(HttpTracing httpTracing) { TracingClientHttpRequestInterceptor(HttpTracing httpTracing) { tracer = httpTracing.tracing().tracer(); remoteServiceName = httpTracing.serverName(); - handler = new HttpClientHandler<>(new HttpAdapter(), httpTracing.clientParser()); + handler = HttpClientHandler.create(new HttpAdapter(), httpTracing.clientParser()); injector = httpTracing.tracing().propagation().injector(HttpHeaders::set); } @@ -40,13 +40,18 @@ public static ClientHttpRequestInterceptor create(HttpTracing httpTracing) { ClientHttpRequestExecution execution) throws IOException { Span span = tracer.nextSpan(); parseServerAddress(request, span); - handler.handleSend(request, span); injector.inject(span.context(), request.getHeaders()); + handler.handleSend(request, span); + + ClientHttpResponse response = null; + Throwable error = null; try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) { - return handler.handleReceive(execution.execute(request, body), span); + return response = execution.execute(request, body); } catch (IOException | RuntimeException e) { - handler.handleError(e, span); + error = e; throw e; + } finally { + handler.handleReceive(response, error, span); } } diff --git a/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingHandlerInterceptor.java b/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingHandlerInterceptor.java index 9c732b6a5f..48d46ae593 100644 --- a/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingHandlerInterceptor.java +++ b/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingHandlerInterceptor.java @@ -4,9 +4,10 @@ import brave.Tracer; import brave.Tracer.SpanInScope; import brave.Tracing; +import brave.http.HttpServerHandler; import brave.http.HttpTracing; import brave.propagation.TraceContext; -import brave.servlet.HttpServletHandler; +import brave.servlet.HttpServletAdapter; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.springframework.beans.factory.annotation.Autowired; @@ -25,42 +26,34 @@ public static HandlerInterceptorAdapter create(HttpTracing httpTracing) { } final Tracer tracer; - final HttpServletHandler handler; + final HttpServerHandler handler; final TraceContext.Extractor extractor; @Autowired TracingHandlerInterceptor(HttpTracing httpTracing) { // internal tracer = httpTracing.tracing().tracer(); - handler = new HttpServletHandler(httpTracing.serverParser()); + handler = HttpServerHandler.create(new HttpServletAdapter(), httpTracing.serverParser()); extractor = httpTracing.tracing().propagation().extractor(HttpServletRequest::getHeader); } @Override - public boolean preHandle(HttpServletRequest request, HttpServletResponse response, - Object handler) { + public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object o) { if (request.getAttribute(SpanInScope.class.getName()) != null) { return true; // already handled (possibly due to async request) } Span span = tracer.nextSpan(extractor, request); - try { - this.handler.handleReceive(request, span); - request.setAttribute(SpanInScope.class.getName(), tracer.withSpanInScope(span)); - } catch (RuntimeException e) { - throw this.handler.handleError(e, span); - } + HttpServletAdapter.parseClientAddress(request, span); + handler.handleReceive(request, span); + request.setAttribute(SpanInScope.class.getName(), tracer.withSpanInScope(span)); return true; } @Override public void afterCompletion(HttpServletRequest request, HttpServletResponse response, - Object handler, Exception ex) { + Object o, Exception ex) { Span span = tracer.currentSpan(); if (span == null) return; ((SpanInScope) request.getAttribute(SpanInScope.class.getName())).close(); - if (ex != null) { - this.handler.handleError(ex, span); - } else { - this.handler.handleSend(response, span); - } + handler.handleSend(response, ex, span); } }