AWS SDK v2.2 SPI Implementation for ADOT Java SDK #1091
Conversation
.../amazon/opentelemetry/javaagent/instrumentation/awssdk_v2_2/AwsSdkInstrumentationModule.java
Outdated
Show resolved
Hide resolved
.../amazon/opentelemetry/javaagent/instrumentation/awssdk_v2_2/AwsSdkInstrumentationModule.java
Outdated
Show resolved
Hide resolved
.../amazon/opentelemetry/javaagent/instrumentation/awssdk_v2_2/AwsSdkInstrumentationModule.java
Outdated
Show resolved
Hide resolved
...re/amazon/opentelemetry/javaagent/instrumentation/awssdk_v2_2/AwsSdkInstrumenterFactory.java
Outdated
Show resolved
Hide resolved
.../amazon/opentelemetry/javaagent/instrumentation/awssdk_v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
.../amazon/opentelemetry/javaagent/instrumentation/awssdk_v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
…ptor after request has been marshalled into HTTP requestion, but before it is sent
|
I have updated my branch with changes to account for the feedback given. This implementation of the aws-sdk v2.2 instrumentation passes the v2 contract tests in ADOT as well. |
| dependencies { | ||
|
|
||
| compileOnly("software.amazon.awssdk:json-utils:2.17.0") | ||
| compileOnly("com.google.code.findbugs:jsr305:3.0.2") |
There was a problem hiding this comment.
Yes, lines 26 and 27 are needed for AwsJsonProtocolFactoryAccess and BedrockJsonParser
|
|
||
| private final FieldMapper fieldMapper = new FieldMapper(); | ||
|
|
||
| // This is the latest point we can obtain the Sdk Request after it is modified by the upstream |
There was a problem hiding this comment.
I feel this may not be the right understanding and we can double check with Ping.
My understanding: The upstream instrumentation precedes our patching, as defined by the sequence in AdotAwsSdkInstrumentationModule. Within the AWS SDK request lifecycle, our patching intercepts the beforeTransmission and afterUnmarshalling events to inject AWS attributes according to the established instrumentation order.
| request(AWS_STATE_MACHINE_ARN.getKey(), "stateMachineArn"), | ||
| request(AWS_STEP_FUNCTIONS_ACTIVITY_ARN.getKey(), "activityArn")), | ||
|
|
||
| // SNS(request(AWS_SNS_TOPIC_ARN.getKey(), "TopicArn")), |
There was a problem hiding this comment.
Can you help remove it? As it is moved to line 68. Thanks.
| // This is the earliest point we can obtain the Sdk Response before it is modified by the upstream | ||
| // interceptor. This ensures the execution attribute (AWS_SDK_REQUEST_ATTRIBUTE) added in by the | ||
| // interceptor is handled only by this interceptor, and not the upstream interceptor. | ||
| @Override |
There was a problem hiding this comment.
Can we move this action to afterExecution as done by upstream so that our changes/updates come after upstream? So far, we don't modify attributes set by OTel, so the order doesn't matter too much. But if we did, based on this implementation, the modified attributes in afterUnmarshalling will be overwritten by OTel in afterExecution step.
Update:
In Otel interceptor, it clearsAWS_SDK_REQUEST_ATTRIBUTE in executionAttributes duringafterExecution.
There was a problem hiding this comment.
Yes, I had tried to instrument afterExecution initially but the upstream clears all the execution attributes. I think using afterUnmarshalling ensures that the interceptor that put in the execution attributes last (i.e. AdotTracingExecutionInterceptor) should be the first to handle the execution attributes. This also ensures the otel span is available to modify at this point of interception.
instrumentation/aws-sdk/README.md
Outdated
| ## ADOT AWS SDK Instrumentation | ||
|
|
||
| ### Overview | ||
| This instrumentation is an SPI-based implementation, in place of ADOT's git patches, to extend the AWS SDK within the upstream OpenTelemetry Instrumentation for Java. |
There was a problem hiding this comment.
Thanks for providing detailed explanation. It helps reviewing the PR easier. However, for people who have no context on the patching mechanism, especially after we implemented SPI, the term "git patches" might be unclear. Let's remove those references and focus on describing the specific patches being added.
| compileOnly("software.amazon.awssdk:lambda:2.2.0") | ||
| compileOnly("software.amazon.awssdk:aws-json-protocol:2.2.0") | ||
| compileOnly("software.amazon.awssdk:sfn:2.2.0") | ||
| compileOnly("software.amazon.awssdk:lambda:2.2.0") |
There was a problem hiding this comment.
Duplicated from line 39
| testImplementation("org.assertj:assertj-core:3.24.2") | ||
| testImplementation("org.mockito:mockito-junit-jupiter:3.12.4") | ||
|
|
||
| // AWS SDK test dependencies |
There was a problem hiding this comment.
We can remove them as they are not used for BedrockJsonParserTest
|
|
||
| testImplementation("io.opentelemetry.javaagent:opentelemetry-javaagent-extension-api") | ||
| testImplementation("io.opentelemetry.instrumentation:opentelemetry-instrumentation-api") | ||
| } |
There was a problem hiding this comment.
Please review the dependencies to remove the non-effective ones. Thanks.
Issue
The current ADOT Java SDK implementation relies on a combination of OpenTelemetry SPI and Git patches to extend the OTel SDK functionality. This approach presents several challenges:
Description of changes:
This PR introduces a new SPI-based implementation for AWS SDK v2.2 instrumentation that:
InstrumentationModuleextension mechanismThe SPI instrumentation module is under 'instrumentation/aws-sdk.' There is also a custom 'TracingExecutionInterceptor' for this implementation for ensure correct instrumentation for files that required patching previously.
Testing
To validate the SPI implementation, I utilized the existing AppSignals contract tests. These tests were originally designed to verify AWS SDK v2 instrumentation behaviour. The tests specifically assert that AWS-specific span attributes are correctly captured in the mock collector (i.e. tests git patching logic).
By removing the AWS SDK v2.2 git patching logic from the patching script and running these same contract tests, I could verify that the SPI implementation maintains identical instrumentation behaviour without requiring patches. The successful passing of all AWS SDK v2 contract tests confirms that the SPI implementation correctly generates the expected spans and attributes, effectively replacing the previous patching mechanism.
Benefits
Next Steps
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.