Apache http client 4.3 low level instrumentation (#10253)

Author: laurit

instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java

@@ -9,30 +9,22 @@
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.context.propagation.ContextPropagators;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.semconv.http.HttpClientRequestResendCount;
 import java.io.IOException;
-import javax.annotation.Nullable;
 import org.apache.http.HttpException;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpResponse;
-import org.apache.http.ProtocolException;
-import org.apache.http.client.ClientProtocolException;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpExecutionAware;
 import org.apache.http.client.methods.HttpRequestWrapper;
 import org.apache.http.client.protocol.HttpClientContext;
 import org.apache.http.conn.routing.HttpRoute;
-import org.apache.http.impl.client.DefaultRedirectStrategy;
-import org.apache.http.impl.client.RedirectLocations;
 import org.apache.http.impl.execchain.ClientExecChain;
 
 final class TracingProtocolExec implements ClientExecChain {
 
-  private static final String REQUEST_CONTEXT_ATTRIBUTE_ID =
+  private static final String REQUEST_PARENT_CONTEXT_ATTRIBUTE_ID =
       TracingProtocolExec.class.getName() + ".context";
-  private static final String REQUEST_WRAPPER_ATTRIBUTE_ID =
-      TracingProtocolExec.class.getName() + ".requestWrapper";
-  private static final String REDIRECT_COUNT_ATTRIBUTE_ID =
-      TracingProtocolExec.class.getName() + ".redirectCount";
 
   private final Instrumenter<ApacheHttpClientRequest, HttpResponse> instrumenter;
   private final ContextPropagators propagators;
@@ -54,14 +46,12 @@ public CloseableHttpResponse execute(
       HttpClientContext httpContext,
       HttpExecutionAware httpExecutionAware)
       throws IOException, HttpException {
-    Context context = httpContext.getAttribute(REQUEST_CONTEXT_ATTRIBUTE_ID, Context.class);
-    if (context != null) {
-      ApacheHttpClientRequest instrumenterRequest =
-          httpContext.getAttribute(REQUEST_WRAPPER_ATTRIBUTE_ID, ApacheHttpClientRequest.class);
-      // Request already had a context so a redirect. Don't create a new span just inject and
-      // execute.
-      propagators.getTextMapPropagator().inject(context, request, HttpHeaderSetter.INSTANCE);
-      return execute(route, request, instrumenterRequest, httpContext, httpExecutionAware, context);
+
+    Context parentContext =
+        httpContext.getAttribute(REQUEST_PARENT_CONTEXT_ATTRIBUTE_ID, Context.class);
+    if (parentContext == null) {
+      parentContext = HttpClientRequestResendCount.initialize(Context.current());
+      httpContext.setAttribute(REQUEST_PARENT_CONTEXT_ATTRIBUTE_ID, parentContext);
     }
 
     HttpHost host = null;
@@ -81,16 +71,11 @@ public CloseableHttpResponse execute(
     }
     ApacheHttpClientRequest instrumenterRequest = new ApacheHttpClientRequest(host, request);
 
-    Context parentContext = Context.current();
     if (!instrumenter.shouldStart(parentContext, instrumenterRequest)) {
       return exec.execute(route, request, httpContext, httpExecutionAware);
     }
 
-    context = instrumenter.start(parentContext, instrumenterRequest);
-    httpContext.setAttribute(REQUEST_CONTEXT_ATTRIBUTE_ID, context);
-    httpContext.setAttribute(REQUEST_WRAPPER_ATTRIBUTE_ID, instrumenterRequest);
-    httpContext.setAttribute(REDIRECT_COUNT_ATTRIBUTE_ID, 0);
-
+    Context context = instrumenter.start(parentContext, instrumenterRequest);
     propagators.getTextMapPropagator().inject(context, request, HttpHeaderSetter.INSTANCE);
 
     return execute(route, request, instrumenterRequest, httpContext, httpExecutionAware, context);
@@ -113,63 +98,7 @@ private CloseableHttpResponse execute(
       error = e;
       throw e;
     } finally {
-      if (!pendingRedirect(context, httpContext, request, instrumenterRequest, response)) {
-        instrumenter.end(context, instrumenterRequest, response, error);
-      }
-    }
-  }
-
-  private boolean pendingRedirect(
-      Context context,
-      HttpClientContext httpContext,
-      HttpRequestWrapper request,
-      ApacheHttpClientRequest instrumenterRequest,
-      @Nullable CloseableHttpResponse response) {
-    if (response == null) {
-      return false;
-    }
-    if (!httpContext.getRequestConfig().isRedirectsEnabled()) {
-      return false;
+      instrumenter.end(context, instrumenterRequest, response, error);
     }
-
-    // TODO(anuraaga): Support redirect strategies other than the default. There is no way to get
-    // the user defined redirect strategy without some tricks, but it's very rare to override
-    // the strategy, usually it is either on or off as checked above. We can add support for this
-    // later if needed.
-    try {
-      if (!DefaultRedirectStrategy.INSTANCE.isRedirected(request, response, httpContext)) {
-        return false;
-      }
-    } catch (ProtocolException e) {
-      // DefaultRedirectStrategy.isRedirected cannot throw this so just return a default.
-      return false;
-    }
-
-    // Very hacky and a bit slow, but the only way to determine whether the client will fail with
-    // a circular redirect, which happens before exec decorators run.
-    RedirectLocations redirectLocations =
-        (RedirectLocations) httpContext.getAttribute(HttpClientContext.REDIRECT_LOCATIONS);
-    if (redirectLocations != null) {
-      RedirectLocations copy = new RedirectLocations();
-      copy.addAll(redirectLocations);
-
-      try {
-        DefaultRedirectStrategy.INSTANCE.getLocationURI(request, response, httpContext);
-      } catch (ProtocolException e) {
-        // We will not be returning to the Exec, finish the span.
-        instrumenter.end(context, instrumenterRequest, response, new ClientProtocolException(e));
-        return true;
-      } finally {
-        httpContext.setAttribute(HttpClientContext.REDIRECT_LOCATIONS, copy);
-      }
-    }
-
-    int redirectCount = httpContext.getAttribute(REDIRECT_COUNT_ATTRIBUTE_ID, Integer.class);
-    if (++redirectCount > httpContext.getRequestConfig().getMaxRedirects()) {
-      return false;
-    }
-
-    httpContext.setAttribute(REDIRECT_COUNT_ATTRIBUTE_ID, redirectCount);
-    return true;
   }
 }

instrumentation/apache-httpclient/apache-httpclient-4.3/testing/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/AbstractApacheHttpClientTest.java

@@ -8,6 +8,7 @@
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult;
+import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.net.URI;
@@ -54,8 +55,15 @@ CloseableHttpClient getClient(URI uri) {
     return client;
   }
 
+  abstract static class ApacheHttpClientTest<T> extends AbstractHttpClientTest<T> {
+    @Override
+    protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
+      optionsBuilder.markAsLowLevelInstrumentation();
+    }
+  }
+
   @Nested
-  class ApacheClientHostRequestTest extends AbstractHttpClientTest<BasicHttpRequest> {
+  class ApacheClientHostRequestTest extends ApacheHttpClientTest<BasicHttpRequest> {
 
     @Override
     public BasicHttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -92,7 +100,7 @@ public void sendRequestWithCallback(
   }
 
   @Nested
-  class ApacheClientHostRequestContextTest extends AbstractHttpClientTest<BasicHttpRequest> {
+  class ApacheClientHostRequestContextTest extends ApacheHttpClientTest<BasicHttpRequest> {
 
     @Override
     public BasicHttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -133,7 +141,7 @@ public void sendRequestWithCallback(
   }
 
   @Nested
-  class ApacheClientHostAbsoluteUriRequestTest extends AbstractHttpClientTest<BasicHttpRequest> {
+  class ApacheClientHostAbsoluteUriRequestTest extends ApacheHttpClientTest<BasicHttpRequest> {
 
     @Override
     public BasicHttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -170,7 +178,7 @@ public void sendRequestWithCallback(
 
   @Nested
   class ApacheClientHostAbsoluteUriRequestContextTest
-      extends AbstractHttpClientTest<BasicHttpRequest> {
+      extends ApacheHttpClientTest<BasicHttpRequest> {
 
     @Override
     public BasicHttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -210,7 +218,7 @@ public void sendRequestWithCallback(
   }
 
   @Nested
-  class ApacheClientUriRequestTest extends AbstractHttpClientTest<HttpUriRequest> {
+  class ApacheClientUriRequestTest extends ApacheHttpClientTest<HttpUriRequest> {
 
     @Override
     public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
@@ -241,7 +249,7 @@ public void sendRequestWithCallback(
   }
 
   @Nested
-  class ApacheClientUriRequestContextTest extends AbstractHttpClientTest<HttpUriRequest> {
+  class ApacheClientUriRequestContextTest extends ApacheHttpClientTest<HttpUriRequest> {
 
     @Override
     public HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {