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

Add details about extracting topic name in the SNS publish API #421

Closed
wants to merge 2 commits into from

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Mar 4, 2021

Another update to the AWS services spec with details about the topic name in SNS.

@estolfo estolfo requested review from a team as code owners March 4, 2021 15:37
@apmmachine
Copy link

apmmachine commented Mar 4, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Started by timer

  • Start Time: 2021-04-26T05:46:00.778+0000

  • Duration: 3 min 56 sec

  • Commit: a219569

Trends 🧪

Image of Build Times

@@ -53,10 +53,16 @@ For distributed tracing, the SQS API has "message attributes" that can be used i
### SNS (AWS Simple Notification Service)

The AWS Simple Notification Service can be instrumented using the [messaging spec](tracing-instrumentation-messaging.md),
but the only action that is instrumented is `PUBLISH`. These specifications supersede those of the messaging spec:
but the only action that is instrumented is `PUBLISH`. These specifications supersede those of the messaging spec:

- `span.name`: The span name should follow this pattern: `SNS PUBLISH <TOPIC-NAME>`. For example,
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we maintain consistency with the messaging spec, should the name be: SNS PUBLISH to <TOPIC-NAME>?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-messaging.md#naming says the name should be <MSG-FRAMEWORK> SEND/RECEIVE/POLL to/from <QUEUE-NAME>, so should the name here be SNS SEND to <TOPIC-NAME>?

Copy link
Member

Choose a reason for hiding this comment

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

From team discussion earlier today it was decided to use SNS PUBLISH to <TOPIC-NAME> (with span.action = 'publish'). According to #446, Ruby is already doing this. Go doesn't have the " to".

Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +61 to +65
`SNS PUBLISH <PHONE_NUMBER>`. For target and topic arns that are Access Points, use the Access Point name preceded by
`accesspoint/` or `accesspoint:`. So a target/topic arn specified as
`arn:aws:s3:us-west-2:123456789012:accesspoint/myendpointslashes`, the agent extracts `accesspoint/myendpointslashes` as
the topic name. Given an Access Point such as `arn:aws:s3:us-west-2:123456789012:accesspoint:myendpointcolons`,
the agent extracts `accesspoint:myendpointcolons` as the topic name.
Copy link
Member

Choose a reason for hiding this comment

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

I'm no expert here, but I don't think accesspoints for SNS are a thing. AFAICT there are:

  1. s3 access points (https://aws.amazon.com/s3/features/access-points/) which result in ARNs with the format arn:aws:s3:region:account-id:accesspoint/access-point-name/object/resource (per https://docs.amazonaws.cn/en_us/AmazonS3/latest/userguide/using-access-points.html)
  2. EFS access points (https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html) which result in ARNs with the format ^arn:aws[-a-z]*:elasticfilesystem:[0-9a-z-:]+:access-point/fsap-[0-9a-f]{8,40}$ (per https://docs.aws.amazon.com/efs/latest/ug/API_CreateAccessPoint.html#API_CreateAccessPoint_ResponseSyntax)

Neither of these are usable with SNS Publish (https://docs.aws.amazon.com/sns/latest/api/API_Publish.html), as far as I can tell.

Does anyone have an example or a reference that shows access point ARNs are relevant for SNS Publish? If not, I think we should remove mention of them here.

Copy link
Member

@trentm trentm Jul 20, 2021

Choose a reason for hiding this comment

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

A possible alternative section for span.name, which also addresses a cardinality issue that I think exists for TargetArn usage (i.e. sending to mobile devices):

Raw spec text:

- `span.name`: ([SNS Publish API reference](https://docs.aws.amazon.com/sns/latest/api/API_Publish.html))
    - For a publish call including a `TopicArn`, the span name MUST be `SNS PUBLISH $topicName`. For example, for a TopicArn of `arn:aws:sns:us-east-2:123456789012:My-Topic` the topicName is `My-Topic`. (Implementation note: this can extracted with the equivalent of this Python expression: `topicArn.split(':').pop()`.)
    - For a publish call including a `TargetArn` (an endpoint ARN created via CreatePlatformEndpoint), the span name MUST be `SNS PUBLISH $applicationName`. For example, for a TargetArn of `arn:aws:sns:us-west-2:123456789012:endpoint/GCM/gcmpushapp/5e3e9847-3183-3f18-a7e8-671c3a57d4b3` the applicationName is `endpoint/GCM/gcmpushapp`. The endpoint UUID represents a device and mobile app. For manageable cardinality, the UUID must be excluded from the span name. (Implementation note: this can be extracted with the equivalent of this Python expression: `targetArn.split(':').pop().rsplit('/', 1)[0]`)
    - For a publish call including a `PhoneNumber`, the span name MUST be `SNS PUBLISH <PHONE_NUMBER>`. The actual phone number MUST NOT be included because it is [PII](https://en.wikipedia.org/wiki/Personal_data) and cardinality is too high. 

Rendered spec text:

  • span.name: (SNS Publish API reference)
    • For a publish call including a TopicArn, the span name MUST be SNS PUBLISH $topicName. For example, for a TopicArn of arn:aws:sns:us-east-2:123456789012:My-Topic the topicName is My-Topic. (Implementation note: this can extracted with the equivalent of this Python expression: topicArn.split(':').pop().)
    • For a publish call including a TargetArn (an endpoint ARN created via CreatePlatformEndpoint), the span name MUST be SNS PUBLISH $applicationName. For example, for a TargetArn of arn:aws:sns:us-west-2:123456789012:endpoint/GCM/gcmpushapp/5e3e9847-3183-3f18-a7e8-671c3a57d4b3 the applicationName is endpoint/GCM/gcmpushapp. The endpoint UUID represents a device and mobile app. For manageable cardinality, the UUID must be excluded from the span name. (Implementation note: this can be extracted with the equivalent of this Python expression: targetArn.split(':').pop().rsplit('/', 1)[0])
    • For a publish call including a PhoneNumber, the span name MUST be SNS PUBLISH <PHONE_NUMBER>. The actual phone number MUST NOT be included because it is PII and cardinality is too high.

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on [PhoneNumber] or [PHONENUMBER] instead of <PHONE_NUMBER>?

  • We already use [REDACTED] as a replacement value elsewhere -- i.e. with square brackets.
  • I like that PhoneNumber matches the API doc parameter name.

Copy link
Member

Choose a reason for hiding this comment

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

The cardinality issues apply to context.destination.service.resource as well.

Copy link
Member

Choose a reason for hiding this comment

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

From team discussion today, there were no objections to the proposed text above. Eyal expressed a slight preference for [PHONENUMBER] (all caps) for the placeholder string. I'll add a new PR comment here with a proposed diff.

specs/agents/tracing-instrumentation-aws.md Outdated Show resolved Hide resolved
Comment on lines 58 to +65
- `span.name`: The span name should follow this pattern: `SNS PUBLISH <TOPIC-NAME>`. For example,
`SNS PUBLISH MyTopic`.
`SNS PUBLISH MyTopic`. The publish API allows a topic to be specified as a topic arn, target arn, or phone number.
If the topic is a phone number, do not put the phone number in the span name. The span name should instead be
`SNS PUBLISH <PHONE_NUMBER>`. For target and topic arns that are Access Points, use the Access Point name preceded by
`accesspoint/` or `accesspoint:`. So a target/topic arn specified as
`arn:aws:s3:us-west-2:123456789012:accesspoint/myendpointslashes`, the agent extracts `accesspoint/myendpointslashes` as
the topic name. Given an Access Point such as `arn:aws:s3:us-west-2:123456789012:accesspoint:myendpointcolons`,
the agent extracts `accesspoint:myendpointcolons` as the topic name.
Copy link
Member

Choose a reason for hiding this comment

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

(GitHub PRs don't allow a suggested diff over a deleted line.) Suggested replacement:

- `span.name`:
    - For a publish action including a `TopicArn`, the span name MUST be `SNS PUBLISH to $topicName`. For example, for a TopicArn of `arn:aws:sns:us-east-2:123456789012:My-Topic` the topicName is `My-Topic`. (Implementation note: this can extracted with the equivalent of this Python expression: `topicArn.split(':').pop()`.)
    - For a publish action including a `TargetArn` (an endpoint ARN created via [CreatePlatformEndpoint](https://docs.aws.amazon.com/sns/latest/api/API_CreatePlatformEndpoint.html)), the span name MUST be `SNS PUBLISH to $applicationName`. For example, for a TargetArn of `arn:aws:sns:us-west-2:123456789012:endpoint/GCM/gcmpushapp/5e3e9847-3183-3f18-a7e8-671c3a57d4b3` the applicationName is `endpoint/GCM/gcmpushapp`. The endpoint UUID represents a device and mobile app. For manageable cardinality, the UUID must be excluded from the span name. (Implementation note: this can be extracted with the equivalent of this Python expression: `targetArn.split(':').pop().rsplit('/', 1)[0]`)
    - For a publish action including a `PhoneNumber`, the span name MUST be `SNS PUBLISH to [PHONENUMBER]`. The actual phone number MUST NOT be included because it is [PII](https://en.wikipedia.org/wiki/Personal_data) and cardinality is too high. 

`accesspoint/` or `accesspoint:`. So a target/topic arn specified as
`arn:aws:s3:us-west-2:123456789012:accesspoint/myendpointslashes`, the agent extracts `accesspoint/myendpointslashes` as
the topic name. Given an Access Point such as `arn:aws:s3:us-west-2:123456789012:accesspoint:myendpointcolons`,
the agent extracts `accesspoint:myendpointcolons` as the topic name.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **`span.action`**: 'publish'

@estolfo
Copy link
Contributor Author

estolfo commented Jul 29, 2021

Superseded by #478

@estolfo estolfo closed this Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants