From e65366b860cd8487c2b4bfbc5246c37b836264ea Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 30 Jan 2025 23:42:48 +0200 Subject: [PATCH] Fix scope leak in aws sdk instrumentation (#13129) --- .../internal/TracingExecutionInterceptor.java | 31 ++++++++++++++----- .../awssdk/v2_2/AbstractAws2ClientTest.java | 23 ++++++++++++++ 2 files changed, 47 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 94243d0b11..40074c85b1 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 final class TracingExecutionInterceptor implements ExecutionInterceptor { 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) @@ -192,13 +197,6 @@ public final class TracingExecutionInterceptor implements ExecutionInterceptor { 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 +231,25 @@ public final class TracingExecutionInterceptor implements ExecutionInterceptor { return request; } + @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 + .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 12b0f79bc7..79e94fb70c 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_SE 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 org.junit.jupiter.params.provider.MethodSource; 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 @@ public abstract class AbstractAws2ClientTest extends AbstractAws2ClientCoreTest 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()); + } }