-
Notifications
You must be signed in to change notification settings - Fork 193
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
feat: update sns span.name to reflect the current spec #1286
Conversation
'span.name' is correctly updated to 'SNS PUBLISH to <specific-name>'. 'TargetArn' is parsed and the endpoint UUID is excluded from the 'span.name'. Add support for 'PhoneNumber' (note that the actual phone number is not stored.) See https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-aws.md#sns-aws-simple-notification-service Add more test cases to increase coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the fixes.
Just one comment about the special case for TopicArn. I'd like to know whether it's needed. If it is, the spec should be updated.
// special check for format: arn:aws:sns:us-east-2:123456789012/MyTopic | ||
if slashIdx := strings.LastIndex(topicArn, "/"); slashIdx != -1 { | ||
return topicArn[slashIdx+1:] | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 TopicArn
s ending with a /
.
See https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arns-syntax
and the current tests:
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.
There was a problem hiding this comment.
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.
🌐 Coverage report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
…hmark-reporting * upstream/main: (25 commits) docs: update correct env flag for loglevel (elastic#1299) fix: readd deprecated span_frames_min_duration option as fallback for older configuration (elastic#1297) feat: rename span_frames_min_duration to span_stack_trace_min_duration (elastic#1285) test: verify Ubuntu cgroup line parsing for container ID (elastic#1293) tracer: Parse global labels per tracer (elastic#1290) feat: update sns span.name to reflect the current spec (elastic#1286) fix: expand k8s pod discovery regex (elastic#1288) test: add testcase for sqs delete_batch operation (elastic#1283) docs: document ELASTIC_APM_SERVER_CA_CERT_FILE (elastic#1289) fix: reformat code with go 1.19 to fix ci failure (elastic#1284) feat: add service target fields support to elasticsearch module (elastic#1281) fix: use the correct destination resource and name for azure queue (elastic#1282) feat: add service target fields support to azure module (elastic#1280) feat: add service target fields support to aws module (elastic#1278) feat: add service target fields support to sql module (elastic#1279) synchronize json schema specs (elastic#1260) fix: make sure at least one of the service target fields is sent (elastic#1277) docs: add link to release-notes-2.x (elastic#1271) feat: add service target fields (elastic#1274) perf: skip tracestate regex validation for es vendor key (elastic#1275) ...
'span.name' is correctly updated to 'SNS PUBLISH to '.
'TargetArn' is parsed and the endpoint UUID is excluded from the
'span.name'.
Add support for 'PhoneNumber' (note that the actual phone number is
not stored.)
See https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-aws.md#sns-aws-simple-notification-service
Add more test cases to increase coverage.
Closes #1004