diff --git a/instrumentation/http/README.md b/instrumentation/http/README.md index e029feed06..a342fca7b4 100644 --- a/instrumentation/http/README.md +++ b/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 void request(HttpAdapter 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. diff --git a/instrumentation/http/src/main/java/brave/http/HttpParser.java b/instrumentation/http/src/main/java/brave/http/HttpParser.java index e301ad72b2..1913f7462e 100644 --- a/instrumentation/http/src/main/java/brave/http/HttpParser.java +++ b/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". * *

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 void request(HttpAdapter 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); diff --git a/instrumentation/http/src/test/java/brave/http/HttpParserTest.java b/instrumentation/http/src/test/java/brave/http/HttpParserTest.java index 19ced5ef2b..8c294c6bb2 100644 --- a/instrumentation/http/src/test/java/brave/http/HttpParserTest.java +++ b/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);