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.

Sign up for GitHub

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
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a219569
Add details about extracting topic name in the SNS publish API
estolfo Mar 4, 2021
8d2ab9e
Update specs/agents/tracing-instrumentation-aws.md
estolfo Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions specs/agents/tracing-instrumentation-aws.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ themselves organized into regions.
- **`context.destination.service.name`**: mandatory. Use `s3`
- **`context.destination.service.resource`**: optional. The bucket name, if available. The s3 API allows either the
bucket name or an Access Point to be provided when referring to a bucket. Access Points can use either slashes or colons.
When an Access Point is provided, the access point name preceded by `accesspoint/` or `accesspoint:` should be extracted.
When an Access Point is provided, the Access Point name preceded by `accesspoint/` or `accesspoint:` should be extracted.
For example, given an Access Point such as `arn:aws:s3:us-west-2:123456789012:accesspoint/myendpointslashes`, the agent
extracts `accesspoint/myendpointslashes`. Given an Access Point such as
`arn:aws:s3:us-west-2:123456789012:accesspoint:myendpointcolons`, the agent extracts `accesspoint:myendpointcolons`.
Expand Down Expand Up @@ -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:
estolfo marked this conversation as resolved.
Show resolved Hide resolved

- `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".

`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.
Comment on lines +61 to +65
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 •
edited
Loading

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.

Comment on lines 58 to +65
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.


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'

- **`context.destination.cloud.region`**: mandatory. The AWS region where the topic is.

Expand Down