feat(instrumentation-aws-sdk)!: Capture full ARN for span attribute messaging.destination.name for SNS topics #1727
Conversation
Problem: The use of topic names instead of ARNs in `messaging.destination` omits essential account information, making topic localization cumbersome. Solution: Adopt full ARNs as the value for `messaging.destination`, preserving both topic name and account details, aligning with current SNS ServiceExtension approach. Closes open-telemetry#1716
alingamn
changed the title
alingamn
changed the title
|
cc: @dyladan please let us know if any further chAnges are needed |
| @@ -41,7 +41,7 @@ export class SnsServiceExtension implements ServiceExtension { | |||
| MessagingDestinationKindValues.TOPIC; | |||
| const { TopicArn, TargetArn, PhoneNumber } = request.commandInput; | |||
| spanAttributes[SemanticAttributes.MESSAGING_DESTINATION] = | |||
There was a problem hiding this comment.
@pavankrish123 Semantic attribute messaging.destination.name is not yet available. This is the PR to address that open-telemetry/opentelemetry-js#4197
Codecov Report
@@ Coverage Diff @@
## main #1727 +/- ##
=======================================
Coverage 91.42% 91.42%
=======================================
Files 143 143
Lines 7301 7303 +2
Branches 1460 1461 +1
=======================================
+ Hits 6675 6677 +2
Misses 626 626
|
alingamn
changed the title
There was a problem hiding this comment.
Overall LGTM, but I would really like either @blumamir or @carolabadeer to also take a look as they're the component owners.
| @@ -41,7 +41,7 @@ export class SnsServiceExtension implements ServiceExtension { | |||
| MessagingDestinationKindValues.TOPIC; | |||
| const { TopicArn, TargetArn, PhoneNumber } = request.commandInput; | |||
| spanAttributes[SemanticAttributes.MESSAGING_DESTINATION] = | |||
There was a problem hiding this comment.
@alingamn We're having some trouble updating the semantic conventions package at the moment (they are auto-generated but already released as stable). In the meantime, please add any non-stable semantic attribute names as constants here in this package.
@blumamir or @carolabadeer It would be great if one of you could take a look at this PR |
|
@wangzlei would you be able to review it from AWS pov. We have a sister PR on Python side as well open-telemetry/opentelemetry-python-contrib#1995 |
|
cc: @srprash can please get a review on JS side as well. |
|
@pichlermarc would you be able to approve the PR now as Python side changes has been +1 ed from AWS X-Ray team? Please and thank you. |
|
Using arn is previously agreed |
|
Running tests and I will merge |
|
This is a breaking change. Please update the pr title with the appropriate semantic commits ! to reflect that |
alingamn
changed the title
alingamn
changed the title
@dyladan I have updated the code to add a new span attribute 'messaging.destination.name as constant since it's not available as in the semantic-conventions repo yet. span attribute messaging.destination.name should be used as per the semantic- convention https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/messaging.md. It is no longer a breaking change. Could you please run the tests again and Merge the PR? Thank you |
alingamn
changed the title
|
@dyladan any chance we can get this merged? |
pichlermarc
changed the title
Problem
The current approach of capturing topic names instead of ARNs in the messaging.destination attribute leads to a loss of valuable information regarding the account associated with the topic. This information is crucial for efficiently locating the topic.
Solution
This PR includes the SNS topic arn as the span attribute 'messaging.destination.name' to uniquely identify the messaging destination. Currently, 'messaging.destination.name' is added as a constant since it is not available as a semantic attribute yet. Attribute messaging.destination.name is added as per Semantic-convention https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/messaging.md
Tests
Unit tests
Closes #1716
cc: @pavankrish123 @svrnm @noltak3 @yumarg