From 3733094d5c1b4f15cd6102b0e9385ee5e1405fac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Fri, 23 Jun 2023 15:11:55 +0200 Subject: [PATCH] aws-sdk-2.2: More reflection cleanup. (#8775) Co-authored-by: Mateusz Rzeszutek --- .../aws-sdk-2.2/javaagent/build.gradle.kts | 49 +++++-- .../AbstractAwsSdkInstrumentationModule.java | 39 ++++++ .../v2_2/AwsSdkInstrumentationModule.java | 35 +---- .../awssdk/v2_2/SqsInstrumentationModule.java | 28 +--- .../aws-sdk-2.2/library/build.gradle.kts | 1 + .../awssdk/v2_2/SqsAccess.java | 44 +++++-- .../awssdk/v2_2/SqsAdviceBridge.java | 6 +- .../instrumentation/awssdk/v2_2/SqsImpl.java | 51 +++++++- .../v2_2/SqsReceiveMessageRequestAccess.java | 122 ------------------ .../v2_2/SqsSendMessageRequestAccess.java | 23 ---- .../v2_2/TracingExecutionInterceptor.java | 50 +------ .../awssdk/v2_2/Aws2ClientTest.groovy | 2 +- .../awssdk/v2_2/AbstractAws2ClientTest.groovy | 39 ++++++ .../v2_2/AbstractAws2SqsTracingTest.groovy | 13 +- 14 files changed, 216 insertions(+), 286 deletions(-) delete mode 100644 instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsReceiveMessageRequestAccess.java delete mode 100644 instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsSendMessageRequestAccess.java diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/build.gradle.kts b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/build.gradle.kts index 4def26b6629d..c33b3b1fa314 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/build.gradle.kts +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/build.gradle.kts @@ -15,9 +15,23 @@ muzzle { // several software.amazon.awssdk artifacts are missing for this version skip("2.17.200") } -} -muzzle { + fail { + group.set("software.amazon.awssdk") + module.set("aws-core") + versions.set("[2.2.0,)") + // Used by all SDK services, the only case it isn't is an SDK extension such as a custom HTTP + // client, which is not target of instrumentation anyways. + extraDependency("software.amazon.awssdk:protocol-core") + + // "fail" asserts that *all* the instrumentation modules fail to load, but the core one is + // actually expected to succeed, so exclude it from checks. + excludeInstrumentationName("aws-sdk-2.2-core") + + // several software.amazon.awssdk artifacts are missing for this version + skip("2.17.200") + } + pass { group.set("software.amazon.awssdk") module.set("sqs") @@ -53,17 +67,26 @@ dependencies { latestDepTestLibrary("software.amazon.awssdk:sqs:+") } -tasks.withType().configureEach { - systemProperty("testLatestDeps", findProperty("testLatestDeps") as Boolean) - // TODO run tests both with and without experimental span attributes, with & without extra propagation - systemProperties(mapOf( - "otel.instrumentation.aws-sdk.experimental-span-attributes" to "true", - "otel.instrumentation.aws-sdk.experimental-use-propagator-for-messaging" to "true", - )) -} +tasks { + val testExperimentalSqs by registering(Test::class) { + group = "verification" + + systemProperty("otel.instrumentation.aws-sdk.experimental-use-propagator-for-messaging", "true") + } + + check { + dependsOn(testExperimentalSqs) + } + + withType().configureEach { + systemProperty("testLatestDeps", findProperty("testLatestDeps") as Boolean) + // TODO run tests both with and without experimental span attributes + systemProperty("otel.instrumentation.aws-sdk.experimental-span-attributes", "true") + } -tasks.withType().configureEach { - mergeServiceFiles { - include("software/amazon/awssdk/global/handlers/execution.interceptors") + withType().configureEach { + mergeServiceFiles { + include("software/amazon/awssdk/global/handlers/execution.interceptors") + } } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java index e3cc3579cd3c..25b7b5c06c38 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AbstractAwsSdkInstrumentationModule.java @@ -5,7 +5,16 @@ package io.opentelemetry.javaagent.instrumentation.awssdk.v2_2; +import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed; +import static java.util.Collections.singletonList; +import static net.bytebuddy.matcher.ElementMatchers.named; + import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; +import java.util.List; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; abstract class AbstractAwsSdkInstrumentationModule extends InstrumentationModule { @@ -17,4 +26,34 @@ protected AbstractAwsSdkInstrumentationModule(String additionalInstrumentationNa public boolean isHelperClass(String className) { return className.startsWith("io.opentelemetry.contrib.awsxray."); } + + @Override + public ElementMatcher.Junction classLoaderMatcher() { + // We don't actually transform it but want to make sure we only apply the instrumentation when + // our key dependency is present. + return hasClassesNamed("software.amazon.awssdk.core.interceptor.ExecutionInterceptor"); + } + + @Override + public List typeInstrumentations() { + return singletonList(new ResourceInjectingTypeInstrumentation()); + } + + abstract void doTransform(TypeTransformer transformer); + + // A type instrumentation is needed to trigger resource injection. + public class ResourceInjectingTypeInstrumentation implements TypeInstrumentation { + @Override + public ElementMatcher typeMatcher() { + // This is essentially the entry point of the AWS SDK, all clients implement it. We can ensure + // our interceptor service definition is injected as early as possible if we typematch against + // it. + return named("software.amazon.awssdk.core.SdkClient"); + } + + @Override + public void transform(TypeTransformer transformer) { + doTransform(transformer); + } + } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java index 8ee1719a7c2e..a6bb33a49b77 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java @@ -5,19 +5,11 @@ package io.opentelemetry.javaagent.instrumentation.awssdk.v2_2; -import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed; -import static java.util.Collections.singletonList; -import static net.bytebuddy.matcher.ElementMatchers.named; - import com.google.auto.service.AutoService; import io.opentelemetry.instrumentation.awssdk.v2_2.autoconfigure.TracingExecutionInterceptor; import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; -import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import java.util.List; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) public class AwsSdkInstrumentationModule extends AbstractAwsSdkInstrumentationModule { @@ -35,30 +27,7 @@ public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) } @Override - public ElementMatcher.Junction classLoaderMatcher() { - // We don't actually transform it but want to make sure we only apply the instrumentation when - // our key dependency is present. - return hasClassesNamed("software.amazon.awssdk.core.interceptor.ExecutionInterceptor"); - } - - @Override - public List typeInstrumentations() { - return singletonList(new ResourceInjectingTypeInstrumentation()); - } - - // A type instrumentation is needed to trigger resource injection. - public static class ResourceInjectingTypeInstrumentation implements TypeInstrumentation { - @Override - public ElementMatcher typeMatcher() { - // This is essentially the entry point of the AWS SDK, all clients implement it. We can ensure - // our interceptor service definition is injected as early as possible if we typematch against - // it. - return named("software.amazon.awssdk.core.SdkClient"); - } - - @Override - public void transform(TypeTransformer transformer) { - // Nothing to transform, this type instrumentation is only used for injecting resources. - } + void doTransform(TypeTransformer transformer) { + // Nothing to transform, this type instrumentation is only used for injecting resources. } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/SqsInstrumentationModule.java b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/SqsInstrumentationModule.java index 380443ab96a1..3fdab6da5d2f 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/SqsInstrumentationModule.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/SqsInstrumentationModule.java @@ -5,19 +5,13 @@ package io.opentelemetry.javaagent.instrumentation.awssdk.v2_2; -import static java.util.Collections.singletonList; -import static net.bytebuddy.matcher.ElementMatchers.isConstructor; -import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.none; import com.google.auto.service.AutoService; import io.opentelemetry.instrumentation.awssdk.v2_2.SqsAdviceBridge; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; -import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import java.util.List; import net.bytebuddy.asm.Advice; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) public class SqsInstrumentationModule extends AbstractAwsSdkInstrumentationModule { @@ -27,21 +21,9 @@ public SqsInstrumentationModule() { } @Override - public List typeInstrumentations() { - return singletonList(new DefaultSqsClientTypeInstrumentation()); - } - - public static class DefaultSqsClientTypeInstrumentation implements TypeInstrumentation { - @Override - public ElementMatcher typeMatcher() { - return named("software.amazon.awssdk.services.sqs.DefaultSqsClient"); - } - - @Override - public void transform(TypeTransformer transformer) { - transformer.applyAdviceToMethod( - isConstructor(), SqsInstrumentationModule.class.getName() + "$RegisterAdvice"); - } + public void doTransform(TypeTransformer transformer) { + transformer.applyAdviceToMethod( + none(), SqsInstrumentationModule.class.getName() + "$RegisterAdvice"); } @SuppressWarnings("unused") @@ -50,7 +32,7 @@ public static class RegisterAdvice { public static void onExit() { // (indirectly) using SqsImpl class here to make sure it is available from SqsAccess // (injected into app classloader) and checked by Muzzle - SqsAdviceBridge.init(); + SqsAdviceBridge.referenceForMuzzleOnly(); } } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/build.gradle.kts b/instrumentation/aws-sdk/aws-sdk-2.2/library/build.gradle.kts index 56f43c7e1668..8ff9545dcb61 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/build.gradle.kts +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/build.gradle.kts @@ -26,5 +26,6 @@ tasks { test { systemProperty("testLatestDeps", findProperty("testLatestDeps") as Boolean) systemProperty("otel.instrumentation.aws-sdk.experimental-span-attributes", true) + systemProperty("otel.instrumentation.aws-sdk.experimental-use-propagator-for-messaging", true) } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAccess.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAccess.java index 6efff826aa29..f1f8aad68dd1 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAccess.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAccess.java @@ -8,12 +8,18 @@ import io.opentelemetry.context.propagation.TextMapPropagator; import io.opentelemetry.javaagent.tooling.muzzle.NoMuzzle; import software.amazon.awssdk.core.SdkRequest; +import software.amazon.awssdk.core.SdkResponse; import software.amazon.awssdk.core.interceptor.Context; import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest; +import software.amazon.awssdk.services.sqs.model.ReceiveMessageResponse; +import software.amazon.awssdk.services.sqs.model.SendMessageRequest; // helper class for calling methods that use sqs types in SqsImpl // if SqsImpl is not present these methods are no op final class SqsAccess { + private SqsAccess() {} + private static final boolean enabled = isSqsImplPresent(); private static boolean isSqsImplPresent() { @@ -31,14 +37,34 @@ private static boolean isSqsImplPresent() { } @NoMuzzle - static SdkRequest injectIntoSqsSendMessageRequest( + static boolean isSendMessageRequest(SdkRequest request) { + return enabled && request instanceof SendMessageRequest; + } + + @NoMuzzle + static SdkRequest injectIntoSendMessageRequest( TextMapPropagator messagingPropagator, SdkRequest rawRequest, io.opentelemetry.context.Context otelContext) { - if (!enabled) { - return rawRequest; - } - return SqsImpl.injectIntoSqsSendMessageRequest(messagingPropagator, rawRequest, otelContext); + assert enabled; // enabled checked already in instance check. + return SqsImpl.injectIntoSendMessageRequest(messagingPropagator, rawRequest, otelContext); + } + + @NoMuzzle + static boolean isReceiveMessageRequest(SdkRequest request) { + return enabled && request instanceof ReceiveMessageRequest; + } + + @NoMuzzle + public static SdkRequest modifyReceiveMessageRequest( + SdkRequest request, boolean useXrayPropagator, TextMapPropagator messagingPropagator) { + assert enabled; // enabled checked already in instance check. + return SqsImpl.modifyReceiveMessageRequest(request, useXrayPropagator, messagingPropagator); + } + + @NoMuzzle + static boolean isReceiveMessageResponse(SdkResponse response) { + return enabled && response instanceof ReceiveMessageResponse; } @NoMuzzle @@ -46,11 +72,7 @@ static void afterReceiveMessageExecution( TracingExecutionInterceptor config, Context.AfterExecution context, ExecutionAttributes executionAttributes) { - if (!enabled) { - return; - } - SqsImpl.afterConsumerResponse(config, executionAttributes, context); + assert enabled; // enabled checked already in instance check. + SqsImpl.afterReceiveMessageExecution(config, executionAttributes, context); } - - private SqsAccess() {} } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAdviceBridge.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAdviceBridge.java index 20eebbeac8e2..ddb3c05c5cbb 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAdviceBridge.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAdviceBridge.java @@ -8,8 +8,8 @@ public final class SqsAdviceBridge { private SqsAdviceBridge() {} - public static void init() { - // called from advice - SqsImpl.init(); // Reference the actual, package-private, implementation class for Muzzle + public static void referenceForMuzzleOnly() { + throw new UnsupportedOperationException( + SqsImpl.class.getName() + " referencing for muzzle, should never be actually called"); } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java index 99c1f155450e..df3b402206f8 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java @@ -7,7 +7,9 @@ import io.opentelemetry.context.propagation.TextMapPropagator; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import software.amazon.awssdk.core.SdkRequest; import software.amazon.awssdk.core.interceptor.Context; @@ -15,6 +17,7 @@ import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.services.sqs.model.Message; import software.amazon.awssdk.services.sqs.model.MessageAttributeValue; +import software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest; import software.amazon.awssdk.services.sqs.model.ReceiveMessageResponse; import software.amazon.awssdk.services.sqs.model.SendMessageRequest; @@ -22,11 +25,7 @@ final class SqsImpl { private SqsImpl() {} - public static void init() { - // called from advice - } - - static SdkRequest injectIntoSqsSendMessageRequest( + static SdkRequest injectIntoSendMessageRequest( TextMapPropagator messagingPropagator, SdkRequest rawRequest, io.opentelemetry.context.Context otelContext) { @@ -48,7 +47,7 @@ static SdkRequest injectIntoSqsSendMessageRequest( } /** Create and close CONSUMER span for each message consumed. */ - static void afterConsumerResponse( + static void afterReceiveMessageExecution( TracingExecutionInterceptor config, ExecutionAttributes executionAttributes, Context.AfterExecution context) { @@ -91,4 +90,44 @@ private static void createConsumerSpan( consumerInstrumenter.end(context, executionAttributes, httpResponse, null); } } + + static SdkRequest modifyReceiveMessageRequest( + SdkRequest rawRequest, boolean useXrayPropagator, TextMapPropagator messagingPropagator) { + ReceiveMessageRequest request = (ReceiveMessageRequest) rawRequest; + boolean hasXrayAttribute = true; + List existingAttributeNames = null; + if (useXrayPropagator) { + existingAttributeNames = request.attributeNamesAsStrings(); + hasXrayAttribute = + existingAttributeNames.contains(SqsParentContext.AWS_TRACE_SYSTEM_ATTRIBUTE); + } + + boolean hasMessageAttribute = true; + List existingMessageAttributeNames = null; + if (messagingPropagator != null) { + existingMessageAttributeNames = request.messageAttributeNames(); + hasMessageAttribute = existingMessageAttributeNames.containsAll(messagingPropagator.fields()); + } + + if (hasMessageAttribute && hasXrayAttribute) { + return request; + } + + ReceiveMessageRequest.Builder builder = request.toBuilder(); + if (!hasXrayAttribute) { + List attributeNames = new ArrayList<>(existingAttributeNames); + attributeNames.add(SqsParentContext.AWS_TRACE_SYSTEM_ATTRIBUTE); + builder.attributeNamesWithStrings(attributeNames); + } + if (messagingPropagator != null) { + List messageAttributeNames = new ArrayList<>(existingMessageAttributeNames); + for (String field : messagingPropagator.fields()) { + if (!existingMessageAttributeNames.contains(field)) { + messageAttributeNames.add(field); + } + } + builder.messageAttributeNames(messageAttributeNames); + } + return builder.build(); + } } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsReceiveMessageRequestAccess.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsReceiveMessageRequestAccess.java deleted file mode 100644 index b6ac23ded0fe..000000000000 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsReceiveMessageRequestAccess.java +++ /dev/null @@ -1,122 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.awssdk.v2_2; - -import static java.lang.invoke.MethodType.methodType; - -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.util.Collection; -import java.util.Collections; -import java.util.List; -import java.util.Optional; -import javax.annotation.Nullable; -import software.amazon.awssdk.core.SdkRequest; - -/** - * Reflective access to aws-sdk-java-sqs class ReceiveMessageRequest. - * - *

We currently don't have a good pattern of instrumenting a core library with various plugins - * that need plugin-specific instrumentation - if we accessed the class directly, Muzzle would - * prevent the entire instrumentation from loading when the plugin isn't available. We need to - * carefully check this class has all reflection errors result in no-op, and in the future we will - * hopefully come up with a better pattern. - * - * @see SDK - * Javadoc - * @see Definition - * JSON - */ -final class SqsReceiveMessageRequestAccess { - - @Nullable private static final MethodHandle ATTRIBUTE_NAMES_WITH_STRINGS; - @Nullable private static final MethodHandle MESSAGE_ATTRIBUTE_NAMES; - - static { - Class receiveMessageRequestClass = null; - try { - receiveMessageRequestClass = - Class.forName("software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest$Builder"); - } catch (Throwable t) { - // Ignore. - } - if (receiveMessageRequestClass != null) { - MethodHandles.Lookup lookup = MethodHandles.publicLookup(); - MethodHandle withAttributeNames = null; - try { - withAttributeNames = - lookup.findVirtual( - receiveMessageRequestClass, - "attributeNamesWithStrings", - methodType(receiveMessageRequestClass, Collection.class)); - } catch (NoSuchMethodException | IllegalAccessException e) { - // Ignore - } - ATTRIBUTE_NAMES_WITH_STRINGS = withAttributeNames; - - MethodHandle messageAttributeNames = null; - try { - messageAttributeNames = - lookup.findVirtual( - receiveMessageRequestClass, - "messageAttributeNames", - methodType(receiveMessageRequestClass, Collection.class)); - } catch (NoSuchMethodException | IllegalAccessException e) { - // Ignore - } - MESSAGE_ATTRIBUTE_NAMES = messageAttributeNames; - } else { - ATTRIBUTE_NAMES_WITH_STRINGS = null; - MESSAGE_ATTRIBUTE_NAMES = null; - } - } - - static boolean isInstance(SdkRequest request) { - return request - .getClass() - .getName() - .equals("software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest"); - } - - static void attributeNamesWithStrings(SdkRequest.Builder builder, List attributeNames) { - if (ATTRIBUTE_NAMES_WITH_STRINGS == null) { - return; - } - try { - ATTRIBUTE_NAMES_WITH_STRINGS.invoke(builder, attributeNames); - } catch (Throwable throwable) { - // Ignore - } - } - - static void messageAttributeNames( - SdkRequest.Builder builder, List messageAttributeNames) { - if (MESSAGE_ATTRIBUTE_NAMES == null) { - return; - } - try { - MESSAGE_ATTRIBUTE_NAMES.invoke(builder, messageAttributeNames); - } catch (Throwable throwable) { - // Ignore - } - } - - private SqsReceiveMessageRequestAccess() {} - - @SuppressWarnings({"rawtypes", "unchecked"}) - static List getAttributeNames(SdkRequest request) { - Optional optional = request.getValueForField("AttributeNames", List.class); - return optional.isPresent() ? (List) optional.get() : Collections.emptyList(); - } - - @SuppressWarnings({"rawtypes", "unchecked"}) - static List getMessageAttributeNames(SdkRequest request) { - Optional optional = request.getValueForField("MessageAttributeNames", List.class); - return optional.isPresent() ? (List) optional.get() : Collections.emptyList(); - } -} diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsSendMessageRequestAccess.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsSendMessageRequestAccess.java deleted file mode 100644 index b863ef3ceaf2..000000000000 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsSendMessageRequestAccess.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.awssdk.v2_2; - -import software.amazon.awssdk.core.SdkRequest; - -/** - * Reflective access to aws-sdk-java-sqs class ReceiveMessageRequest for points where we are not - * sure whether SQS is on the classpath. - */ -final class SqsSendMessageRequestAccess { - static boolean isInstance(SdkRequest request) { - return request - .getClass() - .getName() - .equals("software.amazon.awssdk.services.sqs.model.SendMessageRequest"); - } - - private SqsSendMessageRequestAccess() {} -} diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java index 95d81cd3034d..ccdc223ec5fc 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java @@ -15,7 +15,6 @@ import io.opentelemetry.contrib.awsxray.propagator.AwsXrayPropagator; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; -import java.util.ArrayList; import java.util.List; import javax.annotation.Nullable; import software.amazon.awssdk.awscore.AwsResponse; @@ -122,56 +121,17 @@ public SdkRequest modifyRequest( throw throwable; } - if (SqsReceiveMessageRequestAccess.isInstance(request)) { - return modifySqsReceiveMessageRequest(request); + if (SqsAccess.isReceiveMessageRequest(request)) { + return SqsAccess.modifyReceiveMessageRequest(request, useXrayPropagator, messagingPropagator); } else if (messagingPropagator != null) { - if (SqsSendMessageRequestAccess.isInstance(request)) { - return SqsAccess.injectIntoSqsSendMessageRequest(messagingPropagator, request, otelContext); + if (SqsAccess.isSendMessageRequest(request)) { + return SqsAccess.injectIntoSendMessageRequest(messagingPropagator, request, otelContext); } // TODO: Support SendMessageBatchRequest (and thus SendMessageBatchRequestEntry) } return request; } - private SdkRequest modifySqsReceiveMessageRequest(SdkRequest request) { - boolean hasXrayAttribute = true; - List existingAttributeNames = null; - if (useXrayPropagator) { - existingAttributeNames = SqsReceiveMessageRequestAccess.getAttributeNames(request); - hasXrayAttribute = - existingAttributeNames.contains(SqsParentContext.AWS_TRACE_SYSTEM_ATTRIBUTE); - } - - boolean hasMessageAttribute = true; - List existingMessageAttributeNames = null; - if (messagingPropagator != null) { - existingMessageAttributeNames = - SqsReceiveMessageRequestAccess.getMessageAttributeNames(request); - hasMessageAttribute = existingMessageAttributeNames.containsAll(messagingPropagator.fields()); - } - - if (hasMessageAttribute && hasXrayAttribute) { - return request; - } - - SdkRequest.Builder builder = request.toBuilder(); - if (!hasXrayAttribute) { - List attributeNames = new ArrayList<>(existingAttributeNames); - attributeNames.add(SqsParentContext.AWS_TRACE_SYSTEM_ATTRIBUTE); - SqsReceiveMessageRequestAccess.attributeNamesWithStrings(builder, attributeNames); - } - if (messagingPropagator != null) { - List messageAttributeNames = new ArrayList<>(existingMessageAttributeNames); - for (String field : messagingPropagator.fields()) { - if (!existingMessageAttributeNames.contains(field)) { - messageAttributeNames.add(field); - } - } - SqsReceiveMessageRequestAccess.messageAttributeNames(builder, messageAttributeNames); - } - return builder.build(); - } - @Override public void afterMarshalling( Context.AfterMarshalling context, ExecutionAttributes executionAttributes) { @@ -265,7 +225,7 @@ private void populateRequestAttributes( @Override public void afterExecution( Context.AfterExecution context, ExecutionAttributes executionAttributes) { - if (SqsReceiveMessageRequestAccess.isInstance(context.request())) { + if (SqsAccess.isReceiveMessageResponse(context.response())) { SqsAccess.afterReceiveMessageExecution(this, context, executionAttributes); } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/test/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/Aws2ClientTest.groovy b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/test/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/Aws2ClientTest.groovy index 211773859068..40a88e4c5863 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/library/src/test/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/Aws2ClientTest.groovy +++ b/instrumentation/aws-sdk/aws-sdk-2.2/library/src/test/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/Aws2ClientTest.groovy @@ -15,7 +15,7 @@ class Aws2ClientTest extends AbstractAws2ClientTest implements LibraryTestTrait .addExecutionInterceptor( AwsSdkTelemetry.builder(getOpenTelemetry()) .setCaptureExperimentalSpanAttributes(true) - .setUseConfiguredPropagatorForMessaging(true) // Default on in tests to cover more code + .setUseConfiguredPropagatorForMessaging(isSqsAttributeInjectionEnabled()) .build() .newExecutionInterceptor()) } diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.groovy b/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.groovy index a3fcdca522da..3a10459e6dea 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.groovy +++ b/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2ClientTest.groovy @@ -5,12 +5,14 @@ package io.opentelemetry.instrumentation.awssdk.v2_2 +import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil import io.opentelemetry.instrumentation.test.InstrumentationSpecification import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import io.opentelemetry.testing.internal.armeria.common.HttpResponse import io.opentelemetry.testing.internal.armeria.common.HttpStatus import io.opentelemetry.testing.internal.armeria.common.MediaType import io.opentelemetry.testing.internal.armeria.testing.junit5.server.mock.MockWebServerExtension +import org.junit.jupiter.api.Assumptions import software.amazon.awssdk.auth.credentials.AwsBasicCredentials import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider import software.amazon.awssdk.core.ResponseInputStream @@ -48,6 +50,7 @@ import software.amazon.awssdk.services.s3.model.GetObjectRequest import software.amazon.awssdk.services.sqs.SqsAsyncClient import software.amazon.awssdk.services.sqs.SqsClient import software.amazon.awssdk.services.sqs.model.CreateQueueRequest +import software.amazon.awssdk.services.sqs.model.SendMessageRequest import spock.lang.Shared import spock.lang.Unroll @@ -317,7 +320,22 @@ abstract class AbstractAws2ClientTest extends InstrumentationSpecification { return AttributeValue.builder().s(value).build() } + def isSqsAttributeInjectionEnabled() { + // See io.opentelemetry.instrumentation.awssdk.v2_2.autoconfigure.TracingExecutionInterceptor + return ConfigPropertiesUtil.getBoolean("otel.instrumentation.aws-sdk.experimental-use-propagator-for-messaging", false) + } + + void assumeSupportedConfig(service, operation) { + Assumptions.assumeFalse( + service == "Sqs" + && operation == "SendMessage" + && isSqsAttributeInjectionEnabled(), + "Cannot check Sqs.SendMessage here due to hard-coded MD5.") + } + def "send #operation request with builder #builder.class.getName() mocked response"() { + assumeSupportedConfig(service, operation) + setup: configureSdkClient(builder) def client = builder @@ -384,6 +402,16 @@ abstract class AbstractAws2ClientTest extends InstrumentationSpecification { 7a62c49f-347e-4fc4-9331-6e8e7a96aa73 """ + "Sqs" | "SendMessage" | "POST" | "" | "27daac76-34dd-47df-bd01-1f6e873584a0" | SqsClient.builder() | { c -> c.sendMessage(SendMessageRequest.builder().queueUrl("someurl").messageBody("").build()) } | """ + + + d41d8cd98f00b204e9800998ecf8427e + 3ae8f24a165a8cedc005670c81a27295 + 5fea7756-0ea4-451a-a703-a558b933e274 + + 27daac76-34dd-47df-bd01-1f6e873584a0 + + """ "Ec2" | "AllocateAddress" | "POST" | "" | "59dbff89-35bd-4eac-99ed-be587EXAMPLE" | Ec2Client.builder() | { c -> c.allocateAddress() } | """ 59dbff89-35bd-4eac-99ed-be587EXAMPLE @@ -399,6 +427,7 @@ abstract class AbstractAws2ClientTest extends InstrumentationSpecification { } def "send #operation async request with builder #builder.class.getName() mocked response"() { + assumeSupportedConfig(service, operation) setup: configureSdkClient(builder) def client = builder @@ -465,6 +494,16 @@ abstract class AbstractAws2ClientTest extends InstrumentationSpecification { 7a62c49f-347e-4fc4-9331-6e8e7a96aa73 """ + "Sqs" | "SendMessage" | "POST" | "" | "27daac76-34dd-47df-bd01-1f6e873584a0" | SqsAsyncClient.builder() | { c -> c.sendMessage(SendMessageRequest.builder().queueUrl("someurl").messageBody("").build()) } | """ + + + d41d8cd98f00b204e9800998ecf8427e + 3ae8f24a165a8cedc005670c81a27295 + 5fea7756-0ea4-451a-a703-a558b933e274 + + 27daac76-34dd-47df-bd01-1f6e873584a0 + + """ "Ec2" | "AllocateAddress" | "POST" | "" | "59dbff89-35bd-4eac-99ed-be587EXAMPLE" | Ec2AsyncClient.builder() | { c -> c.allocateAddress() } | """ 59dbff89-35bd-4eac-99ed-be587EXAMPLE diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2SqsTracingTest.groovy b/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2SqsTracingTest.groovy index 7a7a96efefb0..a86c9bc8a81a 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2SqsTracingTest.groovy +++ b/instrumentation/aws-sdk/aws-sdk-2.2/testing/src/main/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/AbstractAws2SqsTracingTest.groovy @@ -6,7 +6,6 @@ package io.opentelemetry.instrumentation.awssdk.v2_2 import io.opentelemetry.instrumentation.test.InstrumentationSpecification -import io.opentelemetry.instrumentation.test.utils.PortUtils import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import org.elasticmq.rest.sqs.SQSRestServerBuilder import software.amazon.awssdk.auth.credentials.AwsBasicCredentials @@ -61,10 +60,10 @@ abstract class AbstractAws2SqsTracingTest extends InstrumentationSpecification { abstract ClientOverrideConfiguration.Builder createOverrideConfigurationBuilder() def setupSpec() { - sqsPort = PortUtils.findOpenPort() - sqs = SQSRestServerBuilder.withPort(sqsPort).withInterface("localhost").start() + sqs = SQSRestServerBuilder.withPort(0).withInterface("localhost").start() + def server = sqs.waitUntilStarted() + sqsPort = server.localAddress().port println getClass().name + " SQS server started at: localhost:$sqsPort/" - } def cleanupSpec() { @@ -181,9 +180,10 @@ abstract class AbstractAws2SqsTracingTest extends InstrumentationSpecification { when: client.sendMessage(sendMessageRequest) - client.receiveMessage(receiveMessageRequest) + def resp = client.receiveMessage(receiveMessageRequest) then: + resp.messages().size() == 1 assertSqsTraces() } @@ -198,9 +198,10 @@ abstract class AbstractAws2SqsTracingTest extends InstrumentationSpecification { when: client.sendMessage(sendMessageRequest).get() - client.receiveMessage(receiveMessageRequest).get() + def resp = client.receiveMessage(receiveMessageRequest).get() then: + resp.messages().size() == 1 assertSqsTraces() } }