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

Capture ARNs in SNS instrumentation #1716

Closed
alingamn opened this issue Oct 5, 2023 · 8 comments · Fixed by #1727
Closed

Capture ARNs in SNS instrumentation #1716

alingamn opened this issue Oct 5, 2023 · 8 comments · Fixed by #1727

Comments

@alingamn
Copy link
Contributor

alingamn commented Oct 5, 2023

Is your feature request related to a 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.

Describe the solution you'd like
To address this issue, we propose utilizing ARNs as the value of the messaging.destination attribute to capture the full ARN of the topic. This change aligns with an existing implementation in the ServiceExtension for SNS.

Alternate Solution:
Instead of modifying the existing attribute, account information related to the topic can be included as an additional span attribute. However, it's important to note that there are currently no defined semantics for specifying the value.
Suggested attribute: aws.account.id:12343441

cc: @svrnm @pavankrish123 @noltak3 @yumarg

@alingamn alingamn changed the title Capture ARNs as additional attribute in SNS instrumentation Capture ARNs in SNS instrumentation Oct 5, 2023
@pavankrish123
Copy link

pavankrish123 commented Oct 6, 2023

Thanks @alingamn -

Also it is worth noting java sdk is capturing the http.url of the sns - example

InstrumentationScope io.opentelemetry.aws-sdk-2.2 1.26.0-alpha
Span #0
    Trace ID       : 3d8163b121c5f5739151a7c329cc53a3
    Parent ID      : 0ed878bc97089ad5
    ID             : 9551240a03d76de6
    Name           : Sns.Publish
    Kind           : Client
    Start time     : 2023-10-05 21:12:31.096381951 +0000 UTC
    End time       : 2023-10-05 21:12:33.237732883 +0000 UTC
    Status code    : Unset
    Status message :
Attributes:

     -> http.url: Str(https://sns.us-west-2.amazonaws.com?Action=Publish&Version=2010-03-31&TopicArn=arn%3Aaws%3Asns%3Aus-west-2%3A71******A&test-topic&Message=my-message-from-lambda1)
     -> thread.id: Int(1)
     -> http.request_content_length: Int(141)
     -> rpc.service: Str(Sns)
     -> http.response_content_length: Int(294)

Note that, I am not proposing to use http.url here as this nuance coming from amazon sdk apis depending on the language. In node and python i don't think url is used, rather topicARN is used to publish to SNS. But I am giving this example to emphasize the need to acpture account.id and/or arn to "uniquely" identify the Topic.

@dyladan
Copy link
Member

dyladan commented Oct 6, 2023

I don't see specifically which instrumentation you're asking about, but it seems most likely to be the aws-sdk instrumentation?

This sounds like a change request most applicable to the OpenTelemetry Semantic Conventions. This repository simply follows the recommendations of the Semantic Conventions and the Specification. If the existing Semantic Convention would be sufficient for your use case, but we are not following the Semantic Convention, please update this issue to reflect that or open a new issue to update the instrumentation following Semantic Conventions.

@alingamn
Copy link
Contributor Author

alingamn commented Oct 6, 2023

Hi @dyladan

yes, this is related to aws-sdk instrumentation. We are setting the span attribute to just the topic name here which doesn't provide uniqueness since multiple accounts can have an identical topic. The request here is to either capture the entire ARN as part of the existing attribute(messaging.destination) or add an additional attribute to identify the account.

@dyladan
Copy link
Member

dyladan commented Oct 6, 2023

The request here is to either capture the entire ARN as part of the existing attribute(messaging.destination) or add an additional attribute to identify the account.

Is that what is recommended by the semantic conventions? I'm not familiar with the messaging semconv but you can find it here: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md

@pavankrish123
Copy link

Thanks @dyladan - This has to do more with the value we are setting for messaging.destination that using the messaging.destination - The current value (TopicName) cannot uniquely identify the topic with out account and region. However something like arn of the topic can. It is also worth noting that topicARN also has the name information.

please let us know if it is acceptable to use arn instead of name of the topic cc: @alingamn

@Flarna
Copy link
Member

Flarna commented Oct 10, 2023

The semconv says it should be unique. If it is not unique the current implementation seems be not correct.

@dyladan
Copy link
Member

dyladan commented Oct 11, 2023

In that case then yes I would accept a PR

@pavankrish123
Copy link

pavankrish123 commented Oct 11, 2023

Thanks @dyladan @Flarna . We have also followed up with this query in semconventions slack thread and they have confirmed the same. ARNs are technically names, (- longer names). @alingamn please open the PRs - letme know if you need any help.

alingamn added a commit to alingamn/opentelemetry-js-contrib that referenced this issue Oct 11, 2023
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
dyladan pushed a commit that referenced this issue Nov 13, 2023
…essaging.destination.name for SNS topics (#1727)

* Capture full ARN for SNS topics in messaging.destination

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 #1716

* Add new span attribute instead of modifying the existing attribute

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants