feat: update sns span.name to reflect the current spec#1286
feat: update sns span.name to reflect the current spec#1286kruskall merged 3 commits intoelastic:mainfrom
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.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
axw
left a comment
axw
left a comment
There was a problem hiding this comment.
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.
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.
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:
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.
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
|
…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) ...
2 participants
'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