-
Notifications
You must be signed in to change notification settings - Fork 373
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
Consistent APM Span tags for AWS SDK Requests 2 #3159
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3159 +/- ##
=======================================
Coverage 98.17% 98.17%
=======================================
Files 1284 1284
Lines 73911 73914 +3
Branches 3418 3418
=======================================
+ Hits 72565 72568 +3
Misses 1346 1346
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
state_machine_name ||= parts[6] | ||
state_machine_account_id = parts[4] | ||
end | ||
|
||
if execution_arn |
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.
The order of this seemed to flow better as the state_machine_arn is easier to understand
state_machine_name = parts[6] | ||
state_machine_account_id = parts[4] |
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.
These two fields are being overwritten here. Is it intentional that execution_arn
overrides state_machine_arn
?
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.
My reasoning was in a situation where execution arn and statemachine arn are present on the same request but after looking at https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/States/Client.html it seems that wont happen, its either or. I can replace logic with an if and else if so it wont get replaced
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.
Makes sense.
You can leave the code as is.
Can you add a comment explaining what you just mentioned above? ^ So in the future we can be confident when making changes to this method?
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.
comment might be unecessary now as I added .nil? to only set if it is nil
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.
and im not entirely confident with #3159 (comment)
…DataDog/dd-trace-rb into ar/ConsistentApmSpanTagsForAwsRequests2
closing for now as this is a stretch goal, will revisit if it becomes a priority helper1: adds state_machine_arn tag and extracts/returns other info for caller to tag |
What does this PR do?
Adds
TAG_STATE_MACHINE_ARN = 'statemachinearn'
TAG_STATE_EXECUTION_ARN = 'execution_arn'
to respective aws sdk request spans
to be in sync with JS and PY, search for the doc name above in cloud search or request link
Motivation:
SVLS-3508
Google docs
Downstream Related Resources
Additional Notes:
How to test the change?
Unit tests
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!