HTTP client instrumentation cleanup, part 0 (#1893)

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java

@@ -67,6 +67,16 @@ public abstract class BaseTracer {
     return tracer.spanBuilder(spanName).setSpanKind(kind).startSpan();
   }
 
+  // TODO (trask) make the key private
+  protected final boolean inClientSpan(Context parentContext) {
+    return parentContext.get(CONTEXT_CLIENT_SPAN_KEY) != null;
+  }
+
+  // TODO (trask) make the key private
+  protected final Context withClientSpan(Context parentContext, Span span) {
+    return parentContext.with(span).with(CONTEXT_CLIENT_SPAN_KEY, span);
+  }
+
   public Scope startScope(Span span) {
     return Context.current().with(span).makeCurrent();
   }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java

@@ -60,7 +60,7 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
   }
 
   public boolean shouldStartSpan(Context parentContext) {
-    return parentContext.get(CONTEXT_CLIENT_SPAN_KEY) == null;
+    return !inClientSpan(parentContext);
   }
 
   public Context startSpan(Context parentContext, REQUEST request, CARRIER carrier) {
@@ -71,14 +71,18 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
       Context parentContext, REQUEST request, CARRIER carrier, long startTimeNanos) {
     Span span =
         internalStartSpan(parentContext, request, spanNameForRequest(request), startTimeNanos);
+    Context context = withClientSpan(parentContext, span);
+    inject(context, carrier);
+    return context;
+  }
+
+  private void inject(Context context, CARRIER carrier) {
     Setter<CARRIER> setter = getSetter();
     if (setter == null) {
       throw new IllegalStateException(
           "getSetter() not defined but calling startScope(), either getSetter must be implemented or the scope should be setup manually");
     }
-    Context context = parentContext.with(span).with(CONTEXT_CLIENT_SPAN_KEY, span);
     OpenTelemetry.getGlobalPropagators().getTextMapPropagator().inject(context, carrier, setter);
-    return context;
   }
 
   public void end(Context context, RESPONSE response) {
@@ -112,6 +116,18 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
     super.endExceptionally(span, throwable, -1);
   }
 
+  // TODO (trask) see if we can reduce the number of end..() variants
+  //  see https://github.com/open-telemetry/opentelemetry-java-instrumentation
+  //                        /pull/1893#discussion_r542111699
+  public void endMaybeExceptionally(
+      Context context, RESPONSE response, @Nullable Throwable throwable) {
+    if (throwable != null) {
+      endExceptionally(context, throwable);
+    } else {
+      end(context, response);
+    }
+  }
+
   private Span internalStartSpan(
       Context parentContext, REQUEST request, String name, long startTimeNanos) {
     SpanBuilder spanBuilder =
@@ -124,7 +140,7 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
     return span;
   }
 
-  protected Span onRequest(Span span, REQUEST request) {
+  protected void onRequest(Span span, REQUEST request) {
     assert span != null;
     if (request != null) {
       span.setAttribute(SemanticAttributes.NET_TRANSPORT, "IP.TCP");
@@ -134,7 +150,6 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
       setFlavor(span, request);
       setUrl(span, request);
     }
-    return span;
   }
 
   private void setFlavor(Span span, REQUEST request) {
@@ -163,7 +178,7 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
     }
   }
 
-  protected Span onResponse(Span span, RESPONSE response) {
+  protected void onResponse(Span span, RESPONSE response) {
     assert span != null;
     if (response != null) {
       Integer status = status(response);
@@ -172,7 +187,6 @@ public abstract class HttpClientTracer<REQUEST, CARRIER, RESPONSE> extends BaseT
         span.setStatus(HttpStatusConverter.statusFromHttpStatus(status));
       }
     }
-    return span;
   }
 
   protected String spanNameForRequest(REQUEST request) {

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

@@ -110,7 +110,7 @@ public class ApacheHttpAsyncClientTracer
 
   /** This method is overridden to allow other classes in this package to call it. */
   @Override
-  public Span onRequest(Span span, HttpRequest httpRequest) {
+  protected void onRequest(Span span, HttpRequest httpRequest) {
-    return super.onRequest(span, httpRequest);
+    super.onRequest(span, httpRequest);
   }
 }

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

@@ -73,7 +73,7 @@ public class ApacheHttpClientTracer
 
   /** This method is overridden to allow other classes in this package to call it. */
   @Override
-  protected Span onResponse(Span span, HttpResponse httpResponse) {
+  protected void onResponse(Span span, HttpResponse httpResponse) {
-    return super.onResponse(span, httpResponse);
+    super.onResponse(span, httpResponse);
   }
 }

instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/AwsSdkClientTracer.java

@@ -64,12 +64,12 @@ public class AwsSdkClientTracer extends HttpClientTracer<Request<?>, Request<?>,
   }
 
   @Override
-  public Span onResponse(Span span, Response<?> response) {
+  public void onResponse(Span span, Response<?> response) {
     if (response != null && response.getAwsResponse() instanceof AmazonWebServiceResponse) {
       AmazonWebServiceResponse awsResp = (AmazonWebServiceResponse) response.getAwsResponse();
       span.setAttribute("aws.requestId", awsResp.getRequestId());
     }
-    return super.onResponse(span, response);
+    super.onResponse(span, response);
   }
 
   private String qualifiedOperation(String service, Class<?> operation) {

instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AwsSdkHttpClientTracer.java

@@ -74,8 +74,8 @@ final class AwsSdkHttpClientTracer
 
   /** This method is overridden to allow other classes in this package to call it. */
   @Override
-  protected Span onRequest(Span span, SdkHttpRequest sdkHttpRequest) {
+  protected void onRequest(Span span, SdkHttpRequest sdkHttpRequest) {
-    return super.onRequest(span, sdkHttpRequest);
+    super.onRequest(span, sdkHttpRequest);
   }
 
   public Context startSpan(Context parentContext, String name, Tracer tracer, Kind kind) {

instrumentation/java-httpclient/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java

@@ -61,16 +61,14 @@ public class JdkHttpClientTracer
   }
 
   @Override
-  protected Span onResponse(Span span, HttpResponse<?> httpResponse) {
+  protected void onResponse(Span span, HttpResponse<?> httpResponse) {
-    span = super.onResponse(span, httpResponse);
+    super.onResponse(span, httpResponse);
 
     if (httpResponse != null) {
       span.setAttribute(
           SemanticAttributes.HTTP_FLAVOR,
           httpResponse.version() == Version.HTTP_1_1 ? "1.1" : "2.0");
     }
-
-    return span;
   }
 
   @Override

instrumentation/khttp-0.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/khttp/KHttpAdvice.java

@@ -45,10 +45,6 @@ public class KHttpAdvice {
     }
 
     scope.close();
-    if (throwable == null) {
+    tracer().endMaybeExceptionally(context, response, throwable);
-      tracer().end(context, response);
-    } else {
-      tracer().endExceptionally(context, response, throwable);
-    }
   }
 }

instrumentation/kubernetes-client-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java

@@ -82,7 +82,7 @@ public class KubernetesClientTracer extends HttpClientTracer<Request, Request, R
 
   /** This method is overridden to allow other classes in this package to call it. */
   @Override
-  protected Span onRequest(Span span, Request request) {
+  protected void onRequest(Span span, Request request) {
-    return super.onRequest(span, request);
+    super.onRequest(span, request);
   }
 }