Fix aws-sdk-2.2 library instrumentation crashing app when SQS is not on classpath #8805
Conversation
| // in the agent | ||
| Class.forName(implFullClassName); | ||
| return true; | ||
| } catch (ClassNotFoundException | LinkageError e) { |
There was a problem hiding this comment.
Main change 1: Catch LinkageError (incl. NoClassDefFoundError)
| // Force loading of SqsClient; this ensures that an exception is thrown at this point when the | ||
| // SQS library is not present, which will cause SnsAccess to have enabled=false in library mode. | ||
| @SuppressWarnings("unused") | ||
| String ensureLoadedDummy = SqsClient.class.getName(); |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
.../aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java
Outdated
Show resolved
Hide resolved
...k-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/PluginImplUtil.java
Show resolved
Hide resolved
| 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 |
There was a problem hiding this comment.
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.
|
thx for catching this! |
This is a regression introduced in my previous merged PR #8775. See especially thread at #8775 (comment). I was only thinking about the javaagent case when doing this, where Muzzle will ensure that everything is safe, but for the library & library-autoconfigure mode we need to be safe against the case that SqsImpl is on the classpath but the actual SDK SQS classes are not.
Adding tests for this also required fixes to how test dependencies are specified, the result is now more aligned with how it's done for the v1 SDK instrumentation.