-
Notifications
You must be signed in to change notification settings - Fork 416
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
contrib/aws: add aws-sdk-go-v2 support package #923
Conversation
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.
@speza Thank you for this.
I have a few nitpicks about the code.
Introduce tracing contrib for the aws-sdk-go-v2. This uses the new aws-sdk-go-v2 middleware to hook into the lifecycle of the request built and sent to AWS. Fixes DataDog#869
2dd6a84
to
b23c3e5
Compare
@knusbaum I have made the changes. I think the test is failing due to a tagging issue for release? |
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 code looks good now.
If you could add a couple more tests to improve the test coverage (https://app.codecov.io/gh/DataDog/dd-trace-go/compare/923/diff) I think we'll be good.
It might not be feasible to get to 100% coverage, but we would like it as high as we can reasonably get.
* More coverage in core middleware * Removed the use of SpanFromContext bool as we will always have a span if the middleware has been added. Otherwise, using a no-op is ok anyway. This simplifies the logic. * More coverage on options. Fixes DataDog#869
@knusbaum I think this should be good now? The only line not tested is for the environment variable options. But this seems to be the same throughout all the contrib package. |
} | ||
|
||
return out, metadata, err | ||
}), middleware.Before) |
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.
Changed this to Before
. Actually missed test coverage on GetRequestIDMetadata and there was a bug. The AWS middleware for setting the request ID was happening after our middleware.
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 great! Glad we caught that bug.
Thanks @knusbaum. Not sure why its failing on codecov/patch... |
Introduce tracing contrib for the aws-sdk-go-v2. This uses the new aws-sdk-go-v2 middleware to hook into the lifecycle of the request built and sent to AWS.
Fixes #869