-
Notifications
You must be signed in to change notification settings - Fork 192
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
Instrument DynamoDB calls #928
Conversation
I had to hunt around for this, and the first place I looked was the Makefile (where I would expect code generation to occur).
There are two names the bucket name can get encoded into the request URL, either as a subdomain prepended to the host, or as the first member in the path. Handle both of these, as we don't have control over which will be used. Even if explicitly set in the config to use the hosted bucket style, it might only be able to do the path style. https://github.com/aws/aws-sdk-go/blob/main/aws/config.go#L118-L127
also, add make target that will update the file if a new module is created.
seems the tests are failing to build on older versions of go
This reverts commit 6b84892.
This reverts commit 052125a.
As mentioned in the comment, after 3 attempts I wasn't able to find an S3 request that used the virtual bucket style pathing (which is supposed to be the default). I opted to test the function for now, even though it's private.
If the bucket name is all caps, it forces the path style URL structure; if the bucket name is lowercase, it will default to virtual bucket style. Test that both cases are handled.
this looks a bit like overkill now, but when we add additional aws services it will be worth it.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
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 overall, just a couple of things
This is most likely less expensive than reading out the body, unmarshaling it into a struct (the process itself already using reflection), and then attaching a new copy of the body back to the request.
Can you please add an entry to the changelog too? |
The table name is a required field in all requests, but if for some reason it is not available, report the span anyway.
thanks for all the reviews! I included the s3 update to the changelog as well, since I didn't do that in the previous PR |
Add support for tracing AWS dynamodb RPCs when using a wrapped `*session.Session` struct
Instrument DynamoDB calls according to the aws and database specs.
Happily, the
session.Session
struct only needs to be wrapped by users; the changes for instrumenting DynamoDB calls are all internal.Note on the code:
There is a moderately sized refactoring that pulls service-specific instrumentation into structs implementing an interface. The code from the S3-specific instrumentation from the previous pr has been moved into
s3.go
, while the DynamoDB-specific instrumentation code is indynamodb.go
. Some code could potentially be streamlined more, but I'm thinking it might make sense to wait for more patterns / potential abstractions to surface when implementing support for additional services.closes #880