Remove url from HttpServerAttributesExtractor (#4209)

* Remove url from HttpServerAttributesExtractor * Remove UriBuilder * Tracers too * apache-camel * Finatra * jsp * Ratpack * Ratpack library * Ratpack * Spark * Feedback * Fix Undertow * Vertx * vertx-web * play-2.4 * webflux * jaxrs * Spotless * Update semantic-conventions.md * Update smoke tests * More realistic target * Remove outdated doc * Wording
open-telemetry · Oct 3, 2021 · 92394ad · 92394ad
1 parent c421b66
commit 92394ad
Show file tree

Hide file tree

Showing 47 changed files with 382 additions and 364 deletions.
diff --git a/docs/semantic-conventions.md b/docs/semantic-conventions.md
@@ -8,12 +8,12 @@ are implemented by Java autoinstrumentation and which ones are not.
 | Attribute | Required | Implemented? |
 |---|:---:|:---:|
 | `http.method` | Y | + |
-| `http.url` | N | + |
-| `http.target` | N | - [1] |
-| `http.host` | N | - [1] |
-| `http.scheme` | N | - [1] |
+| `http.url` | N | - [1] |
+| `http.target` | N | + [1] |
+| `http.host` | N | + [1] |
+| `http.scheme` | N | + [1] |
 | `http.status_code` | Y | + |
-| `http.flavor` | N | + [3] |
+| `http.flavor` | N | + [2] |
 | `http.user_agent` | N | + |
 | `http.request_content_length` | N | - |
 | `http.request_content_length_uncompressed` | N | - |
@@ -23,12 +23,12 @@ are implemented by Java autoinstrumentation and which ones are not.
 | `http.route` | N | - |
 | `http.client_ip` | 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.
+**[1]:** Most server instrumentations capture `http.scheme`, `http.host`, and `http.target`
+(and do not capture `http.url`).
+Netty instrumentation is currently the only exception to this rule. Netty instrumentation
+captures `http.url` (and does not capture `http.scheme`, `http.host`, or `http.target`).
 
-**[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),
+**[2]:** 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.
 
 
@@ -42,19 +42,16 @@ not values defined by spec.
 | `http.host` | N | - [1] |
 | `http.scheme` | N | - [1] |
 | `http.status_code` | Y | + |
-| `http.flavor` | N | + [3] |
+| `http.flavor` | N | + [2] |
 | `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.
+**[1]:** `http.scheme`, `http.host` and `http.target` are unnecessary since `http.url` is captured.
 
-**[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),
+**[2]:** 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.
 
 ## RPC

diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java
@@ -31,9 +31,6 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {
     set(attributes, SemanticAttributes.HTTP_HOST, host(request));
     set(attributes, SemanticAttributes.HTTP_TARGET, target(request));
     set(attributes, SemanticAttributes.HTTP_ROUTE, route(request));
-
-    // TODO: this is specific to clients, should we remove this?
-    set(attributes, SemanticAttributes.HTTP_URL, url(request));
   }
 
   @Override
@@ -49,10 +46,6 @@ protected final void onEnd(
 
   // Attributes that always exist in a request
 
-  // TODO: this is specific to clients, should we remove this?
-  @Nullable
-  protected abstract String url(REQUEST request);
-
   @Nullable
   protected abstract String target(REQUEST request);
 

diff --git a/...mentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/UriBuilder.java b/...mentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/UriBuilder.java
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java
@@ -169,24 +169,15 @@ protected void onRequest(SpanBuilder spanBuilder, REQUEST request) {
     spanBuilder.setAttribute(
         SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
 
-    setUrl(spanBuilder, request);
-
-    // TODO set resource name from URL.
-  }
-
-  /*
-  https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md
-
-  HTTP semantic convention recommends setting http.scheme, http.host, http.target attributes
-  instead of http.url because it "is usually not readily available on the server side but would have
-  to be assembled in a cumbersome and sometimes lossy process from other information".
-
-  But in Java world there is no standard way to access "The full request target as passed in a HTTP request line or equivalent"
-  which is the recommended value for http.target attribute. Therefore we cannot use any of the
-  recommended combinations of attributes and are forced to use http.url.
-   */
-  private void setUrl(SpanBuilder spanBuilder, REQUEST request) {
-    spanBuilder.setAttribute(SemanticAttributes.HTTP_URL, url(request));
+    String url = url(request);
+    if (url != null) {
+      // netty instrumentation uses this
+      spanBuilder.setAttribute(SemanticAttributes.HTTP_URL, url);
+    } else {
+      spanBuilder.setAttribute(SemanticAttributes.HTTP_SCHEME, scheme(request));
+      spanBuilder.setAttribute(SemanticAttributes.HTTP_HOST, host(request));
+      spanBuilder.setAttribute(SemanticAttributes.HTTP_TARGET, target(request));
+    }
   }
 
   protected void onConnectionAndRequest(
@@ -297,7 +288,17 @@ private static void setStatus(Span span, int status) {
 
   protected abstract TextMapGetter<REQUEST> getGetter();
 
-  protected abstract String url(REQUEST request);
+  // netty still uses this, otherwise should prefer scheme/host/target
+  @Nullable
+  protected String url(REQUEST request) {
+    return null;
+  }
+
+  protected abstract String scheme(REQUEST request);
+
+  protected abstract String host(REQUEST request);
+
+  protected abstract String target(REQUEST request);
 
   protected abstract String method(REQUEST request);
 

diff --git a/...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java b/...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java
@@ -25,11 +25,6 @@ protected String method(Map<String, String> request) {
       return request.get("method");
     }
 
-    @Override
-    protected String url(Map<String, String> request) {
-      return request.get("url");
-    }
-
     @Override
     protected String target(Map<String, String> request) {
       return request.get("target");
@@ -99,7 +94,7 @@ void normal() {
     Map<String, String> request = new HashMap<>();
     request.put("method", "POST");
     request.put("url", "http://github.com");
-    request.put("target", "github.com");
+    request.put("target", "/repositories/1");
     request.put("host", "github.com:80");
     request.put("scheme", "https");
     request.put("userAgent", "okhttp 3.x");
@@ -120,8 +115,7 @@ void normal() {
     assertThat(attributes.build())
         .containsOnly(
             entry(SemanticAttributes.HTTP_METHOD, "POST"),
-            entry(SemanticAttributes.HTTP_URL, "http://github.com"),
-            entry(SemanticAttributes.HTTP_TARGET, "github.com"),
+            entry(SemanticAttributes.HTTP_TARGET, "/repositories/1"),
             entry(SemanticAttributes.HTTP_HOST, "github.com:80"),
             entry(SemanticAttributes.HTTP_SCHEME, "https"),
             entry(SemanticAttributes.HTTP_USER_AGENT, "okhttp 3.x"),
@@ -131,8 +125,7 @@ void normal() {
     assertThat(attributes.build())
         .containsOnly(
             entry(SemanticAttributes.HTTP_METHOD, "POST"),
-            entry(SemanticAttributes.HTTP_URL, "http://github.com"),
-            entry(SemanticAttributes.HTTP_TARGET, "github.com"),
+            entry(SemanticAttributes.HTTP_TARGET, "/repositories/1"),
             entry(SemanticAttributes.HTTP_HOST, "github.com:80"),
             entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{id}"),
             entry(SemanticAttributes.HTTP_SCHEME, "https"),

diff --git a/...ation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/UriBuilderTest.java b/...ation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/UriBuilderTest.java
diff --git a/...java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerTracer.java b/...java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerTracer.java
@@ -8,9 +8,11 @@
 import akka.http.javadsl.model.HttpHeader;
 import akka.http.scaladsl.model.HttpRequest;
 import akka.http.scaladsl.model.HttpResponse;
+import akka.http.scaladsl.model.Uri;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapGetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer;
+import scala.Option;
 
 public class AkkaHttpServerTracer
     extends HttpServerTracer<HttpRequest, HttpResponse, HttpRequest, Void> {
@@ -44,8 +46,24 @@ public Context getServerContext(Void none) {
   }
 
   @Override
-  protected String url(HttpRequest httpRequest) {
-    return httpRequest.uri().toString();
+  protected String scheme(HttpRequest httpRequest) {
+    return httpRequest.uri().scheme();
+  }
+
+  @Override
+  protected String host(HttpRequest httpRequest) {
+    Uri.Authority authority = httpRequest.uri().authority();
+    return authority.host().address() + ":" + authority.port();
+  }
+
+  @Override
+  protected String target(HttpRequest httpRequest) {
+    String target = httpRequest.uri().path().toString();
+    Option<String> queryString = httpRequest.uri().rawQueryString();
+    if (queryString.isDefined()) {
+      target += "?" + queryString.get();
+    }
+    return target;
   }
 
   @Override

diff --git a/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy b/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy
@@ -87,7 +87,9 @@ class RestCamelTest extends AgentInstrumentationSpecification implements RetryOn
           kind SERVER
           parentSpanId(span(1).spanId)
           attributes {
-            "$SemanticAttributes.HTTP_URL.key" "http://localhost:$port/api/firstModule/unit/unitOne"
+            "$SemanticAttributes.HTTP_SCHEME.key" "http"
+            "$SemanticAttributes.HTTP_HOST.key" "localhost:$port"
+            "$SemanticAttributes.HTTP_TARGET.key" "/api/firstModule/unit/unitOne"
             "$SemanticAttributes.HTTP_STATUS_CODE.key" 200
             "$SemanticAttributes.HTTP_USER_AGENT.key" String
             "$SemanticAttributes.HTTP_FLAVOR.key" "1.1"

diff --git a/...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy b/...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy
@@ -124,7 +124,9 @@ class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecifica
           attributes {
             "$SemanticAttributes.HTTP_METHOD.key" "POST"
             "$SemanticAttributes.HTTP_STATUS_CODE.key" 200
-            "$SemanticAttributes.HTTP_URL.key" "http://127.0.0.1:$portTwo/serviceTwo"
+            "$SemanticAttributes.HTTP_SCHEME.key" "http"
+            "$SemanticAttributes.HTTP_HOST.key" "127.0.0.1:$portTwo"
+            "$SemanticAttributes.HTTP_TARGET.key" "/serviceTwo"
             "$SemanticAttributes.NET_PEER_PORT.key" Number
             "$SemanticAttributes.NET_PEER_IP.key" "127.0.0.1"
             "$SemanticAttributes.HTTP_USER_AGENT.key" "Jakarta Commons-HttpClient/3.1"

diff --git a/...a/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpServerAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpServerAttributesExtractor.java
@@ -24,11 +24,6 @@ protected String method(RequestContext ctx) {
     return ctx.method().name();
   }
 
-  @Override
-  protected String url(RequestContext ctx) {
-    return request(ctx).uri().toString();
-  }
-
   @Override
   protected String target(RequestContext ctx) {
     return request(ctx).path();

diff --git a/...groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy b/...groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy
@@ -38,13 +38,10 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest<Server> {
   @Override
   List<AttributeKey<?>> extraAttributes() {
     [
-      SemanticAttributes.HTTP_HOST,
       SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
       SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH,
       SemanticAttributes.HTTP_ROUTE,
-      SemanticAttributes.HTTP_SCHEME,
       SemanticAttributes.HTTP_SERVER_NAME,
-      SemanticAttributes.HTTP_TARGET,
       SemanticAttributes.NET_PEER_NAME,
       SemanticAttributes.NET_TRANSPORT
     ]

diff --git a/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy b/instrumentation/dropwizard-testing/src/test/groovy/DropwizardTest.groovy
@@ -113,7 +113,9 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> implements Ag
         // dropwizard reports peer ip as the client ip
         "${SemanticAttributes.NET_PEER_IP.key}" TEST_CLIENT_IP
         "${SemanticAttributes.NET_PEER_PORT.key}" Long
-        "${SemanticAttributes.HTTP_URL.key}" { it == "${endpoint.resolve(address)}" || it == "${endpoint.resolveWithoutFragment(address)}" }
+        "${SemanticAttributes.HTTP_SCHEME.key}" "http"
+        "${SemanticAttributes.HTTP_HOST}" "localhost:${port}"
+        "${SemanticAttributes.HTTP_TARGET}" endpoint.resolvePath(address).getPath() + "${endpoint == QUERY_PARAM ? "?${endpoint.body}" : ""}"
         "${SemanticAttributes.HTTP_METHOD.key}" method
         "${SemanticAttributes.HTTP_STATUS_CODE.key}" endpoint.status
         "${SemanticAttributes.HTTP_FLAVOR.key}" "1.1"