HTTP client instrumentation cleanup: google-http-client (#1910)

* HTTP client instrumentation cleanup: google-http-client

* Bring back comment

* Feedback

instrumentation/google-http-client-1.19/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/googlehttpclient/GoogleHttpClientInstrumentationModule.java

@@ -6,7 +6,6 @@
 package io.opentelemetry.javaagent.instrumentation.googlehttpclient;
 
 import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext;
-import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.spanFromContext;
 import static io.opentelemetry.javaagent.instrumentation.googlehttpclient.GoogleHttpClientTracer.tracer;
 import static java.util.Collections.singletonList;
 import static java.util.Collections.singletonMap;
@@ -19,10 +18,8 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 import com.google.api.client.http.HttpRequest;
 import com.google.api.client.http.HttpResponse;
 import com.google.auto.service.AutoService;
-import io.opentelemetry.api.trace.StatusCode;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
-import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
 import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
 import io.opentelemetry.javaagent.tooling.InstrumentationModule;
 import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
@@ -85,21 +82,20 @@ public class GoogleHttpClientInstrumentationModule extends InstrumentationModule
         @Advice.Local("otelContext") Context context,
         @Advice.Local("otelScope") Scope scope) {
 
-      ContextStore<HttpRequest, Context> contextStore =
+      context = InstrumentationContext.get(HttpRequest.class, Context.class).get(request);
-          InstrumentationContext.get(HttpRequest.class, Context.class);
+      if (context != null) {
-      context = contextStore.get(request);
-
-      if (context == null) {
-        Context parentContext = currentContext();
-        if (tracer().shouldStartSpan(parentContext)) {
-          context = tracer().startSpan(parentContext, request, request.getHeaders());
-          contextStore.put(request, context);
-          scope = context.makeCurrent();
-        }
-      } else {
         // span was created by GoogleHttpClientAsyncAdvice instrumentation below
+        // (executeAsync ends up calling execute from a separate thread)
+        // so make it current and end it in method exit
         scope = context.makeCurrent();
+        return;
       }
+      Context parentContext = currentContext();
+      if (!tracer().shouldStartSpan(parentContext)) {
+        return;
+      }
+      context = tracer().startSpan(parentContext, request);
+      scope = context.makeCurrent();
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
@@ -113,16 +109,7 @@ public class GoogleHttpClientInstrumentationModule extends InstrumentationModule
       }
 
       scope.close();
-      if (throwable == null) {
+      tracer().endMaybeExceptionally(context, response, throwable);
-        tracer().end(context, response);
-      } else {
-        tracer().endExceptionally(context, response, throwable);
-      }
-      // If HttpRequest.setThrowExceptionOnExecuteError is set to false, there are no exceptions
-      // for a failed request.  Thus, check the response code
-      if (response != null && !response.isSuccessStatusCode()) {
-        spanFromContext(context).setStatus(StatusCode.ERROR);
-      }
     }
   }
 
@@ -138,15 +125,10 @@ public class GoogleHttpClientInstrumentationModule extends InstrumentationModule
         return;
       }
 
-      context = tracer().startSpan(parentContext, request, request.getHeaders());
+      context = tracer().startSpan(parentContext, request);
-
-      // propagating the context manually here so this instrumentation will work with and without
-      // the executors instrumentation
-      ContextStore<HttpRequest, Context> contextStore =
-          InstrumentationContext.get(HttpRequest.class, Context.class);
-      contextStore.put(request, context);
-
       scope = context.makeCurrent();
+
+      InstrumentationContext.get(HttpRequest.class, Context.class).put(request, context);
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)

instrumentation/google-http-client-1.19/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/googlehttpclient/GoogleHttpClientTracer.java

@@ -5,10 +5,13 @@
 
 package io.opentelemetry.javaagent.instrumentation.googlehttpclient;
 
+import static io.opentelemetry.javaagent.instrumentation.googlehttpclient.HeadersInjectAdapter.SETTER;
+
 import com.google.api.client.http.HttpHeaders;
 import com.google.api.client.http.HttpRequest;
 import com.google.api.client.http.HttpResponse;
-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 +24,10 @@ public class GoogleHttpClientTracer
     return TRACER;
   }
 
+  public Context startSpan(Context parentContext, HttpRequest request) {
+    return startSpan(parentContext, request, request.getHeaders());
+  }
+
   @Override
   protected String getInstrumentationName() {
     return "io.opentelemetry.javaagent.google-http-client";
@@ -56,8 +63,8 @@ public class GoogleHttpClientTracer
   }
 
   @Override
-  protected Setter<HttpHeaders> getSetter() {
+  protected TextMapPropagator.Setter<HttpHeaders> getSetter() {
-    return HeadersInjectAdapter.SETTER;
+    return SETTER;
   }
 
   private static String header(HttpHeaders headers, String name) {