Defined http.route key and ensures server span name is preferred (#1924)

Especially when using http route based span names, we have to be careful
to not pick a bad name. In shared spans, for example, the UI prefers
server-side's name of the service. However, we have no name priority
logic, so it is arbitrary. This ensures the server's span name wins in a
race.

See #1874

zipkin/src/main/java/zipkin/Span.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2015-2017 The OpenZipkin Authors
+ * Copyright 2015-2018 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -237,9 +237,6 @@ public Builder merge(Span that) {
       if (this.traceIdHigh == null || this.traceIdHigh == 0) {
         this.traceIdHigh = that.traceIdHigh;
       }
-      if (this.name == null || this.name.length() == 0 || this.name.equals("unknown")) {
-        this.name = that.name;
-      }
       if (this.id == null) {
         this.id = that.id;
       }
@@ -248,20 +245,26 @@ public Builder merge(Span that) {
       }
 
       // When we move to span model 2, remove this code in favor of using Span.kind == CLIENT
-      boolean thisIsClientSpan = this.isClientSpan;
-      boolean thatIsClientSpan = false;
+      boolean thisIsClientSpan = isClientSpan, thatIsClientSpan = false, thatIsServerSpan = false;
 
       // This guards to ensure we don't add duplicate annotations or binary annotations on merge
       if (!that.annotations.isEmpty()) {
         boolean thisHadNoAnnotations = this.annotations == null;
         for (Annotation a : that.annotations) {
           if (a.value.equals(Constants.CLIENT_SEND)) thatIsClientSpan = true;
+          if (a.value.equals(Constants.SERVER_RECV)) thatIsServerSpan = true;
           if (thisHadNoAnnotations || !this.annotations.contains(a)) {
             addAnnotation(a);
           }
         }
       }
 
+      if (nameUnknown(this)) {
+        this.name = that.name;
+      } else if (thisIsClientSpan && thatIsServerSpan && !that.name.isEmpty()) {
+        this.name = that.name; // prefer the server's span name on collision
+      }
+
       if (!that.binaryAnnotations.isEmpty()) {
         boolean thisHadNoBinaryAnnotations = this.binaryAnnotations == null;
         for (BinaryAnnotation a : that.binaryAnnotations) {
@@ -519,4 +522,8 @@ Object readResolve() throws ObjectStreamException {
       }
     }
   }
+
+  static boolean nameUnknown(Span.Builder span) {
+    return span.name == null || span.name.length() == 0 || span.name.equals("unknown");
+  }
 }

zipkin/src/main/java/zipkin/TraceKeys.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2015-2017 The OpenZipkin Authors
+ * Copyright 2015-2018 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -14,7 +14,7 @@
 package zipkin;
 
 /**
- * Well-known {@link BinaryAnnotation#key binary annotation keys}.
+ * Well-known {@link BinaryAnnotation#key binary annotation aka Tag keys}.
  *
  * <h3>Overhead of adding Trace Data</h3>
  *
@@ -53,20 +53,32 @@ public final class TraceKeys {
   /**
    * The absolute http path, without any query parameters. Ex. "/objects/abcd-ff"
    *
-   * Used to filter against an http route, portably with zipkin v1.
+   * <p>Used as a filter or to clarify the request path for a given route. For example, the path for
+   * a route "/objects/:objectId" could be "/objects/abdc-ff". This does not limit cardinality like
+   * {@link #HTTP_ROUTE} can, so is not a good input to a span name.
    *
-   * <p>In zipkin v1, only equals filters are supported. Dropping query parameters makes the number
-   * of distinct URIs less. For example, one can query for the same resource, regardless of signing
-   * parameters encoded in the query line. This does not reduce cardinality to a HTTP single route.
-   * For example, it is common to express a route as an http URI template like
-   * /resource/{resource_id}. In systems where only equals queries are available, searching for
-   * http/path=/resource won't match if the actual request was /resource/abcd-ff.
+   * <p>The Zipkin query api only supports equals filters. Dropping query parameters makes the
+   * number of distinct URIs less. For example, one can query for the same resource, regardless of
+   * signing parameters encoded in the query line. Dropping query parameters also limits the
+   * security impact of this tag.
    *
-   * <p>Historical note: This was commonly expressed as "http.uri" in zipkin, eventhough it was most
-   * often just a path.
+   * <p>Historical note: This was commonly expressed as "http.uri" in zipkin, even though it was most
    */
   public static final String HTTP_PATH = "http.path";
 
+  /**
+   * The route which a request matched or "" (empty string) if routing is supported, but there was
+   * no match. Ex "/objects/{objectId}"
+   *
+   * <p>Often used as a span name when known, with empty routes coercing to "not_found" or
+   * "redirected" based on {@link #HTTP_STATUS_CODE}.
+   *
+   * <p>Unlike {@link #HTTP_PATH}, this value is fixed cardinality, so is a safe input to a span
+   * name function or a metrics dimension. Different formats are possible. For example, the
+   * following are all valid route templates: "/objects" "/objects/:objectId" "/objects/*"
+   */
+  public static final String HTTP_ROUTE = "http.route";
+
   /**
    * The entire URL, including the scheme, host and query parameters if available. Ex.
    * "https://mybucket.s3.amazonaws.com/objects/abcd-ff?X-Amz-Algorithm=AWS4-HMAC-SHA256..."

zipkin/src/test/java/zipkin/SpanTest.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2015-2017 The OpenZipkin Authors
+ * Copyright 2015-2018 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -126,6 +126,28 @@ public void mergeWhenBinaryAnnotationsSentSeparately() {
     assertThat(part2.toBuilder().merge(part1).build()).isEqualTo(expected);
   }
 
+  @Test
+  public void mergePrefersServerSpanName() {
+    Span clientPart = Span.builder()
+      .traceId(1L)
+      .name("get")
+      .id(1L)
+      .addAnnotation(Annotation.create(1L, CLIENT_SEND, APP_ENDPOINT))
+      .build();
+
+    Span serverPart = Span.builder()
+      .traceId(1L)
+      .name("/users/:userid")
+      .id(1L)
+      .addAnnotation(Annotation.create(2L, SERVER_RECV, APP_ENDPOINT))
+      .build();
+
+    assertThat(clientPart.toBuilder().merge(serverPart).build().name)
+      .isEqualTo("/users/:userid");
+    assertThat(serverPart.toBuilder().merge(clientPart).build().name)
+      .isEqualTo("/users/:userid");
+  }
+
   /**
    * Test merging of client and server spans into a single span, with a clock skew. Final timestamp
    * and duration for the span should be same as client.