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

Jump to bottom

feat: update sns span.name to reflect the current spec #1286

Merged
merged 3 commits into from
Aug 10, 2022
Merged

feat: update sns span.name to reflect the current spec #1286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62031eb
feat: update sns span.name to reflect the current spec
kruskall Aug 6, 2022
15ada35
Merge branch 'main' into feat/fix-sns-name
kruskall Aug 9, 2022
e9c5aae
Merge branch 'main' into feat/fix-sns-name
kruskall Aug 9, 2022
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
49 changes: 36 additions & 13 deletions module/apmawssdkgo/sns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func newSNS(req *request.Request) (*apmSNS, error) {

topicName := getTopicName(req)
if topicName != "" {
name += " " + topicName
name += " to " + topicName
resourceName += "/" + topicName
}

Expand Down Expand Up @@ -74,21 +74,44 @@ func (s *apmSNS) setAdditional(span *apm.Span) {
}

func getTopicName(req *request.Request) string {
// TODO: PhoneNumber is the third possibility, but I'm guessing we
// don't want to store that for customers?
arn := req.HTTPRequest.FormValue("TopicArn")
if arn == "" {
arn = req.HTTPRequest.FormValue("TargetArn")
// format: arn:aws:sns:us-east-2:123456789012:My-Topic
// should return My-Topic
if topicArn := req.HTTPRequest.FormValue("TopicArn"); topicArn != "" {
idx := strings.LastIndex(topicArn, ":")
if idx == -1 {
return ""
}

// special check for format: arn:aws:sns:us-east-2:123456789012/MyTopic
if slashIdx := strings.LastIndex(topicArn, "/"); slashIdx != -1 {
return topicArn[slashIdx+1:]
}
Comment on lines +85 to +88
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? We were previously handling topic and target ARNs with the same code, hence the special case. Now that they're handled independently, is it still required?

Copy link
Member Author

@kruskall kruskall Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is necessary, this special case should be for TopicArns ending with a /.

See https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arns-syntax

and the current tests:

apm-agent-go/module/apmawssdkgo/sns_test.go

Line 56 in 0f2b9ce

TopicArn: aws.String("arn:aws:sns:us-east-2:123456789012/myTopic"),

The spec seems to provide an example with a topic ARN ending with : but does not mention any restriction on the ARN format.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I suppose it's fine to keep it. The docs I've seen suggest it's always ":" for topic ARNs, but it's never explicitly called out.


return topicArn[idx+1:]
}

// SNS ARN can be in the following formats:
// - arn:aws:sns:us-east-2:123456789012:MyTopic
// - arn:aws:sns:us-east-2:123456789012/MyTopic
parts := strings.Split(arn, "/")
if len(parts) == 1 {
parts = strings.Split(arn, ":")
// format: arn:aws:sns:us-west-2:123456789012:endpoint/GCM/gcmpushapp/5e3e9847-3183-3f18-a7e8-671c3a57d4b3
// should return endpoint/GCM/gcmpushapp
if targetArn := req.HTTPRequest.FormValue("TargetArn"); targetArn != "" {
kruskall marked this conversation as resolved.
Show resolved Hide resolved
idx := strings.LastIndex(targetArn, ":")
if idx == -1 {
return ""
}

endIdx := strings.LastIndex(targetArn, "/")
if endIdx == -1 {
return ""
}

return targetArn[idx+1 : endIdx]
}
return parts[len(parts)-1]

// The actual phone number MUST NOT be included because it is PII and cardinality is too high.
if phoneNumber := req.HTTPRequest.FormValue("PhoneNumber"); phoneNumber != "" {
return "[PHONENUMBER]"
}

return ""
}

// addMessageAttributesSNS adds message attributes to `Publish` RPC calls.
Expand Down
30 changes: 28 additions & 2 deletions module/apmawssdkgo/sns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestSNS(t *testing.T) {
ignored, hasTraceContext, hasError bool
}{
{
name: "SNS PUBLISH myTopic",
name: "SNS PUBLISH to myTopic",
action: "publish",
resource: "sns/myTopic",
topicName: "myTopic",
Expand All @@ -58,7 +58,7 @@ func TestSNS(t *testing.T) {
},
},
{
name: "SNS PUBLISH myTopic",
name: "SNS PUBLISH to myTopic",
action: "publish",
resource: "sns/myTopic",
topicName: "myTopic",
Expand All @@ -70,6 +70,32 @@ func TestSNS(t *testing.T) {
})
},
},
{
name: "SNS PUBLISH to endpoint/GCM/gcmpushapp",
action: "publish",
resource: "sns/endpoint/GCM/gcmpushapp",
topicName: "endpoint/GCM/gcmpushapp",
hasTraceContext: true,
fn: func(ctx context.Context, svc *sns.SNS) {
svc.PublishWithContext(ctx, &sns.PublishInput{
Message: aws.String("my message"),
TargetArn: aws.String("arn:aws:sns:us-west-2:123456789012:endpoint/GCM/gcmpushapp/5e3e9847-3183-3f18-a7e8-671c3a57d4b3"),
})
},
},
{
name: "SNS PUBLISH to [PHONENUMBER]",
action: "publish",
resource: "sns/[PHONENUMBER]",
topicName: "[PHONENUMBER]",
hasTraceContext: true,
fn: func(ctx context.Context, svc *sns.SNS) {
svc.PublishWithContext(ctx, &sns.PublishInput{
Message: aws.String("my message"),
PhoneNumber: aws.String("1"),
})
},
},
{
ignored: true,
fn: func(ctx context.Context, svc *sns.SNS) {
Expand Down