Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apache http client 4.3 low level instrumentation #10253

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Apache http client 4.3 low level instrumentation #10253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 10 additions & 81 deletions ...main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java
View file
Open in desktop
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
Open in desktop
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
Loading