From e2264222ce22a5b9314919849c041f0ea852eee9 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Mon, 14 Dec 2020 16:34:06 -0800 Subject: [PATCH] HTTP client instrumentation cleanup, part 0 (#1893) --- .../api/tracer/BaseTracer.java | 10 +++++++ .../api/tracer/HttpClientTracer.java | 28 ++++++++++++++----- .../ApacheHttpAsyncClientTracer.java | 4 +-- .../v4_0/ApacheHttpClientTracer.java | 4 +-- .../awssdk/v1_11/AwsSdkClientTracer.java | 4 +-- .../awssdk/v2_2/AwsSdkHttpClientTracer.java | 4 +-- .../httpclient/JdkHttpClientTracer.java | 6 ++-- .../instrumentation/khttp/KHttpAdvice.java | 6 +--- .../KubernetesClientTracer.java | 4 +-- 9 files changed, 44 insertions(+), 26 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java index 09abfd95b1..c4afa2e7ac 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java @@ -67,6 +67,16 @@ public abstract class BaseTracer { return tracer.spanBuilder(spanName).setSpanKind(kind).startSpan(); } + // TODO (trask) make the key private + protected final boolean inClientSpan(Context parentContext) { + return parentContext.get(CONTEXT_CLIENT_SPAN_KEY) != null; + } + + // TODO (trask) make the key private + protected final Context withClientSpan(Context parentContext, Span span) { + return parentContext.with(span).with(CONTEXT_CLIENT_SPAN_KEY, span); + } + public Scope startScope(Span span) { return Context.current().with(span).makeCurrent(); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java index 204f60a656..e33953f6fd 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java @@ -60,7 +60,7 @@ public abstract class HttpClientTracer extends BaseT } public boolean shouldStartSpan(Context parentContext) { - return parentContext.get(CONTEXT_CLIENT_SPAN_KEY) == null; + return !inClientSpan(parentContext); } public Context startSpan(Context parentContext, REQUEST request, CARRIER carrier) { @@ -71,14 +71,18 @@ public abstract class HttpClientTracer extends BaseT Context parentContext, REQUEST request, CARRIER carrier, long startTimeNanos) { Span span = internalStartSpan(parentContext, request, spanNameForRequest(request), startTimeNanos); + Context context = withClientSpan(parentContext, span); + inject(context, carrier); + return context; + } + + private void inject(Context context, CARRIER carrier) { Setter setter = getSetter(); if (setter == null) { throw new IllegalStateException( "getSetter() not defined but calling startScope(), either getSetter must be implemented or the scope should be setup manually"); } - Context context = parentContext.with(span).with(CONTEXT_CLIENT_SPAN_KEY, span); OpenTelemetry.getGlobalPropagators().getTextMapPropagator().inject(context, carrier, setter); - return context; } public void end(Context context, RESPONSE response) { @@ -112,6 +116,18 @@ public abstract class HttpClientTracer extends BaseT super.endExceptionally(span, throwable, -1); } + // TODO (trask) see if we can reduce the number of end..() variants + // see https://github.com/open-telemetry/opentelemetry-java-instrumentation + // /pull/1893#discussion_r542111699 + public void endMaybeExceptionally( + Context context, RESPONSE response, @Nullable Throwable throwable) { + if (throwable != null) { + endExceptionally(context, throwable); + } else { + end(context, response); + } + } + private Span internalStartSpan( Context parentContext, REQUEST request, String name, long startTimeNanos) { SpanBuilder spanBuilder = @@ -124,7 +140,7 @@ public abstract class HttpClientTracer extends BaseT return span; } - protected Span onRequest(Span span, REQUEST request) { + protected void onRequest(Span span, REQUEST request) { assert span != null; if (request != null) { span.setAttribute(SemanticAttributes.NET_TRANSPORT, "IP.TCP"); @@ -134,7 +150,6 @@ public abstract class HttpClientTracer extends BaseT setFlavor(span, request); setUrl(span, request); } - return span; } private void setFlavor(Span span, REQUEST request) { @@ -163,7 +178,7 @@ public abstract class HttpClientTracer extends BaseT } } - protected Span onResponse(Span span, RESPONSE response) { + protected void onResponse(Span span, RESPONSE response) { assert span != null; if (response != null) { Integer status = status(response); @@ -172,7 +187,6 @@ public abstract class HttpClientTracer extends BaseT span.setStatus(HttpStatusConverter.statusFromHttpStatus(status)); } } - return span; } protected String spanNameForRequest(REQUEST request) { diff --git a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java index 131d1a8092..6de3fd9bdf 100644 --- a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java +++ b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTracer.java @@ -110,7 +110,7 @@ public class ApacheHttpAsyncClientTracer /** This method is overridden to allow other classes in this package to call it. */ @Override - public Span onRequest(Span span, HttpRequest httpRequest) { - return super.onRequest(span, httpRequest); + protected void onRequest(Span span, HttpRequest httpRequest) { + super.onRequest(span, httpRequest); } } diff --git a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientTracer.java b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientTracer.java index 0253eed1cd..4471ea1215 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientTracer.java +++ b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientTracer.java @@ -73,7 +73,7 @@ public class ApacheHttpClientTracer /** This method is overridden to allow other classes in this package to call it. */ @Override - protected Span onResponse(Span span, HttpResponse httpResponse) { - return super.onResponse(span, httpResponse); + protected void onResponse(Span span, HttpResponse httpResponse) { + super.onResponse(span, httpResponse); } } diff --git a/instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/AwsSdkClientTracer.java b/instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/AwsSdkClientTracer.java index 63b123334b..ee157654e9 100644 --- a/instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/AwsSdkClientTracer.java +++ b/instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/AwsSdkClientTracer.java @@ -64,12 +64,12 @@ public class AwsSdkClientTracer extends HttpClientTracer, Request, } @Override - public Span onResponse(Span span, Response response) { + public void onResponse(Span span, Response response) { if (response != null && response.getAwsResponse() instanceof AmazonWebServiceResponse) { AmazonWebServiceResponse awsResp = (AmazonWebServiceResponse) response.getAwsResponse(); span.setAttribute("aws.requestId", awsResp.getRequestId()); } - return super.onResponse(span, response); + super.onResponse(span, response); } private String qualifiedOperation(String service, Class operation) { diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AwsSdkHttpClientTracer.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AwsSdkHttpClientTracer.java index 98ae7f6e75..bb760662d8 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AwsSdkHttpClientTracer.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AwsSdkHttpClientTracer.java @@ -74,8 +74,8 @@ final class AwsSdkHttpClientTracer /** This method is overridden to allow other classes in this package to call it. */ @Override - protected Span onRequest(Span span, SdkHttpRequest sdkHttpRequest) { - return super.onRequest(span, sdkHttpRequest); + protected void onRequest(Span span, SdkHttpRequest sdkHttpRequest) { + super.onRequest(span, sdkHttpRequest); } public Context startSpan(Context parentContext, String name, Tracer tracer, Kind kind) { diff --git a/instrumentation/java-httpclient/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java b/instrumentation/java-httpclient/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java index 46c667f534..1ba032374a 100644 --- a/instrumentation/java-httpclient/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java +++ b/instrumentation/java-httpclient/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/JdkHttpClientTracer.java @@ -61,16 +61,14 @@ public class JdkHttpClientTracer } @Override - protected Span onResponse(Span span, HttpResponse httpResponse) { - span = super.onResponse(span, httpResponse); + protected void onResponse(Span span, HttpResponse httpResponse) { + super.onResponse(span, httpResponse); if (httpResponse != null) { span.setAttribute( SemanticAttributes.HTTP_FLAVOR, httpResponse.version() == Version.HTTP_1_1 ? "1.1" : "2.0"); } - - return span; } @Override diff --git a/instrumentation/khttp-0.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/khttp/KHttpAdvice.java b/instrumentation/khttp-0.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/khttp/KHttpAdvice.java index be01577594..30a062c63f 100644 --- a/instrumentation/khttp-0.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/khttp/KHttpAdvice.java +++ b/instrumentation/khttp-0.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/khttp/KHttpAdvice.java @@ -45,10 +45,6 @@ public class KHttpAdvice { } scope.close(); - if (throwable == null) { - tracer().end(context, response); - } else { - tracer().endExceptionally(context, response, throwable); - } + tracer().endMaybeExceptionally(context, response, throwable); } } diff --git a/instrumentation/kubernetes-client-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java b/instrumentation/kubernetes-client-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java index 316bf347dc..84008a5311 100644 --- a/instrumentation/kubernetes-client-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java +++ b/instrumentation/kubernetes-client-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java @@ -82,7 +82,7 @@ public class KubernetesClientTracer extends HttpClientTracer