Update Apache HttpAsyncClient to Instrumenter API (#3490)

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java

@@ -40,7 +40,10 @@ public final class HttpSpanStatusExtractor<REQUEST, RESPONSE>
     if (response != null) {
       Integer statusCode = attributesExtractor.statusCode(request, response);
       if (statusCode != null) {
-        return HttpStatusConverter.statusFromHttpStatus(statusCode);
+        StatusCode statusCodeObj = HttpStatusConverter.statusFromHttpStatus(statusCode);
+        if (statusCodeObj == StatusCode.ERROR) {
+          return statusCodeObj;
+        }
       }
     }
     return SpanStatusExtractor.getDefault().extract(request, response, error);

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractorTest.java

@@ -37,7 +37,7 @@ class HttpSpanStatusExtractorTest {
 
   @ParameterizedTest
   @ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
-  void hasStatus_ignoresException(int statusCode) {
+  void hasStatusAndException(int statusCode) {
     when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
 
     // Presence of exception has no effect.
@@ -45,11 +45,11 @@ class HttpSpanStatusExtractorTest {
             HttpSpanStatusExtractor.create(extractor)
                 .extract(
                     Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
-        .isEqualTo(HttpStatusConverter.statusFromHttpStatus(statusCode));
+        .isEqualTo(StatusCode.ERROR);
   }
 
   @Test
-  void fallsBackToDefault_unset() {
+  void hasNoStatus_fallsBackToDefault_unset() {
     when(extractor.statusCode(anyMap(), anyMap())).thenReturn(null);
 
     assertThat(
@@ -59,7 +59,7 @@ class HttpSpanStatusExtractorTest {
   }
 
   @Test
-  void fallsBackToDefault_error() {
+  void hasNoStatus_fallsBackToDefault_error() {
     when(extractor.statusCode(anyMap(), anyMap())).thenReturn(null);
 
     assertThat(

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesExtractor.java

@@ -0,0 +1,106 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
+
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
+import org.apache.http.HttpResponse;
+import org.apache.http.StatusLine;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+final class ApacheHttpAsyncClientHttpAttributesExtractor
+    extends HttpAttributesExtractor<ApacheHttpClientRequest, HttpResponse> {
+
+  @Override
+  protected String method(ApacheHttpClientRequest request) {
+    return request.getMethod();
+  }
+
+  @Override
+  protected String url(ApacheHttpClientRequest request) {
+    return request.getUrl();
+  }
+
+  @Override
+  protected String target(ApacheHttpClientRequest request) {
+    return request.getTarget();
+  }
+
+  @Override
+  @Nullable
+  protected String host(ApacheHttpClientRequest request) {
+    return request.getHost();
+  }
+
+  @Override
+  @Nullable
+  protected String scheme(ApacheHttpClientRequest request) {
+    return request.getScheme();
+  }
+
+  @Override
+  @Nullable
+  protected String userAgent(ApacheHttpClientRequest request) {
+    return request.getHeader("User-Agent");
+  }
+
+  @Override
+  @Nullable
+  protected Long requestContentLength(
+      ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return null;
+  }
+
+  @Override
+  @Nullable
+  protected Long requestContentLengthUncompressed(
+      ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return null;
+  }
+
+  @Override
+  @Nullable
+  protected Integer statusCode(ApacheHttpClientRequest request, HttpResponse response) {
+    StatusLine statusLine = response.getStatusLine();
+    return statusLine != null ? statusLine.getStatusCode() : null;
+  }
+
+  @Override
+  @Nullable
+  protected String flavor(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return request.getFlavor();
+  }
+
+  @Override
+  @Nullable
+  protected Long responseContentLength(ApacheHttpClientRequest request, HttpResponse response) {
+    return null;
+  }
+
+  @Override
+  @Nullable
+  protected Long responseContentLengthUncompressed(
+      ApacheHttpClientRequest request, HttpResponse response) {
+    return null;
+  }
+
+  @Override
+  @Nullable
+  protected String serverName(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return null;
+  }
+
+  @Override
+  @Nullable
+  protected String route(ApacheHttpClientRequest request) {
+    return null;
+  }
+
+  @Override
+  @Nullable
+  protected String clientIp(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return null;
+  }
+}

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java

@@ -7,7 +7,7 @@ package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
 import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
-import static io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientTracer.tracer;
+import static io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientSingletons.instrumenter;
 import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.named;
@@ -70,9 +70,6 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
         @Advice.Argument(value = 3, readOnly = false) FutureCallback<?> futureCallback) {
 
       Context parentContext = currentContext();
-      if (!tracer().shouldStartSpan(parentContext)) {
-        return;
-      }
 
       WrappedFutureCallback<?> wrappedFutureCallback =
           new WrappedFutureCallback<>(parentContext, httpContext, futureCallback);
@@ -104,7 +101,14 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
     @Override
     public HttpRequest generateRequest() throws IOException, HttpException {
       HttpRequest request = delegate.generateRequest();
-      wrappedFutureCallback.context = tracer().startSpan(parentContext, request, request);
+
+      ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(request);
+
+      if (instrumenter().shouldStart(parentContext, otelRequest)) {
+        wrappedFutureCallback.context = instrumenter().start(parentContext, otelRequest);
+        wrappedFutureCallback.otelRequest = otelRequest;
+      }
+
       return request;
     }
 
@@ -148,6 +152,7 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
     private final FutureCallback<T> delegate;
 
     private volatile Context context;
+    private volatile ApacheHttpClientRequest otelRequest;
 
     public WrappedFutureCallback(
         Context parentContext, HttpContext httpContext, FutureCallback<T> delegate) {
@@ -166,7 +171,7 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
         return;
       }
 
-      tracer().end(context, getResponse(httpContext));
+      instrumenter().end(context, otelRequest, getResponse(httpContext), null);
 
       if (parentContext == null) {
         completeDelegate(result);
@@ -188,7 +193,7 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
       }
 
       // end span before calling delegate
-      tracer().endExceptionally(context, getResponse(httpContext), ex);
+      instrumenter().end(context, otelRequest, getResponse(httpContext), ex);
 
       if (parentContext == null) {
         failDelegate(ex);
@@ -209,8 +214,9 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
         return;
       }
 
+      // TODO (trask) add "canceled" span attribute
       // end span before calling delegate
-      tracer().end(context, getResponse(httpContext));
+      instrumenter().end(context, otelRequest, getResponse(httpContext), null);
 
       if (parentContext == null) {
         cancelDelegate();

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientNetAttributesExtractor.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
+
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetAttributesExtractor;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import org.apache.http.HttpResponse;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+final class ApacheHttpAsyncClientNetAttributesExtractor
+    extends NetAttributesExtractor<ApacheHttpClientRequest, HttpResponse> {
+
+  @Override
+  public String transport(ApacheHttpClientRequest request) {
+    return SemanticAttributes.NetTransportValues.IP_TCP;
+  }
+
+  @Override
+  public @Nullable String peerName(
+      ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return request.getPeerName();
+  }
+
+  @Override
+  public Integer peerPort(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return request.getPeerPort();
+  }
+
+  @Override
+  public @Nullable String peerIp(ApacheHttpClientRequest request, @Nullable HttpResponse response) {
+    return null;
+  }
+}

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientSingletons.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
+
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpAttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.javaagent.instrumentation.api.instrumenter.PeerServiceAttributesExtractor;
+import org.apache.http.HttpResponse;
+
+public final class ApacheHttpAsyncClientSingletons {
+  private static final String INSTRUMENTATION_NAME = "io.opentelemetry.apache-httpasyncclient-4.1";
+
+  private static final Instrumenter<ApacheHttpClientRequest, HttpResponse> INSTRUMENTER;
+
+  static {
+    HttpAttributesExtractor<ApacheHttpClientRequest, HttpResponse> httpAttributesExtractor =
+        new ApacheHttpAsyncClientHttpAttributesExtractor();
+    SpanNameExtractor<? super ApacheHttpClientRequest> spanNameExtractor =
+        HttpSpanNameExtractor.create(httpAttributesExtractor);
+    SpanStatusExtractor<? super ApacheHttpClientRequest, ? super HttpResponse> spanStatusExtractor =
+        HttpSpanStatusExtractor.create(httpAttributesExtractor);
+    ApacheHttpAsyncClientNetAttributesExtractor netAttributesExtractor =
+        new ApacheHttpAsyncClientNetAttributesExtractor();
+
+    INSTRUMENTER =
+        Instrumenter.<ApacheHttpClientRequest, HttpResponse>newBuilder(
+                GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, spanNameExtractor)
+            .setSpanStatusExtractor(spanStatusExtractor)
+            .addAttributesExtractor(httpAttributesExtractor)
+            .addAttributesExtractor(netAttributesExtractor)
+            .addAttributesExtractor(PeerServiceAttributesExtractor.create(netAttributesExtractor))
+            .newClientInstrumenter(new HttpHeaderSetter());
+  }
+
+  public static Instrumenter<ApacheHttpClientRequest, HttpResponse> instrumenter() {
+    return INSTRUMENTER;
+  }
+
+  private ApacheHttpAsyncClientSingletons() {}
+}

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java

@@ -1,124 +0,0 @@
-/*
- * Copyright The OpenTelemetry Authors
- * SPDX-License-Identifier: Apache-2.0
- */
-
-package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
-
-import static io.opentelemetry.api.trace.SpanKind.CLIENT;
-import static io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient.HttpHeadersInjectAdapter.SETTER;
-
-import io.opentelemetry.api.trace.Span;
-import io.opentelemetry.context.Context;
-import io.opentelemetry.context.propagation.TextMapSetter;
-import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
-import io.opentelemetry.instrumentation.api.tracer.net.NetPeerAttributes;
-import java.net.URI;
-import java.net.URISyntaxException;
-import org.apache.http.Header;
-import org.apache.http.HttpMessage;
-import org.apache.http.HttpRequest;
-import org.apache.http.HttpResponse;
-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> {
-
-  private static final ApacheHttpAsyncClientTracer TRACER = new ApacheHttpAsyncClientTracer();
-
-  private ApacheHttpAsyncClientTracer() {
-    super(NetPeerAttributes.INSTANCE);
-  }
-
-  public static ApacheHttpAsyncClientTracer tracer() {
-    return TRACER;
-  }
-
-  public Context startSpan(Context parentContext) {
-    Span span = spanBuilder(parentContext, DEFAULT_SPAN_NAME, CLIENT).startSpan();
-    return withClientSpan(parentContext, span);
-  }
-
-  @Override
-  protected String method(HttpRequest request) {
-    if (request instanceof HttpUriRequest) {
-      return ((HttpUriRequest) request).getMethod();
-    } else {
-      RequestLine requestLine = request.getRequestLine();
-      return requestLine == null ? null : requestLine.getMethod();
-    }
-  }
-
-  @Override
-  @Nullable
-  protected String flavor(HttpRequest httpRequest) {
-    return httpRequest.getProtocolVersion().toString();
-  }
-
-  @Override
-  protected URI url(HttpRequest request) throws URISyntaxException {
-    /*
-     * Note: this is essentially an optimization: HttpUriRequest allows quicker access to required information.
-     * The downside is that we need to load HttpUriRequest which essentially means we depend on httpasyncclient
-     * library depending on httpclient library. Currently this seems to be the case.
-     */
-    if (request instanceof HttpUriRequest) {
-      return ((HttpUriRequest) request).getURI();
-    } else {
-      RequestLine requestLine = request.getRequestLine();
-      return requestLine == null ? null : new URI(requestLine.getUri());
-    }
-  }
-
-  @Override
-  protected Integer status(HttpResponse response) {
-    StatusLine statusLine = response.getStatusLine();
-    return statusLine != null ? statusLine.getStatusCode() : null;
-  }
-
-  @Override
-  protected String requestHeader(HttpRequest request, String name) {
-    return header(request, name);
-  }
-
-  @Override
-  protected String responseHeader(HttpResponse response, String name) {
-    return header(response, name);
-  }
-
-  @Override
-  protected TextMapSetter<HttpRequest> getSetter() {
-    return SETTER;
-  }
-
-  private static String header(HttpMessage message, String name) {
-    Header header = message.getFirstHeader(name);
-    return header != null ? header.getValue() : null;
-  }
-
-  @Override
-  protected String getInstrumentationName() {
-    return "io.opentelemetry.apache-httpasyncclient-4.1";
-  }
-
-  /** This method is overridden to allow other classes in this package to call it. */
-  @Override
-  public String spanNameForRequest(HttpRequest httpRequest) {
-    return super.spanNameForRequest(httpRequest);
-  }
-
-  /** This method is overridden to allow other classes in this package to call it. */
-  @Override
-  protected void inject(Context context, HttpRequest httpRequest) {
-    super.inject(context, httpRequest);
-  }
-
-  /** This method is overridden to allow other classes in this package to call it. */
-  @Override
-  protected void onRequest(Span span, HttpRequest httpRequest) {
-    super.onRequest(span, httpRequest);
-  }
-}

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

@@ -0,0 +1,136 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
+
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import java.net.URI;
+import java.net.URISyntaxException;
+import org.apache.http.Header;
+import org.apache.http.HttpRequest;
+import org.apache.http.ProtocolVersion;
+import org.apache.http.RequestLine;
+import org.apache.http.client.methods.HttpUriRequest;
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public final class ApacheHttpClientRequest {
+
+  private static final Logger logger = LoggerFactory.getLogger(ApacheHttpClientRequest.class);
+
+  @Nullable private final URI uri;
+
+  private final HttpRequest delegate;
+
+  public ApacheHttpClientRequest(HttpRequest httpRequest) {
+    URI calculatedUri;
+    if (httpRequest instanceof HttpUriRequest) {
+      // Note: this is essentially an optimization: HttpUriRequest allows quicker access to required
+      // information. The downside is that we need to load HttpUriRequest which essentially means we
+      // depend on httpasyncclient library depending on httpclient library. Currently this seems to
+      // be the case.
+      calculatedUri = ((HttpUriRequest) httpRequest).getURI();
+    } else {
+      RequestLine requestLine = httpRequest.getRequestLine();
+      try {
+        calculatedUri = new URI(requestLine.getUri());
+      } catch (URISyntaxException e) {
+        logger.debug(e.getMessage(), e);
+        calculatedUri = null;
+      }
+    }
+    uri = calculatedUri;
+    delegate = httpRequest;
+  }
+
+  public String getHeader(String name) {
+    Header header = delegate.getFirstHeader(name);
+    return header != null ? header.getValue() : null;
+  }
+
+  public void setHeader(String name, String value) {
+    delegate.setHeader(name, value);
+  }
+
+  public String getMethod() {
+    return delegate.getRequestLine().getMethod();
+  }
+
+  public String getUrl() {
+    return uri != null ? uri.toString() : null;
+  }
+
+  public String getTarget() {
+    if (uri == null) {
+      return null;
+    }
+    String pathString = uri.getPath();
+    String queryString = uri.getQuery();
+    if (pathString != null && queryString != null) {
+      return pathString + "?" + queryString;
+    } else if (queryString != null) {
+      return "?" + queryString;
+    } else {
+      return pathString;
+    }
+  }
+
+  public String getHost() {
+    Header header = delegate.getFirstHeader("Host");
+    if (header != null) {
+      return header.getValue();
+    }
+    return null;
+  }
+
+  public String getScheme() {
+    return uri != null ? uri.getScheme() : null;
+  }
+
+  public String getFlavor() {
+    ProtocolVersion protocolVersion = delegate.getProtocolVersion();
+    String protocol = protocolVersion.getProtocol();
+    if (!protocol.equals("HTTP")) {
+      return null;
+    }
+    int major = protocolVersion.getMajor();
+    int minor = protocolVersion.getMinor();
+    if (major == 1 && minor == 0) {
+      return SemanticAttributes.HttpFlavorValues.HTTP_1_0;
+    }
+    if (major == 1 && minor == 1) {
+      return SemanticAttributes.HttpFlavorValues.HTTP_1_1;
+    }
+    if (major == 2 && minor == 0) {
+      return SemanticAttributes.HttpFlavorValues.HTTP_2_0;
+    }
+    logger.debug("unexpected http protocol version: " + protocolVersion);
+    return null;
+  }
+
+  public String getPeerName() {
+    return uri != null ? uri.getHost() : null;
+  }
+
+  public Integer getPeerPort() {
+    if (uri == null) {
+      return null;
+    }
+    int port = uri.getPort();
+    if (port != -1) {
+      return port;
+    }
+    switch (uri.getScheme()) {
+      case "http":
+        return 80;
+      case "https":
+        return 443;
+      default:
+        logger.debug("no default port mapping for scheme: {}", uri.getScheme());
+        return null;
+    }
+  }
+}

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/HttpHeaderSetter.java

@@ -6,14 +6,11 @@
 package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
 
 import io.opentelemetry.context.propagation.TextMapSetter;
-import org.apache.http.HttpRequest;
 
-public class HttpHeadersInjectAdapter implements TextMapSetter<HttpRequest> {
+public class HttpHeaderSetter implements TextMapSetter<ApacheHttpClientRequest> {
-
-  public static final HttpHeadersInjectAdapter SETTER = new HttpHeadersInjectAdapter();
 
   @Override
-  public void set(HttpRequest carrier, String key, String value) {
+  public void set(ApacheHttpClientRequest carrier, String key, String value) {
     carrier.setHeader(key, value);
   }
 }

instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy

@@ -3,8 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import io.opentelemetry.api.common.AttributeKey
 import io.opentelemetry.instrumentation.test.AgentTestTrait
 import io.opentelemetry.instrumentation.test.base.HttpClientTest
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
 import java.util.concurrent.CancellationException
 import org.apache.http.HttpResponse
 import org.apache.http.client.config.RequestConfig
@@ -77,4 +79,13 @@ class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implement
   boolean testCausality() {
     false
   }
+
+  @Override
+  Set<AttributeKey<?>> httpAttributes(URI uri) {
+    Set<AttributeKey<?>> extra = [
+      SemanticAttributes.HTTP_SCHEME,
+      SemanticAttributes.HTTP_TARGET
+    ]
+    super.httpAttributes(uri) + extra
+  }
 }