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..f07c6886ec 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,48 +262,65 @@ 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 */ + /** If http route is supported, then the span name should include it */ @Test public void notFound() throws Exception { - httpTracing = httpTracing.toBuilder().serverParser(nameIsRoutePlusUrl).build(); - init(); - - assertThat(maybeGet("/foo/bark").code()) + assertThat(call("GET", "/foo/bark").code()) .isEqualTo(404); Span span = takeSpan(); // verify normal tags assertThat(span.tags()) + .hasSize(4) + .containsEntry("http.method", "GET") .containsEntry("http.path", "/foo/bark") - .containsEntry("http.status_code", "404"); + .containsEntry("http.status_code", "404") + .containsKey("error"); // as 404 is an error + + // Either the span name is the method, or it is a route expression + String name = span.name(); + if (name != null && !"get".equals(name)) { + assertThat(name).isEqualTo("get not_found"); + } + } - // 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 + /** + * This tests both that a root path ends up as "/" (slash) not "" (empty), as well that less + * typical OPTIONS methods can be traced. + */ + @Test public void options() throws Exception { + assertThat(call("OPTIONS", "/").isSuccessful()) + .isTrue(); + + Span span = takeSpan(); + + // verify normal tags + assertThat(span.tags()) + .hasSize(2) + .containsEntry("http.method", "OPTIONS") + .containsEntry("http.path", "/"); + + // Either the span name is the method, or it is a route expression String name = span.name(); - if (name != null) assertThat(name).isEqualTo("get"); + if (name != null && !"options".equals(name)) { + assertThat(name).isEqualTo("options /"); + } } @Test @@ -360,7 +377,7 @@ protected Response get(String path) throws Exception { } protected Response get(Request request) throws Exception { - Response response = maybeGet(request); + Response response = call(request); if (response.code() == 404) { // TODO: jetty isn't registering the tracing filter for all paths! spans.poll(100, TimeUnit.MILLISECONDS); @@ -370,12 +387,12 @@ protected Response get(Request request) throws Exception { } /** like {@link #get(String)} except doesn't throw unsupported on not found */ - Response maybeGet(String path) throws IOException { - return maybeGet(new Request.Builder().url(url(path)).build()); + Response call(String method, String path) throws IOException { + return call(new Request.Builder().method(method, null).url(url(path)).build()); } /** like {@link #get(Request)} except doesn't throw unsupported on not found */ - Response maybeGet(Request request) throws IOException { + Response call(Request request) throws IOException { try (Response response = client.newCall(request).execute()) { if (!HttpHeaders.hasBody(response)) return response; 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/jaxrs2/src/test/java/brave/jaxrs2/ITTracingFeature_Container.java b/instrumentation/jaxrs2/src/test/java/brave/jaxrs2/ITTracingFeature_Container.java index 95bf9a3a6d..977aa63759 100644 --- a/instrumentation/jaxrs2/src/test/java/brave/jaxrs2/ITTracingFeature_Container.java +++ b/instrumentation/jaxrs2/src/test/java/brave/jaxrs2/ITTracingFeature_Container.java @@ -9,6 +9,7 @@ import java.io.IOException; import java.lang.reflect.Method; import javax.ws.rs.GET; +import javax.ws.rs.OPTIONS; import javax.ws.rs.Path; import javax.ws.rs.container.AsyncResponse; import javax.ws.rs.container.Suspended; @@ -36,6 +37,12 @@ public static class TestResource { // public for resteasy to inject this.tracer = httpTracing.tracing().tracer(); } + @OPTIONS + @Path("") + public Response root() { + return Response.ok().build(); + } + @GET @Path("foo") public Response foo() { 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 HttpServerAdapter ""); Spark.get("/foo", (req, res) -> "bar"); Spark.get("/extra", (req, res) -> ExtraFieldPropagation.get(EXTRA_KEY)); Spark.get("/badrequest", (req, res) -> { 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 546c821e62..38d8871769 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 @@ -72,7 +72,7 @@ static final class HttpAdapter @Override public String requestHeader(HttpRequest request, String name) { Object result = request.getHeaders().getFirst(name); - return result != null ? result.toString() : null; + return result != null ? result.toString() : ""; } @Override public Integer statusCode(ClientHttpResponse response) { diff --git a/instrumentation/spring-webmvc/src/it/servlet25/pom.xml b/instrumentation/spring-webmvc/src/it/servlet25/pom.xml index bbfeb17600..d0aa9067ea 100644 --- a/instrumentation/spring-webmvc/src/it/servlet25/pom.xml +++ b/instrumentation/spring-webmvc/src/it/servlet25/pom.xml @@ -58,6 +58,12 @@ assertj-core @assertj.version@ + + + org.mockito + mockito-all + @mockito.version@ + diff --git a/instrumentation/spring-webmvc/src/it/servlet25/src/test/java/brave/spring/servlet25/ITTracingHandlerInterceptor.java b/instrumentation/spring-webmvc/src/it/servlet25/src/test/java/brave/spring/servlet25/ITTracingHandlerInterceptor.java index 280b7f735c..34bb1f1ae3 100644 --- a/instrumentation/spring-webmvc/src/it/servlet25/src/test/java/brave/spring/servlet25/ITTracingHandlerInterceptor.java +++ b/instrumentation/spring-webmvc/src/it/servlet25/src/test/java/brave/spring/servlet25/ITTracingHandlerInterceptor.java @@ -19,4 +19,10 @@ public class ITTracingHandlerInterceptor extends brave.spring.webmvc.ITTracingHa @Override public void addsStatusCode_badRequest() throws Exception { super.addsStatusCode_badRequest(); } + + /** TODO: Options dispatch isn't working on servlet 2.5 */ + @Test(expected = AssertionError.class) + @Override public void options() throws Exception { + super.options(); + } } 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/ITTracingAsyncHandlerInterceptor.java b/instrumentation/spring-webmvc/src/test/java/brave/spring/webmvc/ITTracingAsyncHandlerInterceptor.java index 3ba587a205..586f428442 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 @@ -18,6 +18,7 @@ import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; import org.springframework.web.servlet.AsyncHandlerInterceptor; import org.springframework.web.servlet.DispatcherServlet; @@ -38,6 +39,11 @@ public class ITTracingAsyncHandlerInterceptor extends ITServletContainer { this.tracer = httpTracing.tracing().tracer(); } + @RequestMapping(method = RequestMethod.OPTIONS, value = "/") + public ResponseEntity root() { + return new ResponseEntity<>(HttpStatus.OK); + } + @RequestMapping(value = "/foo") public ResponseEntity foo() { return new ResponseEntity<>(HttpStatus.OK); @@ -120,6 +126,8 @@ public void addInterceptors(InterceptorRegistry registry) { appContext.register(TestController.class); // the test resource appContext.register(TracingConfig.class); // generic tracing setup - handler.addServlet(new ServletHolder(new DispatcherServlet(appContext)), "/*"); + DispatcherServlet servlet = new DispatcherServlet(appContext); + servlet.setDispatchOptionsRequest(true); + handler.addServlet(new ServletHolder(servlet), "/*"); } } 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 77eb0c20ea..e5bd274377 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 @@ -18,6 +18,7 @@ import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; import org.springframework.web.servlet.DispatcherServlet; import org.springframework.web.servlet.HandlerInterceptor; @@ -38,18 +39,23 @@ public class ITTracingHandlerInterceptor extends ITServletContainer { this.tracer = httpTracing.tracing().tracer(); } + @RequestMapping(method = RequestMethod.OPTIONS, value = "/") + public ResponseEntity root() { + return new ResponseEntity<>(HttpStatus.OK); + } + @RequestMapping(value = "/foo") - public ResponseEntity foo() throws IOException { + public ResponseEntity foo() { return new ResponseEntity<>(HttpStatus.OK); } @RequestMapping(value = "/extra") - public ResponseEntity extra() throws IOException { + public ResponseEntity extra() { return new ResponseEntity<>(ExtraFieldPropagation.get(EXTRA_KEY), HttpStatus.OK); } @RequestMapping(value = "/badrequest") - public ResponseEntity badrequest() throws IOException { + public ResponseEntity badrequest() { return new ResponseEntity<>(HttpStatus.BAD_REQUEST); } @@ -60,7 +66,7 @@ public ResponseEntity child() { } @RequestMapping(value = "/async") - public Callable> async() throws IOException { + public Callable> async() { return () -> new ResponseEntity<>(HttpStatus.OK); } @@ -70,7 +76,7 @@ public ResponseEntity disconnect() throws IOException { } @RequestMapping(value = "/exceptionAsync") - public Callable> disconnectAsync() throws IOException { + public Callable> disconnectAsync() { return () -> { throw new IOException(); }; @@ -120,6 +126,8 @@ public void addInterceptors(InterceptorRegistry registry) { appContext.register(TestController.class); // the test resource appContext.register(TracingConfig.class); // generic tracing setup - handler.addServlet(new ServletHolder(new DispatcherServlet(appContext)), "/*"); + DispatcherServlet servlet = new DispatcherServlet(appContext); + servlet.setDispatchOptionsRequest(true); + handler.addServlet(new ServletHolder(servlet), "/*"); } } 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 { + ctx.response().end("bar"); + }); router.route("/foo").handler(ctx -> { ctx.response().end("bar"); }); diff --git a/instrumentation/vertx-web/src/test/java/brave/vertx/web/TracingRoutingContextHandlerAdapterTest.java b/instrumentation/vertx-web/src/test/java/brave/vertx/web/TracingRoutingContextHandlerAdapterTest.java new file mode 100644 index 0000000000..08ec4e9bb3 --- /dev/null +++ b/instrumentation/vertx-web/src/test/java/brave/vertx/web/TracingRoutingContextHandlerAdapterTest.java @@ -0,0 +1,40 @@ +package brave.vertx.web; + +import brave.vertx.web.TracingRoutingContextHandler.Adapter; +import brave.vertx.web.TracingRoutingContextHandler.Route; +import io.vertx.core.http.HttpServerResponse; +import org.junit.After; +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 TracingRoutingContextHandlerAdapterTest { + ThreadLocal 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 +