HttpClientTracer cleanup: apache-httpclient (#1907)

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v2_0/ApacheHttpClientInstrumentationModule.java

@@ -74,7 +74,7 @@ public class ApacheHttpClientInstrumentationModule extends InstrumentationModule
         return;
       }
 
-      context = tracer().startSpan(parentContext, httpMethod, httpMethod);
+      context = tracer().startSpan(parentContext, httpMethod);
       scope = context.makeCurrent();
     }
 
@@ -89,11 +89,7 @@ public class ApacheHttpClientInstrumentationModule extends InstrumentationModule
       }
 
       scope.close();
-      if (throwable == null) {
+      tracer().endMaybeExceptionally(context, httpMethod, throwable);
-        tracer().end(context, httpMethod);
-      } else {
-        tracer().endExceptionally(context, httpMethod, throwable);
-      }
     }
   }
 }

instrumentation/apache-httpclient/apache-httpclient-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v2_0/CommonsHttpClientTracer.java

@@ -5,7 +5,8 @@
 
 package io.opentelemetry.javaagent.instrumentation.apachehttpclient.v2_0;
 
-import io.opentelemetry.context.propagation.TextMapPropagator.Setter;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.propagation.TextMapPropagator;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -21,6 +22,10 @@ public class CommonsHttpClientTracer extends HttpClientTracer<HttpMethod, HttpMe
     return TRACER;
   }
 
+  public Context startSpan(Context parentContext, HttpMethod request) {
+    return super.startSpan(parentContext, request, request);
+  }
+
   @Override
   protected String getInstrumentationName() {
     return "io.opentelemetry.javaagent.apache-httpclient";
@@ -60,7 +65,7 @@ public class CommonsHttpClientTracer extends HttpClientTracer<HttpMethod, HttpMe
   }
 
   @Override
-  protected Setter<HttpMethod> getSetter() {
+  protected TextMapPropagator.Setter<HttpMethod> getSetter() {
     return HttpHeadersInjectAdapter.SETTER;
   }
 }

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientHelper.java

@@ -7,21 +7,18 @@ package io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0;
 
 import static io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0.ApacheHttpClientTracer.tracer;
 
-import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import org.apache.http.HttpResponse;
 
 public class ApacheHttpClientHelper {
 
   public static void doMethodExit(Context context, Object result, Throwable throwable) {
-    if (result instanceof HttpResponse) {
-      tracer().onResponse(Span.fromContext(context), (HttpResponse) result);
-    } // else they probably provided a ResponseHandler
-
     if (throwable != null) {
       tracer().endExceptionally(context, throwable);
+    } else if (result instanceof HttpResponse) {
+      tracer().end(context, (HttpResponse) result);
     } else {
-      tracer().end(context);
+      // ended in WrappingStatusSettingResponseHandler
     }
   }
 }

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientInstrumentationModule.java

@@ -157,7 +157,7 @@ public class ApacheHttpClientInstrumentationModule extends InstrumentationModule
         return;
       }
 
-      context = tracer().startSpan(parentContext, request, request);
+      context = tracer().startSpan(parentContext, request);
       scope = context.makeCurrent();
     }
 
@@ -194,12 +194,12 @@ public class ApacheHttpClientInstrumentationModule extends InstrumentationModule
         return;
       }
 
-      context = tracer().startSpan(parentContext, request, request);
+      context = tracer().startSpan(parentContext, request);
       scope = context.makeCurrent();
 
       // Wrap the handler so we capture the status code
       if (handler instanceof ResponseHandler) {
-        handler = new WrappingStatusSettingResponseHandler(context, (ResponseHandler) handler);
+        handler = new WrappingStatusSettingResponseHandler(context, (ResponseHandler<?>) handler);
       }
     }
 
@@ -230,13 +230,7 @@ public class ApacheHttpClientInstrumentationModule extends InstrumentationModule
         return;
       }
 
-      HttpUriRequest httpUriRequest;
+      context = tracer().startSpan(parentContext, host, request);
-      if (request instanceof HttpUriRequest) {
-        httpUriRequest = (HttpUriRequest) request;
-      } else {
-        httpUriRequest = new HostAndRequestAsHttpUriRequest(host, request);
-      }
-      context = tracer().startSpan(parentContext, httpUriRequest, httpUriRequest);
       scope = context.makeCurrent();
     }
 
@@ -274,18 +268,12 @@ public class ApacheHttpClientInstrumentationModule extends InstrumentationModule
         return;
       }
 
-      HttpUriRequest httpUriRequest;
+      context = tracer().startSpan(parentContext, host, request);
-      if (request instanceof HttpUriRequest) {
-        httpUriRequest = (HttpUriRequest) request;
-      } else {
-        httpUriRequest = new HostAndRequestAsHttpUriRequest(host, request);
-      }
-      context = tracer().startSpan(parentContext, httpUriRequest, httpUriRequest);
       scope = context.makeCurrent();
 
       // Wrap the handler so we capture the status code
       if (handler instanceof ResponseHandler) {
-        handler = new WrappingStatusSettingResponseHandler(context, (ResponseHandler) handler);
+        handler = new WrappingStatusSettingResponseHandler(context, (ResponseHandler<?>) handler);
       }
     }
 

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientTracer.java

@@ -7,12 +7,14 @@ package io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0;
 
 import static io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0.HttpHeadersInjectAdapter.SETTER;
 
-import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Context;
-import io.opentelemetry.context.propagation.TextMapPropagator.Setter;
+import io.opentelemetry.context.propagation.TextMapPropagator;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
 import java.net.URI;
 import org.apache.http.Header;
+import org.apache.http.HttpHost;
 import org.apache.http.HttpMessage;
+import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
 import org.apache.http.client.methods.HttpUriRequest;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -26,6 +28,20 @@ public class ApacheHttpClientTracer
     return TRACER;
   }
 
+  public Context startSpan(Context parentContext, HttpHost host, HttpRequest request) {
+    HttpUriRequest httpUriRequest;
+    if (request instanceof HttpUriRequest) {
+      httpUriRequest = (HttpUriRequest) request;
+    } else {
+      httpUriRequest = new HostAndRequestAsHttpUriRequest(host, request);
+    }
+    return startSpan(parentContext, httpUriRequest);
+  }
+
+  public Context startSpan(Context parentContext, HttpUriRequest request) {
+    return startSpan(parentContext, request, request);
+  }
+
   @Override
   protected String method(HttpUriRequest httpRequest) {
     return httpRequest.getMethod();
@@ -57,7 +73,7 @@ public class ApacheHttpClientTracer
   }
 
   @Override
-  protected Setter<HttpUriRequest> getSetter() {
+  protected TextMapPropagator.Setter<HttpUriRequest> getSetter() {
     return SETTER;
   }
 
@@ -70,10 +86,4 @@ public class ApacheHttpClientTracer
   protected String getInstrumentationName() {
     return "io.opentelemetry.javaagent.apache-httpclient";
   }
-
-  /** This method is overridden to allow other classes in this package to call it. */
-  @Override
-  protected void onResponse(Span span, HttpResponse httpResponse) {
-    super.onResponse(span, httpResponse);
-  }
 }

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ContextScopePair.java

@@ -1,27 +0,0 @@
-/*
- * Copyright The OpenTelemetry Authors
- * SPDX-License-Identifier: Apache-2.0
- */
-
-package io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0;
-
-import io.opentelemetry.context.Context;
-import io.opentelemetry.context.Scope;
-
-public class ContextScopePair {
-  private final Context context;
-  private final Scope scope;
-
-  public ContextScopePair(Context context, Scope scope) {
-    this.context = context;
-    this.scope = scope;
-  }
-
-  public Context getContext() {
-    return context;
-  }
-
-  public void closeScope() {
-    scope.close();
-  }
-}

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/WrappingStatusSettingResponseHandler.java

@@ -7,26 +7,23 @@ package io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0;
 
 import static io.opentelemetry.javaagent.instrumentation.apachehttpclient.v4_0.ApacheHttpClientTracer.tracer;
 
-import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import java.io.IOException;
 import org.apache.http.HttpResponse;
 import org.apache.http.client.ResponseHandler;
 
-public class WrappingStatusSettingResponseHandler implements ResponseHandler {
+public class WrappingStatusSettingResponseHandler<T> implements ResponseHandler<T> {
   final Context context;
-  final ResponseHandler handler;
+  final ResponseHandler<T> handler;
 
-  public WrappingStatusSettingResponseHandler(Context context, ResponseHandler handler) {
+  public WrappingStatusSettingResponseHandler(Context context, ResponseHandler<T> handler) {
     this.context = context;
     this.handler = handler;
   }
 
   @Override
-  public Object handleResponse(HttpResponse response) throws IOException {
+  public T handleResponse(HttpResponse response) throws IOException {
-    if (context != null) {
+    tracer().end(context, response);
-      tracer().onResponse(Span.fromContext(context), response);
-    }
     return handler.handleResponse(response);
   }
 }