Skip to content
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

feat(instrumentation-aws-sdk)!: Capture full ARN for span attribute messaging.destination.name for SNS topics #1727

Merged
merged 12 commits into from
Nov 13, 2023

Conversation

alingamn
Copy link
Contributor

@alingamn alingamn commented Oct 11, 2023

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

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 alingamn requested a review from a team October 11, 2023 19:10
@alingamn alingamn changed the title Capture full ARN for SNS topics in messaging.destination Capture full ARN for SNS topics Oct 11, 2023
@alingamn alingamn changed the title Capture full ARN for SNS topics feat(instrumentation-aws-sdk): Capture full ARN for SNS topics Oct 11, 2023
@pavankrish123
Copy link

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] =
Copy link
Contributor Author

@alingamn alingamn Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavankrish123 Semantic attribute messaging.destination.name is not yet available. This is the PR to address that open-telemetry/opentelemetry-js#4197

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #1727 (518470f) into main (9b76ffb) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           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           
Files Coverage Δ
...emetry-instrumentation-aws-sdk/src/services/sns.ts 94.11% <100.00%> (+0.36%) ⬆️

@alingamn alingamn changed the title feat(instrumentation-aws-sdk): Capture full ARN for SNS topics fix(instrumentation-aws-sdk): Capture full ARN for SNS topics Oct 12, 2023
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@alingamn
Copy link
Contributor Author

Overall LGTM, but I would really like either @blumamir or @carolabadeer to also take a look as they're the component owners.

@blumamir or @carolabadeer It would be great if one of you could take a look at this PR

@pavankrish123
Copy link

pavankrish123 commented Oct 24, 2023

@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

@pavankrish123
Copy link

cc: @srprash can please get a review on JS side as well.

@pavankrish123
Copy link

@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.

@dyladan
Copy link
Member

dyladan commented Nov 6, 2023

Using arn is previously agreed

@dyladan
Copy link
Member

dyladan commented Nov 6, 2023

Running tests and I will merge

@dyladan
Copy link
Member

dyladan commented Nov 6, 2023

This is a breaking change. Please update the pr title with the appropriate semantic commits ! to reflect that

@alingamn alingamn changed the title fix(instrumentation-aws-sdk): Capture full ARN for SNS topics fix(instrumentation-aws-sdk): Capture full ARN for span attribute messaging.destination for SNS topics Nov 6, 2023
@alingamn alingamn changed the title fix(instrumentation-aws-sdk): Capture full ARN for span attribute messaging.destination for SNS topics fix(instrumentation-aws-sdk): Capture full ARN for span attribute messaging.destination.name for SNS topics Nov 7, 2023
@alingamn
Copy link
Contributor Author

alingamn commented Nov 7, 2023

This is a breaking change. Please update the pr title with the appropriate semantic commits ! to reflect that

@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 alingamn changed the title fix(instrumentation-aws-sdk): Capture full ARN for span attribute messaging.destination.name for SNS topics feat(instrumentation-aws-sdk): Capture full ARN for span attribute messaging.destination.name for SNS topics Nov 7, 2023
@pavankrish123
Copy link

@dyladan any chance we can get this merged?

@pichlermarc pichlermarc changed the title feat(instrumentation-aws-sdk): Capture full ARN for span attribute messaging.destination.name for SNS topics feat(instrumentation-aws-sdk)!: Capture full ARN for span attribute messaging.destination.name for SNS topics Nov 10, 2023
@dyladan dyladan merged commit 28ea3b6 into open-telemetry:main Nov 13, 2023
13 of 16 checks passed
@dyladan dyladan mentioned this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Capture ARNs in SNS instrumentation
5 participants