Remove url from HttpServerAttributesExtractor

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

instrumentation-api/src/main/java/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);
 

instrumentation-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);
 

instrumentation-api/src/test/java/io/opentelemetry/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:https://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:https://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:https://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"),

instrumentation/akka-http-10.0/javaagent/src/main/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

instrumentation/apache-camel-2.20/javaagent/src/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:https://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"

instrumentation/apache-camel-2.20/javaagent/src/test/groovy/io/opentelemetry/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:https://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"

instrumentation/armeria-1.3/library/src/main/java/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();

instrumentation/armeria-1.3/testing/src/main/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
  ]

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"