From db3d46e42b9a67c9c2deffe1042bd4ca6cac816d Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 21 Feb 2018 15:42:21 +0800 Subject: [PATCH] more goodies --- .../main/java/brave/http/ITHttpServer.java | 109 +++++++++++++----- .../java/brave/http/ITServlet25Container.java | 1 + .../TracingApplicationEventListener.java | 20 ++++ .../ITTracingApplicationEventListener.java | 27 ++++- .../java/brave/netty/http/TestHandler.java | 12 +- .../webmvc25/ITTracingHandlerInterceptor.java | 14 ++- .../ITTracingAsyncHandlerInterceptor.java | 16 ++- .../webmvc/ITTracingHandlerInterceptor.java | 14 +++ .../web/TracingRoutingContextHandler.java | 3 +- .../brave/vertx/web/ITVertxWebTracing.java | 18 +++ 10 files changed, 190 insertions(+), 44 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 090371a8ec..3c632ec54a 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,7 @@ import brave.SpanCustomizer; import brave.propagation.ExtraFieldPropagation; import brave.sampler.Sampler; +import java.io.IOException; import java.util.Arrays; import java.util.LinkedHashSet; import java.util.Set; @@ -222,45 +223,84 @@ public void request(HttpAdapter adapter, Req req, SpanCustomizer c * framework specific, ex "/items/:itemId" in vert.x */ @Test - public void supportsHttpTemplate() throws Exception { - httpTracing = httpTracing.toBuilder().serverParser(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 - } + public void httpRoute() throws Exception { + httpTracing = httpTracing.toBuilder().serverParser(nameIsRoutePlusUrl).build(); + init(); - @Override public void response(HttpAdapter adapter, Resp res, Throwable error, - SpanCustomizer customizer) { - super.response(adapter, res, error, customizer); - String template = adapter.route(res); - if (template != null) customizer.name(template); - } - }).build(); + routeRequestsMatchPrefix("/items"); + } + + /** + * The "/nested/items/{itemId}" endpoint should be implemented by two route expressions: + * A path prefix: "/nested" and then a relative expression "/items/{itemId}" + */ + @Test + public void httpRoute_nested() throws Exception { + httpTracing = httpTracing.toBuilder().serverParser(nameIsRoutePlusUrl).build(); init(); + routeRequestsMatchPrefix("/nested/items"); + } + + private void routeRequestsMatchPrefix(String prefix) throws Exception { // Reading the route parameter from the response ensures the test endpoint is correct - assertThat(get("/items/1?foo").body().string()) + assertThat(get(prefix + "/1?foo").body().string()) .isEqualTo("1"); - assertThat(get("/items/2?bar").body().string()) + assertThat(get(prefix + "/2?bar").body().string()) .isEqualTo("2"); Span span1 = takeSpan(), span2 = takeSpan(); // verify that the path and url reflect the initial request (not a route expression) assertThat(span1.tags()) - .containsEntry("http.path", "/items/1") - .containsEntry("http.url", url("/items/1?foo")); + .containsEntry("http.path", prefix + "/1") + .containsEntry("http.url", url(prefix + "/1?foo")); assertThat(span2.tags()) - .containsEntry("http.path", "/items/2") - .containsEntry("http.url", url("/items/2?bar")); + .containsEntry("http.path", prefix + "/2") + .containsEntry("http.url", url(prefix + "/2?bar")); // 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 templates = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name())); - assertThat(templates).hasSize(1); - assertThat(templates.iterator().next()) - .contains("items"); + Set routes = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name())); + assertThat(routes).hasSize(1); + assertThat(routes.iterator().next()) + .startsWith(prefix); + } + + final HttpServerParser nameIsRoutePlusUrl = 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(); + init(); + + assertThat(maybeGet("/foo/bark").code()) + .isEqualTo(404); + + Span span = takeSpan(); + + // verify normal tags + assertThat(span.tags()) + .containsEntry("http.path", "/foo/bark") + .containsEntry("http.status_code", "404"); + + // 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"); } @Test @@ -317,12 +357,23 @@ protected Response get(String path) throws Exception { } protected Response get(Request request) throws Exception { + Response response = maybeGet(request); + if (response.code() == 404) { + // TODO: jetty isn't registering the tracing filter for all paths! + spans.poll(100, TimeUnit.MILLISECONDS); + throw new AssumptionViolatedException(request.url().encodedPath() + " not supported"); + } + return response; + } + + /** 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()); + } + + /** like {@link #get(Request)} except doesn't throw unsupported on not found */ + Response maybeGet(Request request) throws IOException { try (Response response = client.newCall(request).execute()) { - if (response.code() == 404) { - // TODO: jetty isn't registering the tracing filter for all paths! - spans.poll(100, TimeUnit.MILLISECONDS); - throw new AssumptionViolatedException(request.url().encodedPath() + " not supported"); - } if (!HttpHeaders.hasBody(response)) return response; // buffer response so tests can read it. Otherwise the finally block will drop it diff --git a/instrumentation/http-tests/src/main/java/brave/http/ITServlet25Container.java b/instrumentation/http-tests/src/main/java/brave/http/ITServlet25Container.java index a1bdd22caf..934c511db0 100644 --- a/instrumentation/http-tests/src/main/java/brave/http/ITServlet25Container.java +++ b/instrumentation/http-tests/src/main/java/brave/http/ITServlet25Container.java @@ -97,6 +97,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha @Override public void init(ServletContextHandler handler) { // add servlets for the test resource + handler.addServlet(new ServletHolder(new StatusServlet(404)), "/*"); handler.addServlet(new ServletHolder(new StatusServlet(200)), "/foo"); handler.addServlet(new ServletHolder(new ExtraServlet()), "/extra"); handler.addServlet(new ServletHolder(new StatusServlet(400)), "/badrequest"); 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 c82e7d89fa..af0a13882d 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 @@ -7,15 +7,20 @@ import brave.http.HttpTracing; import brave.propagation.Propagation.Getter; import brave.propagation.TraceContext; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import javax.inject.Inject; import javax.ws.rs.ext.Provider; import org.glassfish.jersey.server.ContainerRequest; import org.glassfish.jersey.server.ContainerResponse; +import org.glassfish.jersey.server.ExtendedUriInfo; import org.glassfish.jersey.server.ManagedAsync; import org.glassfish.jersey.server.monitoring.ApplicationEvent; import org.glassfish.jersey.server.monitoring.ApplicationEventListener; import org.glassfish.jersey.server.monitoring.RequestEvent; import org.glassfish.jersey.server.monitoring.RequestEventListener; +import org.glassfish.jersey.uri.UriTemplate; @Provider public final class TracingApplicationEventListener implements ApplicationEventListener { @@ -75,6 +80,21 @@ static final class Adapter extends HttpServerAdapter matchedTemplates = new ArrayList<>(uriInfo.getMatchedTemplates()); + if (matchedTemplates.size() > 1) { + Collections.reverse(matchedTemplates); + } + final StringBuilder sb = new StringBuilder(); + sb.append(uriInfo.getBaseUri().getPath()); + for (UriTemplate uriTemplate : matchedTemplates) { + sb.append(uriTemplate.getTemplate()); + } + return sb.toString().replaceAll("//+", "/"); + } + @Override public Integer statusCode(ContainerResponse response) { return response.getStatus(); } diff --git a/instrumentation/jersey-server/src/test/java/brave/jersey/server/ITTracingApplicationEventListener.java b/instrumentation/jersey-server/src/test/java/brave/jersey/server/ITTracingApplicationEventListener.java index b6f78b796d..56b54ffbe9 100644 --- a/instrumentation/jersey-server/src/test/java/brave/jersey/server/ITTracingApplicationEventListener.java +++ b/instrumentation/jersey-server/src/test/java/brave/jersey/server/ITTracingApplicationEventListener.java @@ -7,6 +7,7 @@ import java.io.IOException; import javax.ws.rs.GET; import javax.ws.rs.Path; +import javax.ws.rs.PathParam; import javax.ws.rs.container.AsyncResponse; import javax.ws.rs.container.Suspended; import javax.ws.rs.core.Response; @@ -73,15 +74,14 @@ public Response child() { @GET @Path("async") - public void async(@Suspended AsyncResponse response) throws IOException { - System.err.println("ASDFASDFASDFASDFASD " + Thread.currentThread()); + public void async(@Suspended AsyncResponse response) { new Thread(() -> response.resume("foo")).start(); } @GET @Path("managedAsync") @ManagedAsync - public void managedAsync(@Suspended AsyncResponse response) throws IOException { + public void managedAsync(@Suspended AsyncResponse response) { response.resume("foo"); } @@ -91,10 +91,29 @@ public Response disconnect() throws IOException { throw new IOException(); } + @GET + @Path("items/{itemId}") + public String item(@PathParam("itemId") String itemId) { + return itemId; + } + @GET @Path("exceptionAsync") - public void disconnectAsync(@Suspended AsyncResponse response) throws IOException { + public void disconnectAsync(@Suspended AsyncResponse response) { new Thread(() -> response.resume(new IOException())).start(); } + + public static class NestedResource { + @GET + @Path("items/{itemId}") + public String item(@PathParam("itemId") String itemId) { + return itemId; + } + } + + @Path("nested") + public NestedResource nestedResource() { + return new NestedResource(); + } } } diff --git a/instrumentation/netty-codec-http/src/test/java/brave/netty/http/TestHandler.java b/instrumentation/netty-codec-http/src/test/java/brave/netty/http/TestHandler.java index 8dcebdbb7e..6d5795377a 100644 --- a/instrumentation/netty-codec-http/src/test/java/brave/netty/http/TestHandler.java +++ b/instrumentation/netty-codec-http/src/test/java/brave/netty/http/TestHandler.java @@ -40,18 +40,18 @@ class TestHandler extends ChannelInboundHandlerAdapter { String uri = req.uri(); String content = null; HttpResponseStatus status = OK; - if (uri.startsWith("/foo")) { + if (uri.equals("/foo")) { content = "bar"; - } else if (uri.startsWith("/extra")) { + } else if (uri.equals("/extra")) { content = ExtraFieldPropagation.get(EXTRA_KEY); - } else if (uri.startsWith("/child")) { + } else if (uri.equals("/child")) { httpTracing.tracing().tracer().nextSpan().name("child").start().finish(); content = "happy"; - } else if (uri.startsWith("/exception")) { + } else if (uri.equals("/exception")) { throw new IOException("exception"); - } else if (uri.startsWith("/async")) { + } else if (uri.equals("/async")) { content = "async"; - } else if (uri.startsWith("/badrequest")) { + } else if (uri.equals("/badrequest")) { status = BAD_REQUEST; } else { status = NOT_FOUND; diff --git a/instrumentation/spring-webmvc/src/it/spring25/src/test/java/brave/spring/webmvc25/ITTracingHandlerInterceptor.java b/instrumentation/spring-webmvc/src/it/spring25/src/test/java/brave/spring/webmvc25/ITTracingHandlerInterceptor.java index 89cff23a3d..2fb749082f 100644 --- a/instrumentation/spring-webmvc/src/it/spring25/src/test/java/brave/spring/webmvc25/ITTracingHandlerInterceptor.java +++ b/instrumentation/spring-webmvc/src/it/spring25/src/test/java/brave/spring/webmvc25/ITTracingHandlerInterceptor.java @@ -8,7 +8,7 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; -import org.junit.Ignore; +import org.junit.AssumptionViolatedException; import org.springframework.beans.BeansException; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; @@ -24,8 +24,16 @@ public class ITTracingHandlerInterceptor extends ITServletContainer { - @Override @Ignore("HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE doesn't exist until Spring 3") - public void supportsHttpTemplate() { + @Override public void notFound(){ + throw new AssumptionViolatedException("TODO: add MVC handling for not found"); + } + + @Override public void httpRoute() { + throw new AssumptionViolatedException("HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE doesn't exist until Spring 3"); + } + + @Override public void httpRoute_nested() { + throw new AssumptionViolatedException("HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE doesn't exist until Spring 3"); } @Controller 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 8c27e9b023..3ba587a205 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 @@ -8,6 +8,7 @@ import java.util.concurrent.Callable; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; +import org.junit.AssumptionViolatedException; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.context.annotation.Bean; @@ -26,6 +27,10 @@ public class ITTracingAsyncHandlerInterceptor extends ITServletContainer { + @Override public void notFound(){ + throw new AssumptionViolatedException("TODO: add MVC handling for not found"); + } + @Controller static class TestController { final Tracer tracer; @@ -73,7 +78,16 @@ public Callable> disconnectAsync() { @RequestMapping(value = "/items/{itemId}") public ResponseEntity items(@PathVariable String itemId) { - return new ResponseEntity<>(itemId, HttpStatus.OK); + return new ResponseEntity(itemId, HttpStatus.OK); + } + + @Controller + @RequestMapping(value = "/nested") + static class NestedController { + @RequestMapping(value = "/items/{itemId}") + public ResponseEntity items(@PathVariable String itemId) { + return new ResponseEntity(itemId, HttpStatus.OK); + } } } 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 0990ea8666..1fb9955f56 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 @@ -8,6 +8,7 @@ import java.util.concurrent.Callable; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; +import org.junit.AssumptionViolatedException; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.context.annotation.Bean; @@ -26,6 +27,10 @@ public class ITTracingHandlerInterceptor extends ITServletContainer { + @Override public void notFound(){ + throw new AssumptionViolatedException("TODO: add MVC handling for not found"); + } + @Controller static class TestController { final Tracer tracer; @@ -75,6 +80,15 @@ public Callable> disconnectAsync() { public ResponseEntity items(@PathVariable String itemId) { return new ResponseEntity(itemId, HttpStatus.OK); } + + @Controller + @RequestMapping(value = "/nested") + static class NestedController { + @RequestMapping(value = "/items/{itemId}") + public ResponseEntity items(@PathVariable String itemId) { + return new ResponseEntity(itemId, 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 3394eae0d4..5cf8733b2c 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 @@ -131,7 +131,8 @@ static final class Adapter extends HttpServerAdapter { ctx.response().end(ctx.request().getParam("itemId")); }); + Router subrouter = Router.router(vertx); + subrouter.route("/items/:itemId").handler(ctx -> { + ctx.response().end(ctx.request().getParam("itemId")); + }); + router.mountSubRouter("/nested", subrouter); router.route("/exceptionAsync").handler(ctx -> { ctx.request().endHandler(v -> ctx.fail(new Exception())); }); @@ -90,6 +96,18 @@ public class ITVertxWebTracing extends ITHttpServer { handlesReroute("/rerouteAsync"); } + @Override @Test public void httpRoute_nested() throws Exception { + // Can't currently fully resolve the route template of a sub-router + // We get "/nested" not "/nested/items/:itemId + // https://groups.google.com/forum/?fromgroups#!topic/vertx/FtF2yVr5ZF8 + try { + super.httpRoute_nested(); + failBecauseExceptionWasNotThrown(AssertionError.class); + } catch (AssertionError e) { + assertThat(e.getMessage().contains("nested")); + } + } + void handlesReroute(String path) throws Exception { httpTracing = httpTracing.toBuilder().serverParser(new HttpServerParser() { @Override