Adds HttpAdapter.methodFromResponse for route-based span names (#620)

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.

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 <Req> void request(HttpAdapter<Req, ?> 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<String> routes = new LinkedHashSet<>(Arrays.asList(span1.name(), span2.name()));
-    assertThat(routes).hasSize(1);
-    assertThat(routes.iterator().next())
-        .startsWith(prefix)
+    Set<String> 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 <Req> void request(HttpAdapter<Req, ?> 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 <Resp> void response(HttpAdapter<?, Resp> 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;
 

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

instrumentation/http/src/main/java/brave/http/HttpAdapter.java

@@ -17,6 +17,7 @@ public abstract class HttpAdapter<Req, Resp> {
    * "/objects/abcd-ff"
    *
    * <p>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<Req, Resp> {
    */
   @Nullable public abstract String requestHeader(Req request, String name);
 
+  /**
+   * Like {@link #method(Object)} except used in response parsing.
+   *
+   * <p>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)

instrumentation/http/src/main/java/brave/http/HttpParser.java

@@ -37,11 +37,12 @@ protected <Req> String spanName(HttpAdapter<Req, ?> adapter, Req req) {
    *
    * <p>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").
    *
-   * <p>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.
+   * <p>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.
    *
    * <p>If you only want to change how exceptions are parsed, override {@link #error(Integer,
    * Throwable, SpanCustomizer)} instead.
@@ -57,7 +58,7 @@ public <Resp> void response(HttpAdapter<?, Resp> 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 <Resp> void response(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
     error(statusCode, error, customizer);
   }
 
-  static String spanNameFromRoute(String route, int statusCode) {
+  static <Resp> String spanNameFromRoute(HttpAdapter<?, Resp> 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
   }
 

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));
+  }
 }

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() {

instrumentation/jersey-server/src/main/java/brave/jersey/server/TracingApplicationEventListener.java

@@ -79,6 +79,10 @@ static final class Adapter extends HttpServerAdapter<ContainerRequest, Container
       return request.getHeaderString(name);
     }
 
+    @Override public String methodFromResponse(ContainerResponse response) {
+      return response.getRequestContext().getMethod();
+    }
+
     /**
      * This returns the matched template as defined by a base URL and path expressions.
      *

instrumentation/jersey-server/src/test/java/brave/jersey/server/ITTracingApplicationEventListener.java

@@ -6,6 +6,7 @@
 import brave.propagation.ExtraFieldPropagation;
 import java.io.IOException;
 import javax.ws.rs.GET;
+import javax.ws.rs.OPTIONS;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
 import javax.ws.rs.container.AsyncResponse;
@@ -47,6 +48,12 @@ public static class TestResource {
       this.tracer = httpTracing.tracing().tracer();
     }
 
+    @OPTIONS
+    @Path("")
+    public Response root() {
+      return Response.ok().build();
+    }
+
     @GET
     @Path("foo")
     public Response foo() {

instrumentation/jersey-server/src/test/java/brave/jersey/server/TracingApplicationEventListenerAdapterTest.java

@@ -22,6 +22,14 @@ public class TracingApplicationEventListenerAdapterTest {
   @Mock ContainerRequest request;
   @Mock ContainerResponse response;
 
+  @Test public void methodFromResponse() {
+    when(response.getRequestContext()).thenReturn(request);
+    when(request.getMethod()).thenReturn("GET");
+
+    assertThat(adapter.methodFromResponse(response))
+        .isEqualTo("GET");
+  }
+
   @Test public void path_prefixesSlashWhenMissing() {
     when(request.getPath(false)).thenReturn("bar");
 

instrumentation/netty-codec-http/src/test/java/brave/netty/http/ITNettyHttpTracing.java

@@ -46,7 +46,7 @@ protected String url(String path) {
     return "http://127.0.0.1:" + port + path;
   }
 
-  @After public void stop() throws Exception {
+  @After public void stop() {
     if (bossGroup != null) bossGroup.shutdownGracefully();
     if (workerGroup != null) workerGroup.shutdownGracefully();
   }