-
Notifications
You must be signed in to change notification settings - Fork 144
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
Add AWS v2 DynamoDB instrumentation #343
Conversation
Xi Xia and I are looking at the CF work on this right now. |
da3a349
to
240e9b6
Compare
Ok, I think it was failing because we have "passesOnly" on our version for AWS v2 SDK or higher and it can apparently work with much earlier versions (even some 2.0..-preview versions) so made it expect the instrumentation verifier to pass with 2.1.0+ to go back a bit but not 2.0. |
Taking "draft" off PR. |
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.
LGTM...but someone else probably should take a look at this.
Also, lets run the AITs againts this branch before merging.
|
||
@Trace | ||
public TagResourceResponse tagResource(TagResourceRequest tagResourceRequest) { | ||
URI endpoint = clientConfiguration != null ? clientConfiguration.option(SdkClientOption.ENDPOINT) : null; |
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.
This var is not being used.
Guess this is leftover from a refactor that created the getEndpoint 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.
ah yes...thanks!
|
||
@Trace | ||
public CompletableFuture<ScanResponse> scan(ScanRequest scanRequest) { | ||
DynamoDBMetricUtil.metrics(NewRelic.getAgent().getTracedMethod(), "scan", scanRequest.tableName(), getEndpoint()); |
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 honestly don't know. @XiXiaPdx would need to help clear that up.
|
||
@Trace | ||
public CompletableFuture<ScanResponse> scan(ScanRequest scanRequest) { | ||
DynamoDBMetricUtil.metrics(NewRelic.getAgent().getTracedMethod(), "scan", scanRequest.tableName(), getEndpoint()); |
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 honestly don't know. @XiXiaPdx would need to help clear that up.
Unit tests are failing now on "compileTestScala" after removing unused variable. |
Before contributing, please read our contributing guidelines and code of conduct.
Overview
Add AWS v2 DynamoDB instrumentation to sync and async DynamoDB clients
Related Github Issue
NA
Testing
New tests added. "tag" methods not able to be tested as LocalDynamoDB for tests does not implement tags.
here,
Checks
[X] Are your contributions backwards compatible with relevant frameworks and APIs?
[ ] Does your code contain any breaking changes? Please describe.
[X] Does your code introduce any new dependencies? Please describe.