aws-sdk-2.2: SNS.Publish support with experimental messaging propagator flag #8830
Conversation
Oberon00
changed the title
...-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.
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.
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.
Yeah -- @NoMuzzle and perhaps a Class.forName() check somewhere would probably suffice.
There was a problem hiding this comment.
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.
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.
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).