From 8da3d82097e16a13d1c80dfacbc7ee7e13d16b35 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 17 Jan 2024 12:10:31 +0200 Subject: [PATCH] Apache http client 4.3 low level instrumentation --- .../v4_3/TracingProtocolExec.java | 91 ++----------------- .../v4_3/AbstractApacheHttpClientTest.java | 20 ++-- 2 files changed, 24 insertions(+), 87 deletions(-) diff --git a/instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java b/instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java index 3a59039150d5..1f29cf4fa758 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java +++ b/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 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; } } diff --git a/instrumentation/apache-httpclient/apache-httpclient-4.3/testing/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/AbstractApacheHttpClientTest.java b/instrumentation/apache-httpclient/apache-httpclient-4.3/testing/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/AbstractApacheHttpClientTest.java index 92cdfa33181b..5d7d198d7193 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-4.3/testing/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/AbstractApacheHttpClientTest.java +++ b/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 extends AbstractHttpClientTest { + @Override + protected void configure(HttpClientTestOptions.Builder optionsBuilder) { + optionsBuilder.markAsLowLevelInstrumentation(); + } + } + @Nested - class ApacheClientHostRequestTest extends AbstractHttpClientTest { + class ApacheClientHostRequestTest extends ApacheHttpClientTest { @Override public BasicHttpRequest buildRequest(String method, URI uri, Map headers) { @@ -92,7 +100,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientHostRequestContextTest extends AbstractHttpClientTest { + class ApacheClientHostRequestContextTest extends ApacheHttpClientTest { @Override public BasicHttpRequest buildRequest(String method, URI uri, Map headers) { @@ -133,7 +141,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientHostAbsoluteUriRequestTest extends AbstractHttpClientTest { + class ApacheClientHostAbsoluteUriRequestTest extends ApacheHttpClientTest { @Override public BasicHttpRequest buildRequest(String method, URI uri, Map headers) { @@ -170,7 +178,7 @@ public void sendRequestWithCallback( @Nested class ApacheClientHostAbsoluteUriRequestContextTest - extends AbstractHttpClientTest { + extends ApacheHttpClientTest { @Override public BasicHttpRequest buildRequest(String method, URI uri, Map headers) { @@ -210,7 +218,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientUriRequestTest extends AbstractHttpClientTest { + class ApacheClientUriRequestTest extends ApacheHttpClientTest { @Override public HttpUriRequest buildRequest(String method, URI uri, Map headers) { @@ -241,7 +249,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientUriRequestContextTest extends AbstractHttpClientTest { + class ApacheClientUriRequestContextTest extends ApacheHttpClientTest { @Override public HttpUriRequest buildRequest(String method, URI uri, Map headers) {