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 openzipkin/zipkin#1874 (comment)

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,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<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 */
   @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 <Req> void request(HttpAdapter<Req, ?> 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

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/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/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/spring-webmvc/src/it/servlet25/pom.xml

@@ -58,6 +58,12 @@
       <artifactId>assertj-core</artifactId>
       <version>@assertj.version@</version>
     </dependency>
+
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-all</artifactId>
+      <version>@mockito.version@</version>
+    </dependency>
   </dependencies>
 
   <build>

instrumentation/spring-webmvc/src/it/spring25/pom.xml

@@ -58,6 +58,12 @@
       <artifactId>assertj-core</artifactId>
       <version>@assertj.version@</version>
     </dependency>
+
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-all</artifactId>
+      <version>@mockito.version@</version>
+    </dependency>
   </dependencies>
 
   <build>

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{}";
     }
   }
 }

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