Fix messaging.system attr for AWS SNS#8322
Fix messaging.system attr for AWS SNS#8322austindrenski wants to merge 1 commit intoopen-telemetry:mainfrom
messaging.system attr for AWS SNS#8322Conversation
No idea why the semconv committee decided to go with `aws_sqs` and `aws.sns`, but either the semconv is wrong or the instrumentation is, so figured I'd start with a PR here and see what others think.
There was a problem hiding this comment.
Pull request overview
This PR corrects the messaging.system attribute value for AWS SNS to align with OpenTelemetry semantic conventions. The fix changes from using a hardcoded string "aws_sns" to using the predefined semconv constant MessagingSystemAWSSNS, which uses the correct value "aws.sns" (with a dot instead of underscore) as specified in the OTel documentation.
Key Changes:
- Updated SNS attribute builder to use the semantic convention constant instead of hardcoded string
- Updated all three test cases to verify the correct constant usage
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/snsattributes.go |
Updated SNSAttributeBuilder to use semconv.MessagingSystemAWSSNS constant instead of manually constructing the attribute with the "aws_sns" string |
instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/snsattributes_test.go |
Updated three test cases (TestPublishInput, TestPublishInputWithNoDestination, TestPublishBatchInput) to assert against the correct semconv constant |
The code changes look correct and complete. The PR properly addresses the semantic convention mismatch by using the predefined constant from the semconv package (v1.37.0), which ensures the correct value "aws.sns" is used instead of "aws_sns". This brings the SNS instrumentation in line with the OTel specifications and is consistent with how SQS is already implemented using semconv.MessagingSystemAWSSQS. No issues found.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
See open-telemetry/semantic-conventions#2668 for explanation on the naming. |
|
Thank you for your contribution. Currently, there are 2 issues that need attention:
@open-telemetry/go-approvers I'm not quite sure if we are currently allowed to accept such changes. |
No idea why the semconv committee decided to go withedit: see #8322 (comment) for context on this^ discrepancy, as well as plans to eventually renameaws_sqsandaws.sns, but either the semconv is wrong or the instrumentation is, so figured I'd start with a PR here and see what others think.aws{_ => .}sqs.From the OTel semantic conventions docs for
messaging.system:Related