From 82c064d95147a73c78e19c07f8d3f4b2bda4bf2d Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 30 Jan 2025 15:46:30 +0200 Subject: [PATCH 1/2] Fix scope leak in aws sdk instrumentation --- .../internal/TracingExecutionInterceptor.java | 21 +++++++++++------ .../awssdk/v2_2/AbstractAws2ClientTest.java | 23 +++++++++++++++++++ 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java index 94243d0b11bb..1a880276a1e6 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java @@ -192,13 +192,6 @@ public SdkRequest modifyRequest( executionAttributes.putAttribute(PARENT_CONTEXT_ATTRIBUTE, parentOtelContext); executionAttributes.putAttribute(CONTEXT_ATTRIBUTE, otelContext); executionAttributes.putAttribute(REQUEST_FINISHER_ATTRIBUTE, requestFinisher); - if (executionAttributes - .getAttribute(SdkExecutionAttribute.CLIENT_TYPE) - .equals(ClientType.SYNC)) { - // We can only activate context for synchronous clients, which allows downstream - // instrumentation like Apache to know about the SDK span. - executionAttributes.putAttribute(SCOPE_ATTRIBUTE, otelContext.makeCurrent()); - } Span span = Span.fromContext(otelContext); @@ -233,6 +226,20 @@ public SdkRequest modifyRequest( return request; } + @Override + public void afterMarshalling( + Context.AfterMarshalling context, ExecutionAttributes executionAttributes) { + io.opentelemetry.context.Context otelContext = getContext(executionAttributes); + if (otelContext != null + && executionAttributes + .getAttribute(SdkExecutionAttribute.CLIENT_TYPE) + .equals(ClientType.SYNC)) { + // We can only activate context for synchronous clients, which allows downstream + // instrumentation like Apache to know about the SDK span. + executionAttributes.putAttribute(SCOPE_ATTRIBUTE, otelContext.makeCurrent()); + } + } + @Override public void beforeTransmission( Context.BeforeTransmission context, ExecutionAttributes executionAttributes) { diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.java b/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.java index 12b0f79bc7bc..79e94fb70c87 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.java @@ -24,9 +24,11 @@ import static io.opentelemetry.semconv.incubating.RpcIncubatingAttributes.RPC_SYSTEM; import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.catchThrowable; import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.context.Context; import io.opentelemetry.sdk.testing.assertj.AttributeAssertion; import io.opentelemetry.sdk.trace.data.StatusData; import io.opentelemetry.testing.internal.armeria.common.HttpData; @@ -56,6 +58,7 @@ import software.amazon.awssdk.core.ResponseInputStream; import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.exception.SdkClientException; +import software.amazon.awssdk.core.exception.SdkException; import software.amazon.awssdk.core.retry.RetryPolicy; import software.amazon.awssdk.http.apache.ApacheHttpClient; import software.amazon.awssdk.regions.Region; @@ -696,4 +699,24 @@ void testTimeoutAndRetryErrorsAreNotCaptured() { equalTo(stringKey("aws.agent"), "java-aws-sdk"), equalTo(stringKey("aws.bucket.name"), "somebucket")))); } + + // regression test for + // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/13124 + // verify that scope is not leaked on exception + @Test + void testS3ListNullBucket() { + S3ClientBuilder builder = S3Client.builder(); + configureSdkClient(builder); + S3Client client = + builder + .endpointOverride(clientUri) + .region(Region.AP_NORTHEAST_1) + .credentialsProvider(CREDENTIALS_PROVIDER) + .build(); + + assertThatThrownBy(() -> client.listObjectsV2(b -> b.bucket(null))) + .isInstanceOf(SdkException.class); + + assertThat(Context.current()).isEqualTo(Context.root()); + } } From e2b960fd27871c03a0458d4670c9f80b8f9e9eb9 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 30 Jan 2025 16:31:13 +0200 Subject: [PATCH 2/2] fix duplicate interceptor test --- .../v2_2/internal/TracingExecutionInterceptor.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java index 1a880276a1e6..40074c85b12d 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/TracingExecutionInterceptor.java @@ -142,6 +142,11 @@ public SdkRequest modifyRequest( io.opentelemetry.context.Context parentOtelContext = io.opentelemetry.context.Context.current(); SdkRequest request = context.request(); + // the request has already been modified, duplicate interceptor? + if (executionAttributes.getAttribute(SDK_REQUEST_ATTRIBUTE) != null) { + return request; + } + // Ignore presign request. These requests don't run all interceptor methods and the span created // here would never be ended and scope closed. if (executionAttributes.getAttribute(AwsSignerExecutionAttribute.PRESIGNER_EXPIRATION) @@ -229,6 +234,11 @@ public SdkRequest modifyRequest( @Override public void afterMarshalling( Context.AfterMarshalling context, ExecutionAttributes executionAttributes) { + // the request has already been modified, duplicate interceptor? + if (executionAttributes.getAttribute(SCOPE_ATTRIBUTE) != null) { + return; + } + io.opentelemetry.context.Context otelContext = getContext(executionAttributes); if (otelContext != null && executionAttributes