HTTP client instrumentation cleanup: http-url-connection (#1908)

* HttpClientTracer cleanup: http-url-connection

* Checkstyle

* Feedback

instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentationModule.java

@@ -20,6 +20,7 @@ import static net.bytebuddy.matcher.ElementMatchers.not;
 import com.google.auto.service.AutoService;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
+import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
 import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap;
 import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
 import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
@@ -38,7 +39,7 @@ import net.bytebuddy.matcher.ElementMatchers;
 public class HttpUrlConnectionInstrumentationModule extends InstrumentationModule {
 
   public HttpUrlConnectionInstrumentationModule() {
-    super("httpurlconnection");
+    super("http-url-connection");
   }
 
   @Override
@@ -79,75 +80,82 @@ public class HttpUrlConnectionInstrumentationModule extends InstrumentationModul
   public static class HttpUrlConnectionAdvice {
 
     @Advice.OnMethodEnter(suppress = Throwable.class)
-    public static HttpUrlState methodEnter(
+    public static void methodEnter(
         @Advice.This HttpURLConnection connection,
         @Advice.FieldValue("connected") boolean connected,
-        @Advice.Local("otelScope") Scope scope) {
+        @Advice.Local("otelHttpUrlState") HttpUrlState httpUrlState,
+        @Advice.Local("otelScope") Scope scope,
+        @Advice.Local("otelCallDepth") CallDepth callDepth) {
 
-      int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpURLConnection.class);
+      callDepth = CallDepthThreadLocalMap.getCallDepth(HttpURLConnection.class);
-      if (callDepth > 0) {
+      if (callDepth.getAndIncrement() > 0) {
-        return null;
+        // only want the rest of the instrumentation rules (which are complex enough) to apply to
+        // top-level HttpURLConnection calls
+        return;
       }
-
-      ContextStore<HttpURLConnection, HttpUrlState> contextStore =
-          InstrumentationContext.get(HttpURLConnection.class, HttpUrlState.class);
-      HttpUrlState state = contextStore.putIfAbsent(connection, HttpUrlState::new);
-
-      synchronized (state) {
-        if (!state.initialized) {
       Context parentContext = currentContext();
-          if (tracer().shouldStartSpan(parentContext)) {
+      if (!tracer().shouldStartSpan(parentContext)) {
-            state.context = tracer().startSpan(parentContext, connection, connection);
+        return;
-            if (!connected) {
-              scope = state.context.makeCurrent();
       }
+
+      // using storage for a couple of reasons:
+      // - to start an operation in connect() and end it in getInputStream()
+      // - to avoid creating a new operation on multiple subsequent calls to getInputStream()
+      ContextStore<HttpURLConnection, HttpUrlState> storage =
+          InstrumentationContext.get(HttpURLConnection.class, HttpUrlState.class);
+      httpUrlState = storage.get(connection);
+
+      if (httpUrlState != null) {
+        if (!httpUrlState.finished) {
+          scope = httpUrlState.context.makeCurrent();
         }
-          state.initialized = true;
+        return;
       }
-      }
+
-      return state;
+      Context context = tracer().startSpan(parentContext, connection);
+      httpUrlState = new HttpUrlState(context);
+      storage.put(connection, httpUrlState);
+      scope = context.makeCurrent();
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void methodExit(
-        @Advice.Enter HttpUrlState state,
         @Advice.This HttpURLConnection connection,
         @Advice.FieldValue("responseCode") int responseCode,
         @Advice.Thrown Throwable throwable,
         @Advice.Origin("#m") String methodName,
-        @Advice.Local("otelScope") Scope scope) {
+        @Advice.Local("otelHttpUrlState") HttpUrlState httpUrlState,
-
+        @Advice.Local("otelScope") Scope scope,
-      if (scope != null) {
+        @Advice.Local("otelCallDepth") CallDepth callDepth) {
-        scope.close();
+      if (callDepth.decrementAndGet() > 0) {
-      }
-      if (state == null) {
         return;
       }
-      CallDepthThreadLocalMap.reset(HttpURLConnection.class);
+      if (scope == null) {
+        return;
+      }
+      scope.close();
 
-      synchronized (state) {
-        if (state.context != null && !state.finished) {
       if (throwable != null) {
-            tracer().endExceptionally(state.context, throwable);
+        tracer().endExceptionally(httpUrlState.context, throwable);
-            state.finished = true;
+        httpUrlState.finished = true;
-          } else if ("getInputStream".equals(methodName)) {
+      } else if (methodName.equals("getInputStream") && responseCode > 0) {
         // responseCode field is sometimes not populated.
         // We can't call getResponseCode() due to some unwanted side-effects
         // (e.g. breaks getOutputStream).
-            if (responseCode > 0) {
+        tracer().end(httpUrlState.context, new HttpUrlResponse(connection, responseCode));
-              tracer().end(state.context, new HttpUrlResponse(connection, responseCode));
+        httpUrlState.finished = true;
-              state.finished = true;
-            }
-          }
-        }
       }
     }
   }
 
-  // state is always accessed under synchronized block
+  // everything is public since called directly from advice code
+  // (which is inlined into other packages)
   public static class HttpUrlState {
-    public boolean initialized;
+    public final Context context;
-    public Context context;
     public boolean finished;
+
+    public HttpUrlState(Context context) {
+      this.context = context;
+    }
   }
 }

instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionTracer.java

@@ -7,7 +7,8 @@ package io.opentelemetry.javaagent.instrumentation.httpurlconnection;
 
 import static io.opentelemetry.javaagent.instrumentation.httpurlconnection.HeadersInjectAdapter.SETTER;
 
-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.HttpURLConnection;
 import java.net.URI;
@@ -22,6 +23,10 @@ public class HttpUrlConnectionTracer
     return TRACER;
   }
 
+  public Context startSpan(Context parentContext, HttpURLConnection request) {
+    return super.startSpan(parentContext, request, request);
+  }
+
   @Override
   protected String method(HttpURLConnection connection) {
     return connection.getRequestMethod();
@@ -48,7 +53,7 @@ public class HttpUrlConnectionTracer
   }
 
   @Override
-  protected Setter<HttpURLConnection> getSetter() {
+  protected TextMapPropagator.Setter<HttpURLConnection> getSetter() {
     return SETTER;
   }