Reduces the parser hooks to parity with how responses are handled (#406)

Response can be either error or success. This aligns parsing strategy
with handling strategy, which reduces duplication particularly around
parsing status codes.

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

@@ -45,17 +45,6 @@ public abstract class HttpAdapter<Req, Resp> {
    */
   @Nullable public abstract Integer statusCode(Resp response);
 
-  String parseError(@Nullable Resp response, @Nullable Throwable error) {
-    if (error != null) {
-      String message = error.getMessage();
-      return message != null ? message : error.getClass().getSimpleName();
-    }
-    Integer httpStatus = statusCode(response);
-    if (httpStatus != null && (httpStatus < 200 || httpStatus > 399)) {
-      return String.valueOf(httpStatus);
-    }
-    return null;
-  }
   HttpAdapter() {
   }
 }

brave/src/main/java/brave/http/HttpClientHandler.java

@@ -65,6 +65,8 @@ public Span handleSend(TraceContext.Injector<Req> injector, Req request) {
   /**
    * Like {@link #handleSend(TraceContext.Injector, Object)}, except for when the carrier of
    * trace data is not the same as the request.
+   *
+   * @see HttpClientParser#requestTags(HttpAdapter, Object, Span)
    */
   public <C> Span handleSend(TraceContext.Injector<C> injector, C carrier, Req request) {
     Span span = tracer.nextSpan();
@@ -86,16 +88,13 @@ public <C> Span handleSend(TraceContext.Injector<C> injector, C carrier, Req req
    *
    * <p>This is typically called once the response headers are received, and after the span is
    * {@link brave.Tracer.SpanInScope#close() no longer in scope}.
+   *
+   * @see HttpClientParser#responseTags(HttpAdapter, Object, Throwable, Span)
    */
   public void handleReceive(@Nullable Resp response, @Nullable Throwable error, Span span) {
     if (span.isNoop()) return;
-
     try {
-      if (response != null || error != null) {
-        String message = adapter.parseError(response, error);
-        if (message != null) span.tag(zipkin.Constants.ERROR, message);
-      }
-      if (response != null) parser.responseTags(adapter, response, span);
+      parser.responseTags(adapter, response, error, span);
     } finally {
       span.finish();
     }

brave/src/main/java/brave/http/HttpClientParser.java

@@ -2,51 +2,29 @@
 
 import brave.Span;
 import brave.internal.Nullable;
-import zipkin.TraceKeys;
 
 /**
  * Provides reasonable defaults for the data contained in http client spans. Subclass to customize,
  * for example, to add tags based on response headers.
  */
-public class HttpClientParser {
-  /** Returns the span name of the request. Defaults to the http method. */
-  public <Req> String spanName(HttpAdapter<Req, ?> adapter, Req req) {
-    return adapter.method(req);
-  }
+public class HttpClientParser extends HttpParser {
 
   /**
    * Adds any tags based on the request that will be sent to the server.
    *
-   * <p>By default, this adds the {@link TraceKeys#HTTP_PATH}.
+   * <p>{@inheritDoc}
    */
-  public <Req> void requestTags(HttpAdapter<Req, ?> adapter, Req req, Span span) {
-    String path = adapter.path(req);
-    if (path != null) span.tag(TraceKeys.HTTP_PATH, path);
+  @Override public <Req> void requestTags(HttpAdapter<Req, ?> adapter, Req req, Span span) {
+    super.requestTags(adapter, req, span);
   }
 
   /**
    * Adds any tags based on the response received from the server.
    *
-   * <p>By default, this adds the {@link TraceKeys#HTTP_STATUS_CODE} if the status code is >=300.
-   */
-  public <Resp> void responseTags(HttpAdapter<?, Resp> adapter, Resp res, Span span) {
-    Integer httpStatus = adapter.statusCode(res);
-    if (httpStatus != null && (httpStatus < 200 || httpStatus > 299)) {
-      span.tag(TraceKeys.HTTP_STATUS_CODE, String.valueOf(httpStatus));
-    }
-  }
-
-  /**
-   * Returns an {@link zipkin.Constants#ERROR error message}, if there was an error sending the
-   * request to the server, or if the response received was an error.
-   *
-   * <p>By default, this makes an error message based on the exception message, or the status code,
-   * if the status code is >=400.
-   *
-   * <p>Note: Either the response or error parameters may be null, but not both
+   * <p>{@inheritDoc}
    */
-  @Nullable public <Resp> String error(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
-      @Nullable Throwable error) {
-    return adapter.parseError(res, error);
+  @Override public <Resp> void responseTags(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
+      @Nullable Throwable error, Span span) {
+    super.responseTags(adapter, res, error, span);
   }
 }

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

@@ -0,0 +1,61 @@
+package brave.http;
+
+import brave.Span;
+import brave.internal.Nullable;
+import zipkin.Constants;
+import zipkin.TraceKeys;
+
+/**
+ * Provides reasonable defaults for the data contained in http spans. Subclass to customize,
+ * for example, to add tags based on user ID.
+ */
+public class HttpParser {
+  /** Returns the span name of the request. Defaults to the http method. */
+  public <Req> String spanName(HttpAdapter<Req, ?> adapter, Req req) {
+    return adapter.method(req);
+  }
+
+  /** By default, this adds the {@link TraceKeys#HTTP_PATH}. */
+  public <Req> void requestTags(HttpAdapter<Req, ?> adapter, Req req, Span span) {
+    String path = adapter.path(req);
+    if (path != null) span.tag(TraceKeys.HTTP_PATH, path);
+  }
+
+  /***
+   * By default, this adds {@link TraceKeys#HTTP_STATUS_CODE} when it is not 2xx. If there's an
+   * exception or the status code is neither 2xx nor 3xx, it adds {@link Constants#ERROR}.
+   *
+   * <p>Note: Either the response or error parameters may be null, but not both
+   *
+   * @see #parseError(Integer, Throwable)
+   */
+  public <Resp> void responseTags(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
+      @Nullable Throwable error, Span span) {
+    Integer httpStatus = res != null ? adapter.statusCode(res) : null;
+    if (httpStatus != null && (httpStatus < 200 || httpStatus > 299)) {
+      span.tag(TraceKeys.HTTP_STATUS_CODE, String.valueOf(httpStatus));
+    }
+    String message = parseError(httpStatus, error);
+    if (message != null) span.tag(Constants.ERROR, message);
+  }
+
+  /**
+   * Returns the {@link TraceKeys#HTTP_STATUS_CODE} when it is not 2xx. If there's an
+   * exception or the status code is neither 2xx nor 3xx, it adds {@link Constants#ERROR}.
+   *
+   * <p>Note: Either the httpStatus or error parameters may be null, but not both
+   *
+   * @see Constants#ERROR
+   */
+  @Nullable protected String parseError(@Nullable Integer httpStatus, @Nullable Throwable error) {
+    if (error != null) {
+      String message = error.getMessage();
+      return message != null ? message : error.getClass().getSimpleName();
+    }
+    if (httpStatus == null) return null;
+    return httpStatus < 200 || httpStatus > 399 ? String.valueOf(httpStatus) : null;
+  }
+
+  HttpParser() {
+  }
+}

brave/src/main/java/brave/http/HttpServerHandler.java

@@ -58,6 +58,8 @@ public Span handleReceive(TraceContext.Extractor<Req> extractor, Req request) {
   /**
    * Like {@link #handleReceive(TraceContext.Extractor, Object)}, except for when the carrier of
    * trace data is not the same as the request.
+   *
+   * @see HttpServerParser#requestTags(HttpAdapter, Object, Span)
    */
   public <C> Span handleReceive(TraceContext.Extractor<C> extractor, C carrier, Req request) {
     TraceContextOrSamplingFlags contextOrFlags = extractor.extract(carrier);
@@ -81,16 +83,13 @@ public <C> Span handleReceive(TraceContext.Extractor<C> extractor, C carrier, Re
    *
    * <p>This is typically called once the response headers are sent, and after the span is
    * {@link brave.Tracer.SpanInScope#close() no longer in scope}.
+   *
+   * @see HttpServerParser#responseTags(HttpAdapter, Object, Throwable, Span)
    */
   public void handleSend(@Nullable Resp response, @Nullable Throwable error, Span span) {
     if (span.isNoop()) return;
-
     try {
-      if (response != null || error != null) {
-        String message = adapter.parseError(response, error);
-        if (message != null) span.tag(zipkin.Constants.ERROR, message);
-      }
-      if (response != null) parser.responseTags(adapter, response, span);
+      parser.responseTags(adapter, response, error, span);
     } finally {
       span.finish();
     }

brave/src/main/java/brave/http/HttpServerParser.java

@@ -2,51 +2,29 @@
 
 import brave.Span;
 import brave.internal.Nullable;
-import zipkin.TraceKeys;
 
 /**
  * Provides reasonable defaults for the data contained in http server spans. Subclass to customize,
  * for example, to add tags based on user ID.
  */
-public class HttpServerParser {
-  /** Returns the span name of the request. Defaults to the http method. */
-  public <Req> String spanName(HttpAdapter<Req, ?> adapter, Req req) {
-    return adapter.method(req);
-  }
+public class HttpServerParser extends HttpParser {
 
   /**
    * Adds any tags based on the request received from the client.
    *
-   * <p>By default, this adds the {@link TraceKeys#HTTP_PATH}.
+   * <p>{@inheritDoc}
    */
-  public <Req> void requestTags(HttpAdapter<Req, ?> adapter, Req req, Span span) {
-    String path = adapter.path(req);
-    if (path != null) span.tag(TraceKeys.HTTP_PATH, path);
+  @Override public <Req> void requestTags(HttpAdapter<Req, ?> adapter, Req req, Span span) {
+    super.requestTags(adapter, req, span);
   }
 
   /**
    * Adds any tags based on the response sent to the client.
    *
-   * <p>By default, this adds the {@link TraceKeys#HTTP_STATUS_CODE} if the status code is >=300.
-   */
-  public <Resp> void responseTags(HttpAdapter<?, Resp> adapter, Resp res, Span span) {
-    Integer httpStatus = adapter.statusCode(res);
-    if (httpStatus != null && (httpStatus < 200 || httpStatus > 299)) {
-      span.tag(TraceKeys.HTTP_STATUS_CODE, String.valueOf(httpStatus));
-    }
-  }
-
-  /**
-   * Returns an {@link zipkin.Constants#ERROR error message}, if there was an error lieu of a
-   * response, or if the response sent to the client was an error.
-   *
-   * <p>By default, this makes an error message based on the exception message, or the status code,
-   * if the status code is >=400.
-   *
-   * <p>Note: Either the response or error parameters may be null, but not both
+   * <p>{@inheritDoc}
    */
-  @Nullable public <Resp> String error(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
-      @Nullable Throwable error) {
-    return adapter.parseError(res, error);
+  @Override public <Resp> void responseTags(HttpAdapter<?, Resp> adapter, @Nullable Resp res,
+      @Nullable Throwable error, Span span) {
+    super.responseTags(adapter, res, error, span);
   }
 }

brave/src/main/java/brave/internal/StrictCurrentTraceContext.java

@@ -44,4 +44,4 @@ class StrictScope implements Scope {
       return caller.toString();
     }
   }
-}
+}

brave/src/test/java/brave/http/HttpClientHandlerTest.java

@@ -13,8 +13,7 @@
 import zipkin.Span;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Matchers.anyObject;
-import static org.mockito.Matchers.eq;
+import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -35,7 +34,6 @@ public class HttpClientHandlerTest {
     handler = HttpClientHandler.create(httpTracing, adapter);
 
     when(adapter.method(request)).thenReturn("GET");
-    when(adapter.parseError(eq(response), anyObject())).thenCallRealMethod();
   }
 
   @Test public void handleSend_defaultsToMakeNewTrace() {
@@ -77,27 +75,30 @@ public class HttpClientHandlerTest {
         .isEmpty();
   }
 
-  @Test public void handleReceive_doesntErrorOnRedirect() {
-    when(adapter.statusCode(response)).thenReturn(302);
+  @Test public void handleReceive_nothingOnNoop_success() {
+    when(span.isNoop()).thenReturn(true);
 
     handler.handleReceive(response, null, span);
 
-    verify(span, never()).tag("error", "302");
+    verify(span, never()).finish();
   }
 
-  @Test public void handleReceive_tagsErrorOnResponseCode() {
-    when(adapter.statusCode(response)).thenReturn(400);
+  @Test public void handleReceive_nothingOnNoop_error() {
+    when(span.isNoop()).thenReturn(true);
 
-    handler.handleReceive(response, null, span);
+    handler.handleReceive(null, new RuntimeException("drat"), span);
 
-    verify(span).tag("error", "400");
+    verify(span, never()).finish();
   }
 
-  @Test public void handleReceive_tagsErrorPrefersExceptionVsResponseCode() {
-    when(adapter.statusCode(response)).thenReturn(400);
-
-    handler.handleReceive(response, new RuntimeException("drat"), span);
+  @Test public void handleReceive_finishedEvenIfAdapterThrows() {
+    when(adapter.statusCode(response)).thenThrow(new RuntimeException());
 
-    verify(span).tag("error", "drat");
+    try {
+      handler.handleReceive(response, null, span);
+      failBecauseExceptionWasNotThrown(RuntimeException.class);
+    } catch (RuntimeException e) {
+      verify(span).finish();
+    }
   }
 }

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

@@ -0,0 +1,72 @@
+package brave.http;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import zipkin.TraceKeys;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class HttpParserTest {
+  @Mock HttpClientAdapter<Object, Object> adapter;
+  @Mock brave.Span span;
+  Object request = new Object();
+  Object response = new Object();
+  HttpParser parser = new HttpParser();
+
+  @Test public void spanName_isMethod() {
+    when(adapter.method(request)).thenReturn("GET");
+
+    assertThat(parser.spanName(adapter, request))
+        .isEqualTo("GET");
+  }
+
+  @Test public void requestTags_addsPath() {
+    when(adapter.path(request)).thenReturn("/foo");
+
+    parser.requestTags(adapter, request, span);
+
+    verify(span).tag(TraceKeys.HTTP_PATH, "/foo");
+  }
+
+  @Test public void requestTags_doesntCrashOnNullPath() {
+    parser.requestTags(adapter, request, span);
+
+    verify(span, never()).tag(TraceKeys.HTTP_PATH, null);
+  }
+
+  @Test public void responseTags_tagsErrorOnResponseCode() {
+    when(adapter.statusCode(response)).thenReturn(400);
+
+    parser.responseTags(adapter, response, null, span);
+
+    verify(span).tag("error", "400");
+  }
+
+  @Test public void responseTags_tagsErrorFromException() {
+    parser.responseTags(adapter, response, new RuntimeException("drat"), span);
+
+    verify(span).tag("error", "drat");
+  }
+
+  @Test public void responseTags_tagsErrorPrefersExceptionVsResponseCode() {
+    when(adapter.statusCode(response)).thenReturn(400);
+
+    parser.responseTags(adapter, response, new RuntimeException("drat"), span);
+
+    verify(span).tag("error", "drat");
+  }
+
+  @Test public void responseTags_tagsErrorOnExceptionEvenIfStatusOk() {
+    when(adapter.statusCode(response)).thenReturn(200);
+
+    parser.responseTags(adapter, response, new RuntimeException("drat"), span);
+
+    verify(span).tag("error", "drat");
+  }
+}