From 4c72334eb02f236f58f575c9d8473ffe4665495c Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 23 Feb 2018 14:34:15 +0800 Subject: [PATCH] Adds HttpAdapter.methodFromResponse for route-based span names Span names like "/users/:userId" now include the method like "delete /users/:userId" This changes the route-based naming convention to include the http method and fixes some glitches not detected before. To support this, we need to make the http method visible in response parsing phase. See https://github.com/openzipkin/zipkin/issues/1874#issuecomment-367914033 --- .../main/java/brave/http/ITHttpServer.java | 35 +++++++-------- instrumentation/http/README.md | 4 +- .../src/main/java/brave/http/HttpAdapter.java | 11 +++++ .../src/main/java/brave/http/HttpParser.java | 22 ++++++---- .../test/java/brave/http/HttpParserTest.java | 41 ++++++++++++++++++ .../TracingApplicationEventListener.java | 4 ++ ...ngApplicationEventListenerAdapterTest.java | 8 ++++ .../spring-webmvc/src/it/servlet25/pom.xml | 6 +++ .../spring-webmvc/src/it/spring25/pom.xml | 6 +++ .../webmvc/TracingHandlerInterceptor.java | 40 +++++++++-------- .../TracingHandlerInterceptorAdapterTest.java | 33 ++++++++++++++ .../web/TracingRoutingContextHandler.java | 43 ++++++++++++------- ...acingRoutingContextHandlerAdapterTest.java | 40 +++++++++++++++++ pom.xml | 11 ++--- 14 files changed, 235 insertions(+), 69 deletions(-) create mode 100644 instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/TracingHandlerInterceptorAdapterTest.java create mode 100644 instrumentation/vertx-web/src/test/java/brave/vertx/web/TracingRoutingContextHandlerAdapterTest.java 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 09892514a3..512574b7f2 100644 --- a/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java +++ b/instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java @@ -192,7 +192,7 @@ public void defaultSpanNameIsMethodNameOrRoute() throws Exception { Span span = takeSpan(); if (!span.name().equals("get")) { assertThat(span.name()) - .isEqualTo("/foo"); + .isEqualTo("get /foo"); } } @@ -223,10 +223,10 @@ public void request(HttpAdapter adapter, Req req, SpanCustomizer c */ @Test public void httpRoute() throws Exception { - httpTracing = httpTracing.toBuilder().serverParser(nameIsRoutePlusUrl).build(); + httpTracing = httpTracing.toBuilder().serverParser(addHttpUrlTag).build(); init(); - routeRequestsMatchPrefix("/items"); + routeBasedRequestNameIncludesPathPrefix("/items"); } /** @@ -235,13 +235,13 @@ public void httpRoute() throws Exception { */ @Test public void httpRoute_nested() throws Exception { - httpTracing = httpTracing.toBuilder().serverParser(nameIsRoutePlusUrl).build(); + httpTracing = httpTracing.toBuilder().serverParser(addHttpUrlTag).build(); init(); - routeRequestsMatchPrefix("/nested/items"); + routeBasedRequestNameIncludesPathPrefix("/nested/items"); } - private void routeRequestsMatchPrefix(String prefix) throws Exception { + private void routeBasedRequestNameIncludesPathPrefix(String prefix) throws Exception { // Reading the route parameter from the response ensures the test endpoint is correct assertThat(get(prefix + "/1?foo").body().string()) .isEqualTo("1"); @@ -262,32 +262,25 @@ private void routeRequestsMatchPrefix(String prefix) throws Exception { // We don't know the exact format of the http route as it is framework specific // However, we know that it should match both requests and include the common part of the path - Set routes = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name())); - assertThat(routes).hasSize(1); - assertThat(routes.iterator().next()) - .startsWith(prefix) + Set routeBasedNames = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name())); + assertThat(routeBasedNames).hasSize(1); + assertThat(routeBasedNames.iterator().next()) + .startsWith("get " + prefix) .doesNotEndWith("/") // no trailing slashes .doesNotContain("//"); // no duplicate slashes } - final HttpServerParser nameIsRoutePlusUrl = new HttpServerParser() { + final HttpServerParser addHttpUrlTag = new HttpServerParser() { @Override public void request(HttpAdapter adapter, Req req, SpanCustomizer customizer) { super.request(adapter, req, customizer); customizer.tag("http.url", adapter.url(req)); // just the path is logged by default } - - @Override public void response(HttpAdapter adapter, Resp res, Throwable error, - SpanCustomizer customizer) { - super.response(adapter, res, error, customizer); - String route = adapter.route(res); - if (route != null) customizer.name(route); - } }; /** If http route is supported, then it should return empty when there's no route found */ @Test public void notFound() throws Exception { - httpTracing = httpTracing.toBuilder().serverParser(nameIsRoutePlusUrl).build(); + httpTracing = httpTracing.toBuilder().serverParser(addHttpUrlTag).build(); init(); assertThat(maybeGet("/foo/bark").code()) @@ -303,7 +296,9 @@ public void request(HttpAdapter adapter, Req req, SpanCustomizer c // If route is supported, the name should be empty (zipkin2.Span.name coerces empty to null) // If there's a bug in route parsing, a path expression will end up as the span name String name = span.name(); - if (name != null) assertThat(name).isEqualTo("get"); + if (name != null && !"get".equals(name)) { + assertThat(name).isEqualTo("get not_found"); + } } @Test diff --git a/instrumentation/http/README.md b/instrumentation/http/README.md index 8f790559cf..3bf7252113 100644 --- a/instrumentation/http/README.md +++ b/instrumentation/http/README.md @@ -17,8 +17,8 @@ By default, the following are added to both http client and server spans: * "error", when there is an exception or status is >=400 * Remote IP and port information -A route based span name looks like "/users/{userId}", "not_found" or -"redirected". There's a longer section on Http Route later in this doc. +A route based name looks like "delete /users/{userId}", "post not_found" +or "get redirected". There's a longer section on Http Route later. Naming and tags are configurable in a library-agnostic way. For example, the same `HttpTracing` component configures OkHttp or Apache HttpClient diff --git a/instrumentation/http/src/main/java/brave/http/HttpAdapter.java b/instrumentation/http/src/main/java/brave/http/HttpAdapter.java index 0c6967e64d..00a4877346 100644 --- a/instrumentation/http/src/main/java/brave/http/HttpAdapter.java +++ b/instrumentation/http/src/main/java/brave/http/HttpAdapter.java @@ -17,6 +17,7 @@ public abstract class HttpAdapter { * "/objects/abcd-ff" * *

Conventionally associated with the key "http.path" + * @see #route(Object) */ @Nullable public String path(Req request) { String url = url(request); @@ -37,6 +38,16 @@ public abstract class HttpAdapter { */ @Nullable public abstract String requestHeader(Req request, String name); + /** + * Like {@link #method(Object)} except used in response parsing. + * + *

Notably, this is used to create a route-based span name. + */ + // FromResponse suffix is needed as you can't compile methods that only differ on generic params + @Nullable public String methodFromResponse(Resp resp) { + return null; + } + /** * Returns an expression such as "/items/:itemId" representing an application endpoint, * conventionally associated with the tag key "http.route". If no route matched, "" (empty string) diff --git a/instrumentation/http/src/main/java/brave/http/HttpParser.java b/instrumentation/http/src/main/java/brave/http/HttpParser.java index b9184dc099..148ddef511 100644 --- a/instrumentation/http/src/main/java/brave/http/HttpParser.java +++ b/instrumentation/http/src/main/java/brave/http/HttpParser.java @@ -37,11 +37,12 @@ protected String spanName(HttpAdapter adapter, Req req) { * *

By default, this tags "http.status_code" when it is not 2xx. If there's an exception or the * status code is neither 2xx nor 3xx, it tags "error". This also overrides the span name based on - * the {@link HttpAdapter#route(Object)} where possible. + * the {@link HttpAdapter#methodFromResponse(Object)} and {@link HttpAdapter#route(Object)} where + * possible (ex "get /users/:userId"). * - *

If routing is supported, but didn't match due to 404, the span name will be "not_found". If - * it didn't match due to redirect, the span name will be "redirected". If routing is not - * supported, the span name is left alone. + *

If routing is supported, and a GET didn't match due to 404, the span name will be + * "get not_found". If it didn't match due to redirect, the span name will be "get redirected". + * If routing is not supported, the span name is left alone. * *

If you only want to change how exceptions are parsed, override {@link #error(Integer, * Throwable, SpanCustomizer)} instead. @@ -57,7 +58,7 @@ public void response(HttpAdapter adapter, @Nullable Resp res, int statusCode = 0; if (res != null) { statusCode = adapter.statusCodeAsInt(res); - String nameFromRoute = spanNameFromRoute(adapter.route(res), statusCode); + String nameFromRoute = spanNameFromRoute(adapter, res, statusCode); if (nameFromRoute != null) customizer.name(nameFromRoute); if (statusCode != 0 && (statusCode < 200 || statusCode > 299)) { customizer.tag("http.status_code", String.valueOf(statusCode)); @@ -66,11 +67,14 @@ public void response(HttpAdapter adapter, @Nullable Resp res, error(statusCode, error, customizer); } - static String spanNameFromRoute(String route, int statusCode) { + static String spanNameFromRoute(HttpAdapter adapter, Resp res, int statusCode) { + String method = adapter.methodFromResponse(res); + if (method == null) return null; // don't undo a valid name elsewhere + String route = adapter.route(res); if (route == null) return null; // don't undo a valid name elsewhere - if (!"".equals(route)) return route; - if (statusCode / 100 == 3) return "redirected"; - if (statusCode == 404) return "not_found"; + if (!"".equals(route)) return method + " " + route; + if (statusCode / 100 == 3) return method + " redirected"; + if (statusCode == 404) return method + " not_found"; return null; // unexpected } diff --git a/instrumentation/http/src/test/java/brave/http/HttpParserTest.java b/instrumentation/http/src/test/java/brave/http/HttpParserTest.java index 8c294c6bb2..9f426e9ed9 100644 --- a/instrumentation/http/src/test/java/brave/http/HttpParserTest.java +++ b/instrumentation/http/src/test/java/brave/http/HttpParserTest.java @@ -7,6 +7,7 @@ import org.mockito.runners.MockitoJUnitRunner; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -81,4 +82,44 @@ public class HttpParserTest { verify(customizer).tag("error", "drat"); } + + @Test public void routeBasedName() { + when(adapter.methodFromResponse(response)).thenReturn("GET"); + when(adapter.route(response)).thenReturn("/users/:userId"); + when(adapter.statusCodeAsInt(response)).thenReturn(200); + + parser.response(adapter, response, null, customizer); + + verify(customizer).name("GET /users/:userId"); // zipkin will implicitly lowercase this + } + + @Test public void routeBasedName_redirect() { + when(adapter.methodFromResponse(response)).thenReturn("GET"); + when(adapter.route(response)).thenReturn(""); + when(adapter.statusCodeAsInt(response)).thenReturn(307); + + parser.response(adapter, response, null, customizer); + + verify(customizer).name("GET redirected"); // zipkin will implicitly lowercase this + } + + @Test public void routeBasedName_notFound() { + when(adapter.methodFromResponse(response)).thenReturn("DELETE"); + when(adapter.route(response)).thenReturn(""); + when(adapter.statusCodeAsInt(response)).thenReturn(404); + + parser.response(adapter, response, null, customizer); + + verify(customizer).name("DELETE not_found"); // zipkin will implicitly lowercase this + } + + @Test public void routeBasedName_skipsOnMissingData() { + when(adapter.methodFromResponse(response)).thenReturn("DELETE"); + when(adapter.route(response)).thenReturn(null); // missing! + when(adapter.statusCodeAsInt(response)).thenReturn(404); + + parser.response(adapter, response, null, customizer); + + verify(customizer, never()).name(any(String.class)); + } } diff --git a/instrumentation/jersey-server/src/main/java/brave/jersey/server/TracingApplicationEventListener.java b/instrumentation/jersey-server/src/main/java/brave/jersey/server/TracingApplicationEventListener.java index 7dd727d7cc..7214b79f25 100644 --- a/instrumentation/jersey-server/src/main/java/brave/jersey/server/TracingApplicationEventListener.java +++ b/instrumentation/jersey-server/src/main/java/brave/jersey/server/TracingApplicationEventListener.java @@ -79,6 +79,10 @@ static final class Adapter extends HttpServerAdapterassertj-core @assertj.version@ + + + org.mockito + mockito-all + @mockito.version@ + diff --git a/instrumentation/spring-webmvc/src/it/spring25/pom.xml b/instrumentation/spring-webmvc/src/it/spring25/pom.xml index 06112f8f55..80e1741c4f 100644 --- a/instrumentation/spring-webmvc/src/it/spring25/pom.xml +++ b/instrumentation/spring-webmvc/src/it/spring25/pom.xml @@ -58,6 +58,12 @@ assertj-core @assertj.version@ + + + org.mockito + mockito-all + @mockito.version@ + 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 3556b120f2..68b7c994d0 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 @@ -46,16 +46,7 @@ public static HandlerInterceptor create(HttpTracing httpTracing) { @Autowired TracingHandlerInterceptor(HttpTracing httpTracing) { // internal tracer = httpTracing.tracing().tracer(); - handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter() { - @Override public String route(HttpServletResponse response) { - return response instanceof HttpServletResponseWithTemplate - ? ((HttpServletResponseWithTemplate) response).template : null; - } - - @Override public String toString() { - return "WebMVCAdapter{}"; - } - }); + handler = HttpServerHandler.create(httpTracing, new Adapter()); extractor = httpTracing.tracing().propagation().extractor(GETTER); } @@ -82,18 +73,31 @@ public void afterCompletion(HttpServletRequest request, HttpServletResponse resp if (span == null) return; ((SpanInScope) request.getAttribute(SpanInScope.class.getName())).close(); Object template = request.getAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE); - if (template != null) { - response = new HttpServletResponseWithTemplate(response, template.toString()); - } - handler.handleSend(response, ex, span); + handler.handleSend(new DecoratedHttpServletResponse(response, request.getMethod(), template), + ex, span); } - static class HttpServletResponseWithTemplate extends HttpServletResponseWrapper { - final String template; + static class DecoratedHttpServletResponse extends HttpServletResponseWrapper { + final String method, template; - HttpServletResponseWithTemplate(HttpServletResponse response, String template) { + DecoratedHttpServletResponse(HttpServletResponse response, String method, Object template) { super(response); - this.template = template; + this.method = method; + this.template = template != null ? template.toString() : ""; + } + } + + static final class Adapter extends HttpServletAdapter { + @Override public String methodFromResponse(HttpServletResponse response) { + return ((DecoratedHttpServletResponse) response).method; + } + + @Override public String route(HttpServletResponse response) { + return ((DecoratedHttpServletResponse) response).template; + } + + @Override public String toString() { + return "WebMVCAdapter{}"; } } } diff --git a/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/TracingHandlerInterceptorAdapterTest.java b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/TracingHandlerInterceptorAdapterTest.java new file mode 100644 index 0000000000..510b5776da --- /dev/null +++ b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/TracingHandlerInterceptorAdapterTest.java @@ -0,0 +1,33 @@ +package brave.spring.webmvc; + +import brave.spring.webmvc.TracingHandlerInterceptor.Adapter; +import brave.spring.webmvc.TracingHandlerInterceptor.DecoratedHttpServletResponse; +import javax.servlet.http.HttpServletResponse; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; + +import static org.assertj.core.api.Assertions.assertThat; + +@RunWith(MockitoJUnitRunner.class) +public class TracingHandlerInterceptorAdapterTest { + Adapter adapter = new Adapter(); + @Mock HttpServletResponse response; + + @Test public void methodFromResponse() { + assertThat(adapter.methodFromResponse( + new DecoratedHttpServletResponse(response, "GET", null))) + .isEqualTo("GET"); + } + + @Test public void route_emptyByDefault() { + assertThat(adapter.route(new DecoratedHttpServletResponse(response, "GET", null))) + .isEmpty(); + } + + @Test public void route() { + assertThat(adapter.route(new DecoratedHttpServletResponse(response, "GET", "/users/:userID"))) + .isEqualTo("/users/:userID"); + } +} 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 9ea9d24eab..5ab4d66a8b 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 @@ -47,14 +47,14 @@ final class TracingRoutingContextHandler implements Handler { }; final Tracer tracer; - final ThreadLocal currentTemplate; + final ThreadLocal currentRoute; final HttpServerHandler serverHandler; final TraceContext.Extractor extractor; TracingRoutingContextHandler(HttpTracing httpTracing) { tracer = httpTracing.tracing().tracer(); - currentTemplate = new ThreadLocal<>(); - serverHandler = HttpServerHandler.create(httpTracing, new Adapter(currentTemplate)); + currentRoute = new ThreadLocal<>(); + serverHandler = HttpServerHandler.create(httpTracing, new Adapter(currentRoute)); extractor = httpTracing.tracing().propagation().extractor(GETTER); } @@ -93,29 +93,38 @@ class TracingHandler implements Handler { @Override public void handle(Void aVoid) { if (!context.request().isEnded()) return; - 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; - } + Route route = new Route(context.request().rawMethod(), context.currentRoute().getPath()); try { - currentTemplate.set(template); + currentRoute.set(route); serverHandler.handleSend(context.response(), context.failure(), span); } finally { - currentTemplate.remove(); + currentRoute.remove(); } } } + static final class Route { + final String method, path; + + Route(String method, String path) { + this.method = method; + this.path = path; + } + + @Override public String toString() { + return "Route{method=" + method + ", path=" + path + "}"; + } + } + static final class Adapter extends HttpServerAdapter { - final ThreadLocal currentTemplate; + final ThreadLocal currentRoute; - Adapter(ThreadLocal currentTemplate) { - this.currentTemplate = currentTemplate; + Adapter(ThreadLocal currentRoute) { + this.currentRoute = currentRoute; } @Override public String method(HttpServerRequest request) { - return request.method().name(); + return request.rawMethod(); } @Override public String path(HttpServerRequest request) { @@ -130,8 +139,12 @@ static final class Adapter extends HttpServerAdapter currentRoute = new ThreadLocal<>(); + Adapter adapter = new Adapter(currentRoute); + @Mock HttpServerResponse response; + + @After public void clear() { + currentRoute.remove(); + } + + @Test public void methodFromResponse() { + currentRoute.set(new Route("GET", null)); + assertThat(adapter.methodFromResponse(response)) + .isEqualTo("GET"); + } + + @Test public void route_emptyByDefault() { + currentRoute.set(new Route("GET", null)); + assertThat(adapter.route(response)).isEmpty(); + } + + @Test public void route() { + currentRoute.set(new Route("GET", "/users/:userID")); + assertThat(adapter.route(response)) + .isEqualTo("/users/:userID"); + } +} diff --git a/pom.xml b/pom.xml index 571ed2ac59..19975cbd1f 100755 --- a/pom.xml +++ b/pom.xml @@ -78,6 +78,7 @@ 4.12 3.9.0 2.0.0-beta.5 + 1.10.19 1.3.1 2.25.1 @@ -351,11 +352,11 @@ test - org.mockito - mockito-all - 1.10.19 - test - + org.mockito + mockito-all + ${mockito.version} + test +