From 0b6af9f2e962442a1fc479f491c64662b88f010c Mon Sep 17 00:00:00 2001 From: elbiocaetano Date: Thu, 25 Jan 2024 15:09:12 +0100 Subject: [PATCH] chore: setting low level instrumentation --- docs/supported-libraries.md | 3 +- .../apache-httpclient-5.2/library/README.md | 10 +- .../v5_2/OtelExecChainHandler.java | 117 ++---------------- .../v5_2/AbstractApacheHttpClient5Test.java | 20 ++- 4 files changed, 30 insertions(+), 120 deletions(-) diff --git a/docs/supported-libraries.md b/docs/supported-libraries.md index 327ac003d8f6..9fc03b92c250 100644 --- a/docs/supported-libraries.md +++ b/docs/supported-libraries.md @@ -29,8 +29,7 @@ These are the supported libraries and frameworks: | [Apache DBCP](https://commons.apache.org/proper/commons-dbcp/) | 2.0+ | [opentelemetry-apache-dbcp-2.0](../instrumentation/apache-dbcp-2.0/library) | [Database Pool Metrics] | | [Apache Dubbo](https://github.com/apache/dubbo/) | 2.7+ | [opentelemetry-apache-dubbo-2.7](../instrumentation/apache-dubbo-2.7/library-autoconfigure) | [RPC Client Spans], [RPC Server Spans] | | [Apache HttpAsyncClient](https://hc.apache.org/index.html) | 4.1+ | N/A | [HTTP Client Spans], [HTTP Client Metrics] | -| [Apache HttpClient](https://hc.apache.org/index.html) | 2.0+ | [opentelemetry-apache-httpclient-4.3](../instrumentation/apache-httpclient/apache-httpclient-4.3/library) | [HTTP Client Spans], [HTTP Client Metrics] | -| [Apache HttpClient 5](https://hc.apache.org/index.html) | 2.0+ | [opentelemetry-apache-httpclient-5.2](../instrumentation/apache-httpclient/apache-httpclient-5.2/library) | [HTTP Client Spans], [HTTP Client Metrics] | +| [Apache HttpClient](https://hc.apache.org/index.html) | 2.0+ | [opentelemetry-apache-httpclient-4.3](../instrumentation/apache-httpclient/apache-httpclient-4.3/library),
[opentelemetry-apache-httpclient-5.2](../instrumentation/apache-httpclient/apache-httpclient-5.2/library) | [HTTP Client Spans], [HTTP Client Metrics] | | [Apache Kafka Producer/Consumer API](https://kafka.apache.org/documentation/#producerapi) | 0.11+ | [opentelemetry-kafka-clients-2.6](../instrumentation/kafka/kafka-clients/kafka-clients-2.6/library) | [Messaging Spans] | | [Apache Kafka Streams API](https://kafka.apache.org/documentation/streams/) | 0.11+ | N/A | [Messaging Spans] | | [Apache MyFaces](https://myfaces.apache.org/) | 1.2+ (not including 3.x yet) | N/A | Provides `http.route` [2], Controller Spans [3] | diff --git a/instrumentation/apache-httpclient/apache-httpclient-5.2/library/README.md b/instrumentation/apache-httpclient/apache-httpclient-5.2/library/README.md index 7002686939e7..522c6c4d1f26 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-5.2/library/README.md +++ b/instrumentation/apache-httpclient/apache-httpclient-5.2/library/README.md @@ -29,9 +29,9 @@ implementation("io.opentelemetry.instrumentation:opentelemetry-apache-httpclient ### Usage -The instrumentation library provides a builder class `ApacheHttpClient5Telemetry` that wraps -an instance of the `HttpClientBuilder` to provide OpenTelemetry-based spans and context -propagation. +The instrumentation library provides the class `ApacheHttpClient5Telemetry` that has a builder +method and allows the creation of an instance of the `HttpClientBuilder` to provide +OpenTelemetry-based spans and context propagation: ```java import io.opentelemetry.api.OpenTelemetry; @@ -47,12 +47,12 @@ public class ApacheHttpClient5Configuration { this.openTelemetry = openTelemetry; } - // your configuration of the HttpClient goes here: + // creates a new http client builder for constructing http clients with open telemetry instrumentation protected HttpClientBuilder createBuilder() { return ApacheHttpClient5Telemetry.builder(openTelemetry).build().newHttpClientBuilder(); } - // creates a new httpClient with openTelemetry instrumentation + // creates a new http client with open telemetry instrumentation public HttpClient newHttpClient() { return ApacheHttpClient5Telemetry.builder(openTelemetry).build().newHttpClient(); } diff --git a/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/OtelExecChainHandler.java b/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/OtelExecChainHandler.java index 2b8d664ac54f..bf361ea646d1 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/OtelExecChainHandler.java +++ b/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/OtelExecChainHandler.java @@ -8,33 +8,21 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.ContextPropagators; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; -import jakarta.annotation.Nullable; +import io.opentelemetry.instrumentation.api.semconv.http.HttpClientRequestResendCount; import java.io.IOException; -import java.net.URI; -import org.apache.hc.client5.http.CircularRedirectException; -import org.apache.hc.client5.http.ClientProtocolException; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecChain.Scope; import org.apache.hc.client5.http.classic.ExecChainHandler; -import org.apache.hc.client5.http.impl.DefaultRedirectStrategy; -import org.apache.hc.client5.http.protocol.HttpClientContext; -import org.apache.hc.client5.http.protocol.RedirectLocations; import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpResponse; -import org.apache.hc.core5.http.ProtocolException; class OtelExecChainHandler implements ExecChainHandler { - private static final String REQUEST_CONTEXT_ATTRIBUTE_ID = + private static final String REQUEST_PARENT_CONTEXT_ATTRIBUTE_ID = OtelExecChainHandler.class.getName() + ".context"; - private static final String REQUEST_WRAPPER_ATTRIBUTE_ID = - OtelExecChainHandler.class.getName() + ".requestWrapper"; - private static final String REDIRECT_COUNT_ATTRIBUTE_ID = - OtelExecChainHandler.class.getName() + ".redirectCount"; private final Instrumenter instrumenter; private final ContextPropagators propagators; @@ -49,39 +37,29 @@ public OtelExecChainHandler( @Override public ClassicHttpResponse execute(ClassicHttpRequest request, Scope scope, ExecChain chain) throws IOException, HttpException { - Context context = scope.clientContext.getAttribute(REQUEST_CONTEXT_ATTRIBUTE_ID, Context.class); + Context parentContext = + scope.clientContext.getAttribute(REQUEST_PARENT_CONTEXT_ATTRIBUTE_ID, Context.class); request.setVersion(scope.clientContext.getProtocolVersion()); - if (context != null) { - ApacheHttpClient5Request instrumenterRequest = - scope.clientContext.getAttribute( - REQUEST_WRAPPER_ATTRIBUTE_ID, ApacheHttpClient5Request.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(request, instrumenterRequest, scope.clientContext, chain, scope, context); + if (parentContext == null) { + parentContext = HttpClientRequestResendCount.initialize(Context.current()); + scope.clientContext.setAttribute(REQUEST_PARENT_CONTEXT_ATTRIBUTE_ID, parentContext); } ApacheHttpClient5Request instrumenterRequest = getApacheHttpClient5Request(request, scope); - Context parentContext = Context.current(); if (!instrumenter.shouldStart(parentContext, instrumenterRequest)) { return chain.proceed(request, scope); } - context = instrumenter.start(parentContext, instrumenterRequest); - scope.clientContext.setAttribute(REQUEST_CONTEXT_ATTRIBUTE_ID, context); - scope.clientContext.setAttribute(REQUEST_WRAPPER_ATTRIBUTE_ID, instrumenterRequest); - scope.clientContext.setAttribute(REDIRECT_COUNT_ATTRIBUTE_ID, 0); - + Context context = instrumenter.start(parentContext, instrumenterRequest); propagators.getTextMapPropagator().inject(context, request, HttpHeaderSetter.INSTANCE); - return execute(request, instrumenterRequest, scope.clientContext, chain, scope, context); + return execute(request, instrumenterRequest, chain, scope, context); } private ClassicHttpResponse execute( ClassicHttpRequest request, ApacheHttpClient5Request instrumenterRequest, - HttpClientContext httpContext, ExecChain chain, Scope scope, Context context) @@ -95,83 +73,8 @@ private ClassicHttpResponse 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, - HttpRequest request, - ApacheHttpClient5Request instrumenterRequest, - @Nullable ClassicHttpResponse response) - throws HttpException { - if (response == null) { - return false; - } - if (!httpContext.getRequestConfig().isRedirectsEnabled()) { - return false; - } - - // 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.getAll().isEmpty()) { - RedirectLocations copy = new RedirectLocations(); - for (URI uri : redirectLocations.getAll()) { - copy.add(uri); - } - - try { - getLocationUri(request, response, httpContext, copy); - } catch (ProtocolException e) { - // We will not be returning to the Exec, finish the span. - instrumenter.end( - context, instrumenterRequest, response, new ClientProtocolException(e.getMessage(), 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; - } - - private static URI getLocationUri( - HttpRequest request, - HttpResponse response, - HttpClientContext httpContext, - RedirectLocations redirectLocations) - throws HttpException { - URI redirectUri = - DefaultRedirectStrategy.INSTANCE.getLocationURI(request, response, httpContext); - if (!httpContext.getRequestConfig().isCircularRedirectsAllowed() - && redirectLocations.contains(redirectUri)) { - throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'"); + instrumenter.end(context, instrumenterRequest, response, error); } - return DefaultRedirectStrategy.INSTANCE.getLocationURI(request, response, httpContext); } private static ApacheHttpClient5Request getApacheHttpClient5Request( diff --git a/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/test/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/AbstractApacheHttpClient5Test.java b/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/test/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/AbstractApacheHttpClient5Test.java index 5ec2c9ef1bc6..b23b5f32d247 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/test/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/AbstractApacheHttpClient5Test.java +++ b/instrumentation/apache-httpclient/apache-httpclient-5.2/library/src/test/java/io/opentelemetry/instrumentation/apachehttpclient/v5_2/AbstractApacheHttpClient5Test.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 BasicClassicHttpRequest buildRequest( @@ -95,7 +103,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientHostRequestContextTest extends AbstractHttpClientTest { + class ApacheClientHostRequestContextTest extends ApacheHttpClientTest { @Override public BasicClassicHttpRequest buildRequest( @@ -138,7 +146,7 @@ public void sendRequestWithCallback( @Nested class ApacheClientHostAbsoluteUriRequestTest - extends AbstractHttpClientTest { + extends ApacheHttpClientTest { @Override public BasicClassicHttpRequest buildRequest( @@ -178,7 +186,7 @@ public void sendRequestWithCallback( @Nested class ApacheClientHostAbsoluteUriRequestContextTest - extends AbstractHttpClientTest { + extends ApacheHttpClientTest { @Override public BasicClassicHttpRequest buildRequest( @@ -219,7 +227,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientUriRequestTest extends AbstractHttpClientTest { + class ApacheClientUriRequestTest extends ApacheHttpClientTest { @Override public BasicClassicHttpRequest buildRequest( @@ -251,7 +259,7 @@ public void sendRequestWithCallback( } @Nested - class ApacheClientUriRequestContextTest extends AbstractHttpClientTest { + class ApacheClientUriRequestContextTest extends ApacheHttpClientTest { @Override public HttpUriRequest buildRequest(String method, URI uri, Map headers) {