-
Notifications
You must be signed in to change notification settings - Fork 828
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.
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
aws-sdk-2.2: SNS.Publish support with experimental messaging propagator flag #8830
aws-sdk-2.2: SNS.Publish support with experimental messaging propagator flag #8830
Conversation
...-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/SnsAdviceBridge.java
Outdated
Show resolved
Hide resolved
// To add support, either raise the minimum requirement for the SNS-specific features, or use | ||
// reflection + NoMuzzle (NB: Can use SnsRequest base class, MessageAttributeValue class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that raising the minimum compileOnly version should be fine as long as we have tests that cover 2.2, and verify that this works even without PublishBatchRequest
being present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, one would only need to sprinkle a few (hopefully only one) @NoMuzzle
so that the other code is still loaded if an older version is used at runtime, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah -- @NoMuzzle
and perhaps a Class.forName()
check somewhere would probably suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I won't do this in this PR since it is self-contained follow-up work, but in case we want to do it in the future, I captured the response of a PublishBatch request using two messages with entry IDs i1 and i2 for use in the unit tests:
<PublishBatchResponse xmlns="http://sns.amazonaws.com/doc/2010-03-31/">
<PublishBatchResult>
<Failed/>
<Successful>
<member>
<MessageId>51416048-2afd-54c9-be4f-3d50f9062398</MessageId>
<Id>i1</Id>
</member>
<member>
<MessageId>1a2260b6-2e62-5192-a623-3daecd99b6c4</MessageId>
<Id>i2</Id>
</member>
</Successful>
</PublishBatchResult>
<ResponseMetadata>
<RequestId>733a6d44-b19e-5c97-8995-305c52654bd6</RequestId>
</ResponseMetadata>
</PublishBatchResponse>
(There was no example at https://docs.aws.amazon.com/sns/latest/api/API_PublishBatch.html)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I won't do this in this PR since it is self-contained follow-up work,
Definitely 💯
This PR is fine as it is, and small PRs are a delight to review -- I'd prefer the SNS batch thing to be a separate one if you still want to do that in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adapted the comment here in 722c0f3 though, to not lose this thread in case I, or anybody else wants to tackle it.
Continuing the series after #8798, this makes the experimental messaging propagator flag also apply to the SNS.Publish (but not PublishBatch) API.
This would be the last of my planned follow-up PRs for the v2 instrumentation (as outlined in #8405).