From b235c5126e12a4e6e401474984be75c0d19304c6 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 12 Feb 2018 16:35:01 +0800 Subject: [PATCH] Adds HttpAdapter.template in support of "http.template" tag --- .../main/java/brave/http/ITHttpServer.java | 29 ++++++++++++++++++ .../src/main/java/brave/http/HttpAdapter.java | 20 +++++++++++++ .../java/brave/jaxrs2/ContainerAdapter.java | 5 ++++ .../brave/servlet/HttpServletAdapter.java | 3 +- .../TracingAsyncHandlerInterceptor.java | 11 ++++--- .../webmvc/TracingHandlerInterceptor.java | 30 +++++++++++++++++-- .../ITTracingAsyncHandlerInterceptor.java | 6 ++++ .../webmvc/ITTracingHandlerInterceptor.java | 6 ++++ .../web/TracingRoutingContextHandler.java | 26 ++++++++++++++-- .../brave/vertx/web/ITVertxWebTracing.java | 3 ++ 10 files changed, 126 insertions(+), 13 deletions(-) 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 cb9cef0f0d..b2e25370d4 100644 --- a/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java +++ b/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java @@ -3,6 +3,9 @@ import brave.SpanCustomizer; import brave.propagation.ExtraFieldPropagation; import brave.sampler.Sampler; +import java.util.Arrays; +import java.util.LinkedHashSet; +import java.util.Set; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; @@ -212,6 +215,32 @@ public void request(HttpAdapter adapter, Req req, SpanCustomizer c .containsEntry("context.visible", "true"); } + @Test + public void supportsHttpTemplate() throws Exception { + httpTracing = httpTracing.toBuilder().serverParser(new HttpServerParser() { + @Override public void response(HttpAdapter adapter, Resp res, Throwable error, + SpanCustomizer customizer) { + super.response(adapter, res, error, customizer); + String template = adapter.template(res); + if (template != null) customizer.name(template); + } + }).build(); + init(); + + get("/items/1?foo"); + get("/items/2?bar"); + + Span span1 = takeSpan(), span2 = takeSpan(); + assertThat(Arrays.asList(span1.tags().get("http.path"), span2.tags().get("http.path"))) + .containsExactly("/items/1", "/items/2"); + + Set templates = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name())); + + assertThat(templates).hasSize(1); + assertThat(templates.iterator().next()) + .contains("items"); + } + @Test public void addsStatusCode_badRequest() throws Exception { try { diff --git a/instrumentation/http/src/main/java/brave/http/HttpAdapter.java b/instrumentation/http/src/main/java/brave/http/HttpAdapter.java index 9e0792e4ac..591327f4c3 100644 --- a/instrumentation/http/src/main/java/brave/http/HttpAdapter.java +++ b/instrumentation/http/src/main/java/brave/http/HttpAdapter.java @@ -37,6 +37,26 @@ public abstract class HttpAdapter { */ @Nullable public abstract String requestHeader(Req request, String name); + /** + * An expression representing an application endpoint, used to group similar requests together. + * + *

For example, the template "/products/{key}", would match "/products/1" and "/products/2". + * There is no format required for the encoding, as it is sometimes application defined. The + * important part is that the value namespace is low cardinality. + * + *

Conventionally associated with the key "http.template" + * + *

Eventhough the template is associated with the request, not the response, this is present + * on the response object. The reasons is that many server implementations process the request + * before they can identify the route route. + */ + // BRAVE5: It isn't possible for a user to easily consume HttpServerAdapter, which is why this + // method, while generally about the server, is pushed up to the HttpAdapter. The signatures for + // sampling and parsing could be changed to make it more convenient. + @Nullable public String template(Resp response) { + return null; + } + /** * The HTTP status code or null if unreadable. * diff --git a/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/ContainerAdapter.java b/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/ContainerAdapter.java index c60ad5e2c2..5b650d913c 100644 --- a/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/ContainerAdapter.java +++ b/instrumentation/jaxrs2/src/main/java/brave/jaxrs2/ContainerAdapter.java @@ -38,6 +38,11 @@ public final class ContainerAdapter return request.getHeaderString(name); } + @Override public String template(ContainerResponseContext response) { + // There's no portable means to get the template eventhough there is a way in jersey2 + return null; + } + @Override public Integer statusCode(ContainerResponseContext response) { return response.getStatus(); } diff --git a/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java b/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java index edc3106ece..b21db88778 100644 --- a/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java +++ b/instrumentation/servlet/src/main/java/brave/servlet/HttpServletAdapter.java @@ -7,8 +7,7 @@ /** This can also parse the remote IP of the client. */ // public for others like sparkjava to use -public final class HttpServletAdapter - extends HttpServerAdapter { +public class HttpServletAdapter extends HttpServerAdapter { final ServletRuntime servlet = ServletRuntime.get(); /** diff --git a/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingAsyncHandlerInterceptor.java b/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingAsyncHandlerInterceptor.java index b140b5e3c6..b4600d4b99 100644 --- a/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingAsyncHandlerInterceptor.java +++ b/instrumentation/spring-webmvc/src/main/java/brave/spring/webmvc/TracingAsyncHandlerInterceptor.java @@ -22,21 +22,20 @@ public static AsyncHandlerInterceptor create(HttpTracing httpTracing) { return new TracingAsyncHandlerInterceptor(httpTracing); } - final HandlerInterceptor delegate; + final TracingHandlerInterceptor delegate; @Autowired TracingAsyncHandlerInterceptor(HttpTracing httpTracing) { // internal - delegate = TracingHandlerInterceptor.create(httpTracing); + delegate = new TracingHandlerInterceptor(httpTracing); } @Override - public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object o) - throws Exception { + public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object o) { return delegate.preHandle(request, response, o); } @Override - public void afterCompletion(HttpServletRequest request, HttpServletResponse response, - Object o, Exception ex) throws Exception { + public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object o, + Exception ex) { delegate.afterCompletion(request, response, o, ex); } } 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 5628add5f4..180282f290 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 @@ -17,6 +17,9 @@ /** Tracing interceptor for Spring Web MVC {@link HandlerInterceptor}. */ public final class TracingHandlerInterceptor implements HandlerInterceptor { + // redefined from HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE as doesn't exist until Spring 3 + static final String BEST_MATCHING_PATTERN_ATTRIBUTE = + "org.springframework.web.servlet.HandlerMapping.bestMatchingPattern"; static final Propagation.Getter GETTER = new Propagation.Getter() { @Override public String get(HttpServletRequest carrier, String key) { @@ -37,12 +40,23 @@ public static HandlerInterceptor create(HttpTracing httpTracing) { } final Tracer tracer; + final ThreadLocal currentTemplate; final HttpServerHandler handler; final TraceContext.Extractor extractor; @Autowired TracingHandlerInterceptor(HttpTracing httpTracing) { // internal tracer = httpTracing.tracing().tracer(); - handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter()); + currentTemplate = new ThreadLocal<>(); + handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter() { + @Override public String template(HttpServletResponse response) { + Object result = currentTemplate.get(); + return result != null ? result.toString() : null; + } + + @Override public String toString() { + return "WebMVCAdapter{}"; + } + }); extractor = httpTracing.tracing().propagation().extractor(GETTER); } @@ -59,7 +73,7 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons @Override public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, - ModelAndView modelAndView) throws Exception { + ModelAndView modelAndView) { } @Override @@ -68,6 +82,16 @@ public void afterCompletion(HttpServletRequest request, HttpServletResponse resp Span span = tracer.currentSpan(); if (span == null) return; ((SpanInScope) request.getAttribute(SpanInScope.class.getName())).close(); - handler.handleSend(response, ex, span); + Object template = request.getAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE); + if (template == null) { // skip thread-local overhead if there's no attribute + handler.handleSend(response, ex, span); + return; + } + try { + currentTemplate.set(template); + handler.handleSend(response, ex, span); + } finally { + currentTemplate.remove(); + } } } diff --git a/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingAsyncHandlerInterceptor.java b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingAsyncHandlerInterceptor.java index 1e47599e45..5b17307eb5 100644 --- a/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingAsyncHandlerInterceptor.java +++ b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingAsyncHandlerInterceptor.java @@ -15,6 +15,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; import org.springframework.web.servlet.AsyncHandlerInterceptor; @@ -69,6 +70,11 @@ public Callable> disconnectAsync() { throw new IOException(); }; } + + @RequestMapping(value = "/items/{itemId}") + public ResponseEntity items(@PathVariable String itemId) throws IOException { + return new ResponseEntity<>(HttpStatus.OK); + } } @Configuration diff --git a/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingHandlerInterceptor.java b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingHandlerInterceptor.java index 10670bc310..d51910f8d4 100644 --- a/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingHandlerInterceptor.java +++ b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingHandlerInterceptor.java @@ -15,6 +15,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; import org.springframework.web.servlet.DispatcherServlet; @@ -69,6 +70,11 @@ public Callable> disconnectAsync() throws IOException { throw new IOException(); }; } + + @RequestMapping(value = "/items/{itemId}") + public ResponseEntity items(@PathVariable String itemId) throws IOException { + return new ResponseEntity<>(HttpStatus.OK); + } } @Configuration diff --git a/instrumentation/vertx-web/src/main/java/brave/vertx/web/TracingRoutingContextHandler.java b/instrumentation/vertx-web/src/main/java/brave/vertx/web/TracingRoutingContextHandler.java index ab5246a90f..65e1a87a3f 100644 --- a/instrumentation/vertx-web/src/main/java/brave/vertx/web/TracingRoutingContextHandler.java +++ b/instrumentation/vertx-web/src/main/java/brave/vertx/web/TracingRoutingContextHandler.java @@ -36,12 +36,14 @@ final class TracingRoutingContextHandler implements Handler { }; final Tracer tracer; + final ThreadLocal currentTemplate; final HttpServerHandler serverHandler; final TraceContext.Extractor extractor; TracingRoutingContextHandler(HttpTracing httpTracing) { tracer = httpTracing.tracing().tracer(); - serverHandler = HttpServerHandler.create(httpTracing, new Adapter()); + currentTemplate = new ThreadLocal<>(); + serverHandler = HttpServerHandler.create(httpTracing, new Adapter(currentTemplate)); extractor = httpTracing.tracing().propagation().extractor(GETTER); } @@ -80,11 +82,27 @@ class TracingHandler implements Handler { @Override public void handle(Void aVoid) { if (!context.request().isEnded()) return; - serverHandler.handleSend(context.response(), context.failure(), span); + String template = context.currentRoute().getPath(); + if (template == null) { // skip thread-local overhead if there's no attribute + serverHandler.handleSend(context.response(), context.failure(), span); + return; + } + try { + currentTemplate.set(template); + serverHandler.handleSend(context.response(), context.failure(), span); + } finally { + currentTemplate.remove(); + } } } static final class Adapter extends HttpServerAdapter { + final ThreadLocal currentTemplate; + + Adapter(ThreadLocal currentTemplate) { + this.currentTemplate = currentTemplate; + } + @Override public String method(HttpServerRequest request) { return request.method().name(); } @@ -101,6 +119,10 @@ static final class Adapter extends HttpServerAdapter { ctx.fail(new Exception()); }); + router.route("/items/:itemId").handler(ctx -> { + ctx.response().end("bar"); + }); router.route("/exceptionAsync").handler(ctx -> { ctx.request().endHandler(v -> ctx.fail(new Exception())); });