Skip to content

Commit

Permalink
Apache http client 4.3 low level instrumentation (open-telemetry#10253)
Browse files Browse the repository at this point in the history
  • Loading branch information
@laurit @steverao
laurit authored and steverao committed Feb 16, 2024
1 parent 43a0852 commit 55b6bf3
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 87 deletions.
91 changes: 10 additions & 81 deletions ...main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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;
}
}
20 changes: 14 additions & 6 deletions .../io/opentelemetry/instrumentation/apachehttpclient/v4_3/AbstractApacheHttpClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 55b6bf3

Please sign in to comment.