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

Fix aws-sdk-2.2 library instrumentation crashing app when SQS is not on classpath #8805

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Fix aws-sdk-2.2 library instrumentation crashing app when SQS is not on classpath #8805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
07f8ed5
aws-sdk-2.2: Fix library-mode not working w/o SQS on classpath.
Oberon00 Jun 26, 2023
2e3bbcf
aws-sdk-2.2: Add test with core-only.
Oberon00 Jun 26, 2023
f8ef380
Lint & test dependencies
Oberon00 Jun 26, 2023
5f78040
Fix wrong comment: Sns -> Sqs
Oberon00 Jun 26, 2023
e53bf95
Add comment re. non-use of getPackage.
Oberon00 Jun 27, 2023
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
14 changes: 6 additions & 8 deletions instrumentation/aws-sdk/aws-sdk-2.2/javaagent/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,12 @@ dependencies {
testImplementation(project(":instrumentation:apache-httpclient:apache-httpclient-4.0:javaagent"))
testImplementation(project(":instrumentation:netty:netty-4.1:javaagent"))

latestDepTestLibrary("software.amazon.awssdk:aws-json-protocol:+")
latestDepTestLibrary("software.amazon.awssdk:aws-core:+")
latestDepTestLibrary("software.amazon.awssdk:dynamodb:+")
latestDepTestLibrary("software.amazon.awssdk:ec2:+")
latestDepTestLibrary("software.amazon.awssdk:kinesis:+")
latestDepTestLibrary("software.amazon.awssdk:rds:+")
latestDepTestLibrary("software.amazon.awssdk:s3:+")
latestDepTestLibrary("software.amazon.awssdk:sqs:+")
testLibrary("software.amazon.awssdk:dynamodb:2.2.0")
testLibrary("software.amazon.awssdk:ec2:2.2.0")
testLibrary("software.amazon.awssdk:kinesis:2.2.0")
testLibrary("software.amazon.awssdk:rds:2.2.0")
testLibrary("software.amazon.awssdk:s3:2.2.0")
testLibrary("software.amazon.awssdk:sqs:2.2.0")
}

tasks {
Expand Down
14 changes: 6 additions & 8 deletions instrumentation/aws-sdk/aws-sdk-2.2/library-autoconfigure/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@ dependencies {

testImplementation(project(":instrumentation:aws-sdk:aws-sdk-2.2:testing"))

latestDepTestLibrary("software.amazon.awssdk:aws-core:+")
latestDepTestLibrary("software.amazon.awssdk:aws-json-protocol:+")
latestDepTestLibrary("software.amazon.awssdk:dynamodb:+")
latestDepTestLibrary("software.amazon.awssdk:ec2:+")
latestDepTestLibrary("software.amazon.awssdk:kinesis:+")
latestDepTestLibrary("software.amazon.awssdk:rds:+")
latestDepTestLibrary("software.amazon.awssdk:s3:+")
latestDepTestLibrary("software.amazon.awssdk:sqs:+")
testLibrary("software.amazon.awssdk:dynamodb:2.2.0")
testLibrary("software.amazon.awssdk:ec2:2.2.0")
testLibrary("software.amazon.awssdk:kinesis:2.2.0")
testLibrary("software.amazon.awssdk:rds:2.2.0")
testLibrary("software.amazon.awssdk:s3:2.2.0")
testLibrary("software.amazon.awssdk:sqs:2.2.0")
}

tasks {
Expand Down
35 changes: 26 additions & 9 deletions instrumentation/aws-sdk/aws-sdk-2.2/library/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,35 @@ dependencies {

testImplementation(project(":instrumentation:aws-sdk:aws-sdk-2.2:testing"))

latestDepTestLibrary("software.amazon.awssdk:aws-core:+")
latestDepTestLibrary("software.amazon.awssdk:aws-json-protocol:+")
latestDepTestLibrary("software.amazon.awssdk:dynamodb:+")
latestDepTestLibrary("software.amazon.awssdk:ec2:+")
latestDepTestLibrary("software.amazon.awssdk:kinesis:+")
latestDepTestLibrary("software.amazon.awssdk:rds:+")
latestDepTestLibrary("software.amazon.awssdk:s3:+")
latestDepTestLibrary("software.amazon.awssdk:sqs:+")
testLibrary("software.amazon.awssdk:dynamodb:2.2.0")
testLibrary("software.amazon.awssdk:ec2:2.2.0")
testLibrary("software.amazon.awssdk:kinesis:2.2.0")
testLibrary("software.amazon.awssdk:rds:2.2.0")
testLibrary("software.amazon.awssdk:s3:2.2.0")
}

testing {
suites {
val testCoreOnly by registering(JvmTestSuite::class) {
sources {
groovy {
setSrcDirs(listOf("src/testCoreOnly/groovy"))
}
}

dependencies {
implementation(project())
implementation(project(":instrumentation:aws-sdk:aws-sdk-2.2:testing"))
implementation("software.amazon.awssdk:aws-core:2.2.0")
implementation("software.amazon.awssdk:aws-json-protocol:2.2.0")
implementation("software.amazon.awssdk:dynamodb:2.2.0")
}
}
}
}

tasks {
test {
withType<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)
Expand Down
49 changes: 49 additions & 0 deletions ....2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/PluginImplUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.awssdk.v2_2;

import java.util.logging.Level;
import java.util.logging.Logger;

final class PluginImplUtil {
private PluginImplUtil() {}

private static final Logger logger = Logger.getLogger(PluginImplUtil.class.getName());

/**
* Check if the given {@code moduleNameImpl} is present.
*
* <p>For library instrumentations, the Impls will always be available but might fail to load if
* the corresponding SDK classes are not on the class path. For javaagent, the Impl is available
* only when the corresponding InstrumentationModule was successfully applied (muzzle passed).
*
* <p>Note that an present-but-incompatible library can only be detected by Muzzle. In
* library-mode, users need to ensure they are using a compatible SDK (component) versions
* themselves.
*
* @param implSimpleClassName The simple name of the impl class, e.g. {@code "SqsImpl"}. *
*/
static boolean isImplPresent(String implSimpleClassName) {
// We use getName().replace() instead of getPackage() because the latter is not guaranteed to
// work in all cases (e.g., we might be loaded into a custom classloader that doesn't handle it)
String implFullClassName =
PluginImplUtil.class.getName().replace(".PluginImplUtil", "." + implSimpleClassName);
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
try {
// using package name here because library instrumentation classes are relocated when embedded
// in the agent
Class.forName(implFullClassName);
return true;
} catch (ClassNotFoundException | LinkageError e) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main change 1: Catch LinkageError (incl. NoClassDefFoundError)

// ClassNotFoundException will happen when muzzle disabled us in javaagent mode; LinkageError
// (most likely NoClassDefFoundError) should happen when the class is loaded in library mode
// (where it is always available) but a dependency failed to load (most likely because the
// corresponding SDK dependency is not on the class path).
logger.log(
Level.FINE, e, () -> implFullClassName + " not present, probably incompatible version");
return false;
}
}
}
16 changes: 1 addition & 15 deletions ...sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,7 @@
final class SqsAccess {
private SqsAccess() {}

private static final boolean enabled = isSqsImplPresent();

private static boolean isSqsImplPresent() {
try {
// for library instrumentation SqsImpl is always available
// for javaagent instrumentation SqsImpl is available only when SqsInstrumentationModule was
// successfully applied (muzzle passed)
// using package name here because library instrumentation classes are relocated when embedded
// in the agent
Class.forName(SqsAccess.class.getName().replace(".SqsAccess", ".SqsImpl"));
return true;
} catch (ClassNotFoundException e) {
return false;
}
}
private static final boolean enabled = PluginImplUtil.isImplPresent("SqsImpl");

@NoMuzzle
static boolean isSendMessageRequest(SdkRequest request) {
Expand Down
8 changes: 8 additions & 0 deletions ...s-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import software.amazon.awssdk.core.interceptor.Context;
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
import software.amazon.awssdk.http.SdkHttpResponse;
import software.amazon.awssdk.services.sqs.SqsClient;
import software.amazon.awssdk.services.sqs.model.Message;
import software.amazon.awssdk.services.sqs.model.MessageAttributeValue;
import software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest;
Expand All @@ -23,6 +24,13 @@

// this class is only used from SqsAccess from method with @NoMuzzle annotation
final class SqsImpl {
static {
// Force loading of SqsClient; this ensures that an exception is thrown at this point when the
// SQS library is not present, which will cause SqsAccess to have enabled=false in library mode.
@SuppressWarnings("unused")
String ensureLoadedDummy = SqsClient.class.getName();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main change 2: Ensure that initializing the class fails when the required module is not on the classpath.

Note that for me, this change here was not even required and it seemed to fail already anyways. But from trying to understand https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-5.html#jvms-5.4, I think without this change it would be JVM-implementation defined if we would get an error when initializing the class or only later when actually calling a method that has an unloadable type in its signature/implementation.

}

private SqsImpl() {}

static SdkRequest injectIntoSendMessageRequest(
Expand Down
22 changes: 22 additions & 0 deletions ...oreOnly/groovy/io/opentelemetry/instrumentation/awssdk/v2_2/Aws2ClientDynamodbTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.awssdk.v2_2

import io.opentelemetry.instrumentation.test.LibraryTestTrait
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration

class Aws2ClientDynamodbTest extends AbstractAws2ClientCoreTest implements LibraryTestTrait {
@Override
ClientOverrideConfiguration.Builder createOverrideConfigurationBuilder() {
return ClientOverrideConfiguration.builder()
.addExecutionInterceptor(
AwsSdkTelemetry.builder(getOpenTelemetry())
.setCaptureExperimentalSpanAttributes(true)
.setUseConfiguredPropagatorForMessaging(isSqsAttributeInjectionEnabled())
.build()
.newExecutionInterceptor())
}
}
17 changes: 9 additions & 8 deletions instrumentation/aws-sdk/aws-sdk-2.2/testing/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ dependencies {
api("software.amazon.awssdk:apache-client:2.2.0")
// older versions don't play nice with armeria http server
api("software.amazon.awssdk:netty-nio-client:2.11.0")
// When adding libraries, make sure to also add to the library/javaagent build files
// to ensure they are bumped to the latest version under testLatestDeps
api("software.amazon.awssdk:dynamodb:2.2.0")
api("software.amazon.awssdk:ec2:2.2.0")
api("software.amazon.awssdk:kinesis:2.2.0")
api("software.amazon.awssdk:rds:2.2.0")
api("software.amazon.awssdk:s3:2.2.0")
api("software.amazon.awssdk:sqs:2.2.0")

// compileOnly because we never want to pin the low version implicitly; need to add dependencies
Copy link
Member Author

@Oberon00 Oberon00 Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was required to allow using the testing module from the testCoreOnly test suite without also dragging in SQS. It required a slew of follow-up changes, but I think the end result is a bit cleaner, and also aligned with what the v1.1 SDK does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 👍
Honestly this is the pattern that I prefer myself when it comes to the testing modules -- let the actual instrumentation tests bring the versions they want.

// explicitly in user projects, e.g. using testLatestDeps.
compileOnly("software.amazon.awssdk:dynamodb:2.2.0")
compileOnly("software.amazon.awssdk:ec2:2.2.0")
compileOnly("software.amazon.awssdk:kinesis:2.2.0")
compileOnly("software.amazon.awssdk:rds:2.2.0")
compileOnly("software.amazon.awssdk:s3:2.2.0")
compileOnly("software.amazon.awssdk:sqs:2.2.0")

// needed for SQS - using emq directly as localstack references emq v0.15.7 ie WITHOUT AWS trace header propagation
implementation("org.elasticmq:elasticmq-rest-sqs_2.12:1.0.0")
Expand Down
Loading