Adds http.method tag by default and removes bad naming advice (#616)

This adds the http.method tag by default as many frameworks override
the span name, or will as soon as http.route is supported. This allows
users to always see basic http info at the cost of a small, fixed
cardinality tag. Users who really don't want to see this tag can already
disable parsing of it by overriding the default parser.

This also removes the bad advice from the README, which hinted at using
a path as a span name. This will result in ever-expanding span name
index when there are path variables. The http route based approach is
superior and merges next.

Author: adriancole

instrumentation/http/README.md

@@ -11,6 +11,7 @@ instructions on what to put into http spans, and sampling policy.
 By default, the following are added to both http client and server spans:
 * Span.name as the http method in lowercase: ex "get"
 * 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
@@ -20,15 +21,14 @@ 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
       }
     })
@@ -38,8 +38,8 @@ apache = TracingHttpClientBuilder.create(httpTracing.clientOf("s3"));
 okhttp = TracingCallFactory.create(httpTracing.clientOf("sqs"), new OkHttpClient());
 ```
 
-If you just want to control span naming policy, override `spanName` in
-your client or server parser.
+If you just want to control span naming policy based on the request,
+override `spanName` in your client or server parser.
 
 Ex:
 ```java
@@ -50,6 +50,9 @@ overrideSpanName = new HttpClientParser() {
 };
 ```
 
+Note that span name can be overwritten any time, for example, when
+parsing the response.
+
 ## Sampling Policy
 The default sampling policy is to use the default (trace ID) sampler for
 server and client requests.

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

@@ -6,15 +6,22 @@
 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.
    *
    * @see #spanName(HttpAdapter, Object)
    */
+  // Eventhough the default span name is the method, we have no way of knowing that a user hasn't
+  // overwritten the name to something else. If that occurs during response parsing, it is too late
+  // to go back and get the http method. Adding http method by default ensures span naming doesn't
+  // prevent basic HTTP info from being visible. A cost of this is another tag, but it is small with
+  // very limited cardinality. Moreover, users who care strictly about size can override this.
   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);
   }
@@ -66,7 +73,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);

instrumentation/http/src/test/java/brave/http/HttpParserTest.java

@@ -23,14 +23,16 @@ public class HttpParserTest {
     when(adapter.method(request)).thenReturn("GET");
 
     assertThat(parser.spanName(adapter, request))
-        .isEqualTo("GET");
+        .isEqualTo("GET"); // note: in practice this will become lowercase
   }
 
-  @Test public void request_addsPath() {
+  @Test public void request_addsMethodAndPath() {
+    when(adapter.method(request)).thenReturn("GET");
     when(adapter.path(request)).thenReturn("/foo");
 
     parser.request(adapter, request, customizer);
 
+    verify(customizer).tag("http.method", "GET");
     verify(customizer).tag("http.path", "/foo");
   }
 
@@ -49,6 +51,15 @@ public class HttpParserTest {
     verify(customizer).tag("error", "400");
   }
 
+  @Test public void response_statusZeroIsNotAnError() {
+    when(adapter.statusCodeAsInt(response)).thenReturn(0);
+
+    parser.response(adapter, response, null, customizer);
+
+    verify(customizer, never()).tag("http.status_code", "0");
+    verify(customizer, never()).tag("error", "0");
+  }
+
   @Test public void response_tagsErrorFromException() {
     parser.response(adapter, response, new RuntimeException("drat"), customizer);