Updates default naming policy to include route-based names

Author: Adrian Cole

instrumentation/http-tests/src/main/java/brave/http/ITHttpServer.java

@@ -186,20 +186,21 @@ public void reportsServerKindToZipkin() throws Exception {
   }
 
   @Test
-  public void defaultSpanNameIsMethodName() throws Exception {
+  public void defaultSpanNameIsMethodNameOrRoute() throws Exception {
     get("/foo");
 
     Span span = takeSpan();
-    assertThat(span.name())
-        .isEqualTo("get");
+    if (!span.name().equals("get")) {
+      assertThat(span.name())
+          .isEqualTo("/foo");
+    }
   }
 
   @Test
   public void supportsPortableCustomization() throws Exception {
     httpTracing = httpTracing.toBuilder().serverParser(new HttpServerParser() {
       @Override
       public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer customizer) {
-        customizer.name(adapter.method(req).toLowerCase() + " " + adapter.path(req));
         customizer.tag("http.url", adapter.url(req)); // just the path is logged by default
         customizer.tag("context.visible", String.valueOf(currentTraceContext.get() != null));
       }
@@ -210,8 +211,6 @@ public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer c
     get(uri);
 
     Span span = takeSpan();
-    assertThat(span.name())
-        .isEqualTo("get /foo");
     assertThat(span.tags())
         .containsEntry("http.url", url(uri))
         .containsEntry("context.visible", "true");
@@ -253,9 +252,11 @@ private void routeRequestsMatchPrefix(String prefix) throws Exception {
 
     // verify that the path and url reflect the initial request (not a route expression)
     assertThat(span1.tags())
+        .containsEntry("http.method", "GET")
         .containsEntry("http.path", prefix + "/1")
         .containsEntry("http.url", url(prefix + "/1?foo"));
     assertThat(span2.tags())
+        .containsEntry("http.method", "GET")
         .containsEntry("http.path", prefix + "/2")
         .containsEntry("http.url", url(prefix + "/2?bar"));
 

instrumentation/http/README.md

@@ -9,26 +9,29 @@ instructions on what to put into http spans, and sampling policy.
 
 ## Span data policy
 By default, the following are added to both http client and server spans:
-* Span.name as the http method in lowercase: ex "get"
+* Span.name is the http method in lowercase: ex "get" or a route described below
 * Tags/binary annotations:
+  * "http.method", eg "GET"
   * "http.path", which does not include query parameters.
   * "http.status_code" when the status us not success.
   * "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.
+
 Naming and tags are configurable in a library-agnostic way. For example,
 the same `HttpTracing` component configures OkHttp or Apache HttpClient
 identically.
 
-For example, to change the span and tag naming policy for clients, you
-can do something like this:
+For example, to change the tagging policy for clients, you can do
+something like this:
 
 ```java
 httpTracing = httpTracing.toBuilder()
     .clientParser(new HttpClientParser() {
       @Override
       public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer customizer) {
-        customizer.name(adapter.method(req).toLowerCase() + " " + adapter.path(req));
         customizer.tag(TraceKeys.HTTP_URL, adapter.url(req)); // the whole url, not just the path
       }
     })
@@ -85,6 +88,29 @@ httpTracingBuilder.serverSampler(HttpRuleSampler.newBuilder()
   .build());
 ```
 
+## Http Route
+The http route is an expression such as `/items/:itemId` representing an
+application endpoint. `HttpAdapter.route()` parses this from a response,
+returning the route that matched the request, empty if no route matched,
+or null if routes aren't supported. This value is either used to create
+a tag "http.route" or as an input to a span naming function.
+
+### Http route cardinality
+The http route groups similar requests together, so results in limited
+cardinality, often better choice for a span name vs the http method.
+
+For example, the route `/users/{userId}`, matches `/users/25f4c31d` and
+`/users/e3c553be`. If a span name function used the http path instead,
+it could DOS-style attack vector on your span name index, as it would
+grow unbounded vs `/users/{userId}`. Even if different frameworks use
+different formats, such as `/users/[0-9a-f]+` or `/users/:userId`, the
+cardinality is still fixed with regards to request count.
+
+The http route can can be empty on redirect or not-found. If you are
+using the route for metrics, coerce empty to constants like "redirected"
+or "not_found" using the http status. For example, the default span name
+policy does this.
+
 # Developing new instrumentation
 
 Check for [instrumentation written here](../instrumentation/) and [Zipkin's list](http://zipkin.io/pages/existing_instrumentations.html)
@@ -177,4 +203,76 @@ try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) { // 2.
 } finally {
   handler.handleSend(response, error, span); // 5.
 }
-```
+```
+
+### Supporting HttpAdapter.route(response)
+
+Eventhough the route is associated with the request, not the response,
+it is parsed from the response object. The reason is that many server
+implementations process the request before they can identify the route.
+
+Instrumentation authors implement support via extending HttpAdapter
+accordingly. There are a few patterns which might help.
+
+#### Callback with non-final type
+When a framework uses an callback model, you are in control of the type
+being parsed. If the response type isn't final, simply subclass it with
+the route data.
+
+For example, if spring MVC, it would be this:
+```java
+    // check for a wrapper type which holds the template
+    handler = HttpServerHandler.create(httpTracing, new HttpServletAdapter() {
+      @Override public String template(HttpServletResponse response) {
+        return response instanceof HttpServletResponseWithTemplate
+            ? ((HttpServletResponseWithTemplate) response).route : null;
+      }
+
+      @Override public String toString() {
+        return "WebMVCAdapter{}";
+      }
+    });
+
+--snip--
+
+// when parsing the response, scope the route from the request
+
+    Object template = request.getAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE);
+    if (route != null) {
+      response = new HttpServletResponseWithTemplate(response, route.toString());
+    }
+    handler.handleSend(response, ex, span);
+```
+
+#### Callback with final type
+If the response type is final, you may be able to make a copy and stash
+the route as a synthetic header. Since this is a copy of the response,
+there's no chance a user will receive this header in a real response.
+
+Here's an example for Play, where the header "brave-http-template" holds
+the route temporarily until the parser can read it.
+```scala
+    result.onComplete {
+      case Failure(t) => handler.handleSend(null, t, span)
+      case Success(r) => {
+        // add a synthetic header if there was a routing path set
+        var resp = template.map(t => r.withHeaders("brave-http-template" -> t)).getOrElse(r)
+        handler.handleSend(resp, null, span)
+      }
+    }
+--snip--
+    override def template(response: Result): String =
+      response.header.headers.apply("brave-http-template")
+```
+
+#### Common mistakes
+
+For grouping to work, we want routes that are effectively the same, to
+in fact be the same. Here are a couple things on that.
+
+* Always start with a leading slash
+  * This allows you to differentiate the root path from empty (no route)
+  * This prevents accidental partitioning like `users/:userId` from `/users/:userId`
+* Take care not to duplicate slashes
+  * When joining nested paths, avoid writing templates like `/nested//users/:userId`
+  * The `ITHttpServer` test will catch some of this

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

@@ -38,19 +38,9 @@ public abstract class HttpAdapter<Req, Resp> {
   @Nullable public abstract String requestHeader(Req request, String name);
 
   /**
-   * An expression such as "/items/:itemId" representing an application endpoint, conventionally
-   * associated with the tag key "http.route".
-   *
-   * <p>The http route groups similar requests together, so results in limited cardinality, often
-   * better choice for a span name vs the http method. However, the route can be absent on redirect
-   * or file-not-found. Also, not all frameworks include support for http route expressions,
-   * and some don't expose templates programmatically for readback.
-   *
-   * <p>For example, the route "/users/{userId}", matches "/users/25f4c31d" and "/users/e3c553be".
-   * If a span name function used the http path instead, it could DOS-style attack vector on your
-   * span name index, as it would grow unbounded vs "/users/{userId}". Even if different frameworks
-   * use different formats, like "/users/[0-9a-f]+" or "/users/:userId", the cardinality is still
-   * fixed with regards to request count.
+   * 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)
+   * is returned. Null indicates this instrumentation doesn't understand http routes.
    *
    * <p>Eventhough the route is associated with the request, not the response, this is present
    * on the response object. The reasons is that many server implementations process the request
@@ -70,6 +60,16 @@ public abstract class HttpAdapter<Req, Resp> {
    */
   @Nullable public abstract Integer statusCode(Resp response);
 
+  /**
+   * The HTTP status code or 0 if unreadable.
+   *
+   * <p>Conventionally associated with the key "http.status_code"
+   */
+  public int statusCodeAsInt(Resp response) {
+    Integer maybeStatus = statusCode(response);
+    return maybeStatus != null ? maybeStatus : 0;
+  }
+
   HttpAdapter() {
   }
 }

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

@@ -6,7 +6,7 @@
 public class HttpParser {
   /**
    * Override to change what data from the http request are parsed into the span representing it. By
-   * default, this sets the span name to the http method and tags "http.path"
+   * default, this sets the span name to the http method and tags "http.method" and "http.path".
    *
    * <p>If you only want to change the span name, you can override {@link #spanName(HttpAdapter,
    * Object)} instead.
@@ -15,6 +15,8 @@ public class HttpParser {
    */
   public <Req> void request(HttpAdapter<Req, ?> adapter, Req req, SpanCustomizer customizer) {
     customizer.name(spanName(adapter, req));
+    String method = adapter.method(req);
+    if (method != null) customizer.tag("http.method", method);
     String path = adapter.path(req);
     if (path != null) customizer.tag("http.path", path);
   }
@@ -26,8 +28,15 @@ protected <Req> String spanName(HttpAdapter<Req, ?> adapter, Req req) {
 
   /**
    * Override to change what data from the http response or error are parsed into the span modeling
-   * it. 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".
+   * it.
+   *
+   * <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.
+   *
+   * <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 you only want to change how exceptions are parsed, override {@link #error(Integer,
    * Throwable, SpanCustomizer)} instead.
@@ -40,11 +49,40 @@ protected <Req> String spanName(HttpAdapter<Req, ?> adapter, Req req) {
   // If this were not an abstraction, we'd use separate hooks for response and error.
   public <Resp> void response(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
       @Nullable Throwable error, SpanCustomizer customizer) {
-    Integer httpStatus = res != null ? adapter.statusCode(res) : null;
-    if (httpStatus != null && (httpStatus < 200 || httpStatus > 299)) {
-      customizer.tag("http.status_code", String.valueOf(httpStatus));
+    int statusCode = 0;
+    if (res != null) {
+      statusCode = adapter.statusCodeAsInt(res);
+      String route = adapter.route(res);
+      if (route != null) {
+        if (!"".equals(route)) {
+          customizer.name(route);
+        } else if (statusCode / 100 == 3) {
+          customizer.name("redirected");
+        } else if (statusCode == 404) {
+          customizer.name("not_found");
+        }
+      }
+      if (statusCode != 0 && (statusCode < 200 || statusCode > 299)) {
+        customizer.tag("http.status_code", String.valueOf(statusCode));
+      }
     }
-    error(httpStatus, error, customizer);
+    error(statusCode, error, customizer);
+  }
+
+  /**
+   * Returns a span name according to the {@link HttpAdapter#route(Object)}, or the constant
+   * "redirected" or "not_found". Returns null if routing isn't supported or there was no http
+   * status available.
+   */
+  @Nullable protected <Resp> String spanNameFromRoute(HttpAdapter<?, Resp> adapter, Resp res) {
+    String route = adapter.route(res);
+    if (route == null) return null;
+    if (!"".equals(route)) return route;
+    Integer statusCode = adapter.statusCode(res);
+    if (statusCode == null) return null;
+    if (statusCode / 100 == 3) return "redirected";
+    if (statusCode == 404) return "not_found";
+    return null;
   }
 
   /**
@@ -61,7 +99,7 @@ protected void error(@Nullable Integer httpStatus, @Nullable Throwable error,
     if (error != null) {
       message = error.getMessage();
       if (message == null) message = error.getClass().getSimpleName();
-    } else if (httpStatus != null) {
+    } else if (httpStatus != null && httpStatus != 0) {
       message = httpStatus < 200 || httpStatus > 399 ? String.valueOf(httpStatus) : null;
     }
     if (message != null) customizer.tag("error", message);