Review semantic convention for Http Client spans (#1214)

* Review semantic convention for Http Client spans * Polish * Update instrumentation/google-http-client-1.19/src/test/groovy/AbstractGoogleHttpClientTest.groovy Co-authored-by: Trask Stalnaker <[email protected]> Co-authored-by: Trask Stalnaker <[email protected]>
open-telemetry · Sep 18, 2020 · beb1901 · beb1901
1 parent e8b5488
commit beb1901
Show file tree

Hide file tree

Showing 27 changed files with 257 additions and 148 deletions.
diff --git a/docs/semantic-conventions.md b/docs/semantic-conventions.md
@@ -34,3 +34,32 @@ attribute. As either it or `http.url` is required, we set the latter. This, in t
 
 **[3]:** In case of Armeria, return values are [SessionProtocol](https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/common/SessionProtocol.java),
 not values defined by spec.
+
+## Http Client
+
+| Attribute | Required | Implemented? |
+|---|:---:|:---:|
+| `http.method` | Y | + |
+| `http.url` | N | + |
+| `http.target` | N | - [1] |
+| `http.host` | N | - [1] |
+| `http.scheme` | N | - [1] |
+| `http.status_code` | Y | + |
+| `http.status_text` | N | - [2] |
+| `http.flavor` | N | + [3] |
+| `http.user_agent` | N | + |
+| `http.request_content_length` | N | - |
+| `http.request_content_length_uncompressed` | N | - |
+| `http.response_content_length` | N | - |
+| `http.response_content_length_uncompressed` | N | - |
+
+**[1]:** As the majority of Java frameworks don't provide a standard way to obtain "The full request
+target as passed in a HTTP request line or equivalent.", we don't set `http.target` semantic
+attribute. As either it or `http.url` is required, we set the latter. This, in turn, makes setting
+`http.schema` and `http.host` unnecessary duplication. Therefore, we do not set them as well.
+
+**[2]: TODO** After [this PR](https://github.com/open-telemetry/opentelemetry-specification/issues/950)
+ is merged, remove this line. If it rejected, then implement this attribute.
+
+**[3]:** In case of Armeria, return values are [SessionProtocol](https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/common/SessionProtocol.java),
+not values defined by spec.
diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java
@@ -27,7 +27,6 @@
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
 import java.util.Properties;
 import java.util.SortedSet;
@@ -76,8 +75,6 @@ public class Config {
   public static final String TRACE_CLASSES_EXCLUDE = "trace.classes.exclude";
   public static final String HTTP_SERVER_ERROR_STATUSES = "http.server.error.statuses";
   public static final String HTTP_CLIENT_ERROR_STATUSES = "http.client.error.statuses";
-  public static final String HTTP_SERVER_TAG_QUERY_STRING = "http.server.tag.query-string";
-  public static final String HTTP_CLIENT_TAG_QUERY_STRING = "http.client.tag.query-string";
   public static final String SCOPE_DEPTH_LIMIT = "trace.scope.depth.limit";
   public static final String RUNTIME_CONTEXT_FIELD_INJECTION =
       "trace.runtime.context.field.injection";
@@ -93,13 +90,8 @@ public class Config {
 
   private static final boolean DEFAULT_RUNTIME_CONTEXT_FIELD_INJECTION = true;
 
-  private static final boolean DEFAULT_HTTP_SERVER_TAG_QUERY_STRING = false;
-  private static final boolean DEFAULT_HTTP_CLIENT_TAG_QUERY_STRING = false;
   private static final int DEFAULT_SCOPE_DEPTH_LIMIT = 100;
 
-  public static final boolean DEFAULT_LOG_INJECTION_ENABLED = false;
-  public static final String DEFAULT_EXPERIMENTAL_LOG_CAPTURE_THRESHOLD = null;
-
   public static final boolean DEFAULT_KAFKA_CLIENT_PROPAGATION_ENABLED = true;
 
   public static final boolean DEFAULT_HYSTRIX_TAGS_ENABLED = false;
@@ -119,8 +111,6 @@ public class Config {
   private final boolean traceEnabled;
   private final boolean integrationsEnabled;
   private final List<String> excludedClasses;
-  private final boolean httpServerTagQueryString;
-  private final boolean httpClientTagQueryString;
   private final Integer scopeDepthLimit;
   private final boolean runtimeContextFieldInjection;
 
@@ -157,14 +147,6 @@ public class Config {
 
     excludedClasses = getListSettingFromEnvironment(TRACE_CLASSES_EXCLUDE, null);
 
-    httpServerTagQueryString =
-        getBooleanSettingFromEnvironment(
-            HTTP_SERVER_TAG_QUERY_STRING, DEFAULT_HTTP_SERVER_TAG_QUERY_STRING);
-
-    httpClientTagQueryString =
-        getBooleanSettingFromEnvironment(
-            HTTP_CLIENT_TAG_QUERY_STRING, DEFAULT_HTTP_CLIENT_TAG_QUERY_STRING);
-
     scopeDepthLimit =
         getIntegerSettingFromEnvironment(SCOPE_DEPTH_LIMIT, DEFAULT_SCOPE_DEPTH_LIMIT);
 
@@ -213,14 +195,6 @@ private Config(Properties properties, Config parent) {
     excludedClasses =
         getPropertyListValue(properties, TRACE_CLASSES_EXCLUDE, parent.excludedClasses);
 
-    httpServerTagQueryString =
-        getPropertyBooleanValue(
-            properties, HTTP_SERVER_TAG_QUERY_STRING, parent.httpServerTagQueryString);
-
-    httpClientTagQueryString =
-        getPropertyBooleanValue(
-            properties, HTTP_CLIENT_TAG_QUERY_STRING, parent.httpClientTagQueryString);
-
     scopeDepthLimit =
         getPropertyIntegerValue(properties, SCOPE_DEPTH_LIMIT, parent.scopeDepthLimit);
 
@@ -492,10 +466,6 @@ private static Properties loadConfigurationFile() {
     return properties;
   }
 
-  private static String toUpper(String str) {
-    return str == null ? null : str.toUpperCase(Locale.ENGLISH);
-  }
-
   // This has to be placed after all other static fields to give them a chance to initialize
   private static final Config INSTANCE = new Config();
 
@@ -535,14 +505,6 @@ public List<String> getExcludedClasses() {
     return excludedClasses;
   }
 
-  public boolean isHttpServerTagQueryString() {
-    return httpServerTagQueryString;
-  }
-
-  public boolean isHttpClientTagQueryString() {
-    return httpClientTagQueryString;
-  }
-
   public Integer getScopeDepthLimit() {
     return scopeDepthLimit;
   }
@@ -604,10 +566,6 @@ public String toString() {
         + integrationsEnabled
         + ", excludedClasses="
         + excludedClasses
-        + ", httpServerTagQueryString="
-        + httpServerTagQueryString
-        + ", httpClientTagQueryString="
-        + httpClientTagQueryString
         + ", scopeDepthLimit="
         + scopeDepthLimit
         + ", runtimeContextFieldInjection="

diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java
@@ -24,10 +24,7 @@
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.context.propagation.TextMapPropagator;
 import io.opentelemetry.context.propagation.TextMapPropagator.Setter;
-import io.opentelemetry.instrumentation.api.MoreAttributes;
-import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.instrumentation.api.decorator.HttpStatusConverter;
-import io.opentelemetry.instrumentation.api.tracer.utils.HttpUrlUtils;
 import io.opentelemetry.instrumentation.api.tracer.utils.NetPeerUtils;
 import io.opentelemetry.trace.DefaultSpan;
 import io.opentelemetry.trace.Span;
@@ -54,6 +51,12 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
   @Nullable
   protected abstract URI url(REQUEST request) throws URISyntaxException;
 
+  @Nullable
+  protected String flavor(REQUEST request) {
+    // This is de facto standard nowadays, so let us use it, unless overridden
+    return "1.1";
+  }
+
   protected abstract Integer status(RESPONSE response);
 
   @Nullable
@@ -138,32 +141,40 @@ private Span startSpan(REQUEST request, String name, long startTimeNanos) {
   protected Span onRequest(Span span, REQUEST request) {
     assert span != null;
     if (request != null) {
-      span.setAttribute(SemanticAttributes.HTTP_METHOD.key(), method(request));
-      span.setAttribute(SemanticAttributes.NET_TRANSPORT.key(), "IP.TCP");
+      SemanticAttributes.NET_TRANSPORT.set(span, "IP.TCP");
+      SemanticAttributes.HTTP_METHOD.set(span, method(request));
+      SemanticAttributes.HTTP_USER_AGENT.set(span, requestHeader(request, USER_AGENT));
 
-      String userAgent = requestHeader(request, USER_AGENT);
-      if (userAgent != null) {
-        SemanticAttributes.HTTP_USER_AGENT.set(span, userAgent);
-      }
+      setFlavor(span, request);
+      setUrl(span, request);
+    }
+    return span;
+  }
 
-      try {
-        URI url = url(request);
-        if (url != null && url.getHost() != null) {
-          NetPeerUtils.setNetPeer(span, url.getHost(), null);
-          if (url.getPort() > 0) {
-            span.setAttribute(SemanticAttributes.NET_PEER_PORT.key(), url.getPort());
-          }
-        }
-        HttpUrlUtils.setHttpUrl(span, url);
-        if (Config.get().isHttpClientTagQueryString()) {
-          span.setAttribute(MoreAttributes.HTTP_QUERY, url.getQuery());
-          span.setAttribute(MoreAttributes.HTTP_FRAGMENT, url.getFragment());
-        }
-      } catch (Exception e) {
-        log.debug("Error tagging url", e);
+  private void setFlavor(Span span, REQUEST request) {
+    String flavor = flavor(request);
+    if (flavor == null) {
+      return;
+    }
+
+    String httpProtocolPrefix = "HTTP/";
+    if (flavor.startsWith(httpProtocolPrefix)) {
+      flavor = flavor.substring(httpProtocolPrefix.length());
+    }
+
+    SemanticAttributes.HTTP_FLAVOR.set(span, flavor);
+  }
+
+  private void setUrl(Span span, REQUEST request) {
+    try {
+      URI url = url(request);
+      if (url != null) {
+        NetPeerUtils.setNetPeer(span, url.getHost(), null, url.getPort());
+        SemanticAttributes.HTTP_URL.set(span, url.toString());
       }
+    } catch (Exception e) {
+      log.debug("Error tagging url", e);
     }
-    return span;
   }
 
   protected Span onResponse(Span span, RESPONSE response) {

diff --git a/...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy b/...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy
@@ -17,13 +17,13 @@
 package io.opentelemetry.instrumentation.api.tracer
 
 import static io.opentelemetry.auto.test.utils.ConfigUtils.withConfigOverride
+
 import io.opentelemetry.context.propagation.TextMapPropagator
 import io.opentelemetry.instrumentation.api.decorator.HttpStatusConverter
 import io.opentelemetry.trace.Span
 import io.opentelemetry.trace.attributes.SemanticAttributes
 import spock.lang.Shared
 
-
 class HttpClientTracerTest extends BaseTracerTest {
 
   @Shared
@@ -47,6 +47,7 @@ class HttpClientTracerTest extends BaseTracerTest {
       1 * span.setAttribute(SemanticAttributes.NET_PEER_NAME.key(), req.url.host)
       1 * span.setAttribute(SemanticAttributes.NET_PEER_PORT.key(), req.url.port)
       1 * span.setAttribute(SemanticAttributes.HTTP_USER_AGENT.key(), req["User-Agent"])
+      1 * span.setAttribute(SemanticAttributes.HTTP_FLAVOR.key(), "1.1")
     }
     0 * _
 
@@ -78,6 +79,7 @@ class HttpClientTracerTest extends BaseTracerTest {
       1 * span.setAttribute(SemanticAttributes.NET_PEER_PORT.key(), req.url.port)
       1 * span.setAttribute("peer.service", "reservation-service")
       1 * span.setAttribute(SemanticAttributes.HTTP_USER_AGENT.key(), req["User-Agent"])
+      1 * span.setAttribute(SemanticAttributes.HTTP_FLAVOR.key(), "1.1")
     }
     0 * _
   }
@@ -87,20 +89,16 @@ class HttpClientTracerTest extends BaseTracerTest {
     def tracer = newTracer()
 
     when:
-    withConfigOverride(io.opentelemetry.instrumentation.api.config.Config.HTTP_CLIENT_TAG_QUERY_STRING, "$tagQueryString") {
-      tracer.onRequest(span, req)
-    }
+    tracer.onRequest(span, req)
 
     then:
     1 * span.setAttribute(SemanticAttributes.NET_TRANSPORT.key(), "IP.TCP")
-    if (expectedUrl) {
+    if (expectedUrl != null) {
       1 * span.setAttribute(SemanticAttributes.HTTP_URL.key(), expectedUrl)
     }
-    if (expectedUrl && tagQueryString) {
-      1 * span.setAttribute(io.opentelemetry.instrumentation.api.MoreAttributes.HTTP_QUERY, expectedQuery)
-      1 * span.setAttribute(io.opentelemetry.instrumentation.api.MoreAttributes.HTTP_FRAGMENT, expectedFragment)
-    }
     1 * span.setAttribute(SemanticAttributes.HTTP_METHOD.key(), null)
+    1 * span.setAttribute(SemanticAttributes.HTTP_FLAVOR.key(), "1.1")
+    1 * span.setAttribute(SemanticAttributes.HTTP_USER_AGENT.key(), null)
     if (hostname) {
       1 * span.setAttribute(SemanticAttributes.NET_PEER_NAME.key(), hostname)
     }
@@ -110,19 +108,13 @@ class HttpClientTracerTest extends BaseTracerTest {
     0 * _
 
     where:
-    tagQueryString | url                                                   | expectedUrl                                     | expectedQuery      | expectedFragment      | hostname | port
-    false          | null                                                  | null                                            | null               | null                  | null     | null
-    false          | ""                                                    | "/"                                             | ""                 | null                  | null     | null
-    false          | "/path?query"                                         | "/path?query"                                   | ""                 | null                  | null     | null
-    false          | "https://host:0"                                      | "https://host/"                                 | ""                 | null                  | "host"   | null
-    false          | "https://host/path"                                   | "https://host/path"                             | ""                 | null                  | "host"   | null
-    false          | "http://host:99/path?query#fragment"                  | "http://host:99/path?query#fragment"            | ""                 | null                  | "host"   | 99
-    true           | null                                                  | null                                            | null               | null                  | null     | null
-    true           | ""                                                    | "/"                                             | null               | null                  | null     | null
-    true           | "/path?encoded+%28query%29%3F"                        | "/path?encoded+(query)?"                        | "encoded+(query)?" | null                  | null     | null
-    true           | "https://host:0"                                      | "https://host/"                                 | null               | null                  | "host"   | null
-    true           | "https://host/path"                                   | "https://host/path"                             | null               | null                  | "host"   | null
-    true           | "http://host:99/path?query#encoded+%28fragment%29%3F" | "http://host:99/path?query#encoded+(fragment)?" | "query"            | "encoded+(fragment)?" | "host"   | 99
+    tagQueryString | url                                  | expectedUrl                          | expectedQuery | expectedFragment | hostname | port
+    false          | null                                 | null                                 | null          | null             | null     | null
+    false          | ""                                   | ""                                   | ""            | null             | null     | null
+    false          | "/path?query"                        | "/path?query"                        | ""            | null             | null     | null
+    false          | "https://host:0"                     | "https://host:0"                     | ""            | null             | "host"   | null
+    false          | "https://host/path"                  | "https://host/path"                  | ""            | null             | "host"   | null
+    false          | "http://host:99/path?query#fragment" | "http://host:99/path?query#fragment" | ""            | null             | "host"   | 99
 
     req = [url: url == null ? null : new URI(url)]
   }

diff --git a/....0/src/main/java/io/opentelemetry/instrumentation/auto/akkahttp/AkkaHttpClientTracer.java b/....0/src/main/java/io/opentelemetry/instrumentation/auto/akkahttp/AkkaHttpClientTracer.java
@@ -48,6 +48,11 @@ protected URI url(HttpRequest httpRequest) throws URISyntaxException {
     return new URI(httpRequest.uri().toString());
   }
 
+  @Override
+  protected String flavor(HttpRequest httpRequest) {
+    return httpRequest.protocol().value();
+  }
+
   @Override
   protected Integer status(HttpResponse httpResponse) {
     return httpResponse.status().intValue();

diff --git a/...opentelemetry/instrumentation/auto/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java b/...opentelemetry/instrumentation/auto/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java
@@ -30,6 +30,7 @@
 import org.apache.http.RequestLine;
 import org.apache.http.StatusLine;
 import org.apache.http.client.methods.HttpUriRequest;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 public class ApacheHttpAsyncClientTracer
     extends HttpClientTracer<HttpRequest, HttpRequest, HttpResponse> {
@@ -46,6 +47,11 @@ protected String method(HttpRequest request) {
     }
   }
 
+  @Override
+  protected @Nullable String flavor(HttpRequest httpRequest) {
+    return httpRequest.getProtocolVersion().toString();
+  }
+
   @Override
   protected URI url(HttpRequest request) throws URISyntaxException {
     /*

diff --git a/...a/io/opentelemetry/instrumentation/auto/apachehttpclient/v4_0/ApacheHttpClientTracer.java b/...a/io/opentelemetry/instrumentation/auto/apachehttpclient/v4_0/ApacheHttpClientTracer.java
@@ -26,6 +26,7 @@
 import org.apache.http.HttpMessage;
 import org.apache.http.HttpResponse;
 import org.apache.http.client.methods.HttpUriRequest;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 class ApacheHttpClientTracer
     extends HttpClientTracer<HttpUriRequest, HttpUriRequest, HttpResponse> {
@@ -37,6 +38,11 @@ protected String method(HttpUriRequest httpRequest) {
     return httpRequest.getMethod();
   }
 
+  @Override
+  protected @Nullable String flavor(HttpUriRequest httpUriRequest) {
+    return httpUriRequest.getProtocolVersion().toString();
+  }
+
   @Override
   protected URI url(HttpUriRequest request) {
     return request.getURI();

diff --git a/...c/main/java/io/opentelemetry/instrumentation/armeria/v1_0/client/ArmeriaClientTracer.java b/...c/main/java/io/opentelemetry/instrumentation/armeria/v1_0/client/ArmeriaClientTracer.java
@@ -40,6 +40,11 @@ protected String method(ClientRequestContext ctx) {
     return ctx.method().name();
   }
 
+  @Override
+  protected @Nullable String flavor(ClientRequestContext clientRequestContext) {
+    return clientRequestContext.sessionProtocol().toString();
+  }
+
   @Override
   @Nullable
   protected URI url(ClientRequestContext ctx) throws URISyntaxException {

diff --git a/.../src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_0/AbstractArmeriaTest.groovy b/.../src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_0/AbstractArmeriaTest.groovy
@@ -77,6 +77,7 @@ abstract class AbstractArmeriaTest extends InstrumentationSpecification {
             "${SemanticAttributes.HTTP_URL.key()}" "${server.httpUri()}${path}"
             "${SemanticAttributes.HTTP_METHOD.key()}" method.name()
             "${SemanticAttributes.HTTP_STATUS_CODE.key()}" code
+            "${SemanticAttributes.HTTP_FLAVOR.key()}" "http"
           }
         }
         span(1) {