Skip to content
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

Linking DynamoDB queries to infra's entities #1868

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

meiao
Copy link
Contributor

@meiao meiao commented Apr 19, 2024

Overview

This PR adds the ability to specify a cloud resource id to some externals.
Having a cloud resource id in a span allows the queried infra entity to be linked to the APM entity.

Note that simply adding the cloud resource id is not enough to make the link. The type of entity must also be supported and instrumented.

To add the cloud resource id, use the new methods cloudResourceId(String) available in the builders for DatastoreParameters, MessageConsumeParameters and MessageProduceParameters.

This PR also populates this field for DynamoDB externals.

Related Github Issue

Fixes #1802

Testing

Instrumentation tests were already disabled because they were flaky in GHA. Furthermore, while trying to run the tests, an issue was found with some dependencies. So for now the instrumentation tests are on hold.

A new agent integration test was added to test the DynamoDB instrumentation.

Checks

  • Your contributions are backwards compatible with relevant frameworks and APIs.
  • Your code does not contain any breaking changes. Otherwise please describe.
  • Your code does not introduce any new dependencies. Otherwise please describe.

Setting cloud.resource_id in DynamoDB instrumentation
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 70.17544% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 70.68%. Comparing base (f1e9c8c) to head (592a01d).

Files Patch % Lines
...java/com/newrelic/agent/tracers/DefaultTracer.java 46.66% 7 Missing and 1 partial ⚠️
...om/newrelic/agent/util/AgentCollectionFactory.java 0.00% 5 Missing ⚠️
...ewrelic/agent/bridge/DefaultCollectionFactory.java 60.00% 1 Missing and 1 partial ⚠️
...n/java/com/newrelic/agent/util/AwsAccountUtil.java 90.47% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1868   +/-   ##
=========================================
  Coverage     70.68%   70.68%           
- Complexity     9877     9893   +16     
=========================================
  Files           828      829    +1     
  Lines         39848    39897   +49     
  Branches       6064     6073    +9     
=========================================
+ Hits          28167    28203   +36     
- Misses         8954     8962    +8     
- Partials       2727     2732    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jasonjkeller
jasonjkeller previously approved these changes May 1, 2024
Comment on lines +409 to +412
} else if (parameters instanceof MessageProduceParameters) {
MessageProduceParameters messageProduceParameters = (MessageProduceParameters) parameters;
setCategory(SpanCategory.generic);
setCloudResourceId(messageProduceParameters.getCloudResourceId());
Copy link
Contributor

@jasonjkeller jasonjkeller May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a bug where the cloud.resource_id attribute was never actually getting added to external spans for message producers & consumers when calling cloudResourceId(arn) on the MessageProduceParameters/MessageConsumeParameters builders with a valid ARN. I pushed up a fix and tests for that.

Technically, I think the same problem would exist if reporting an external with a HttpParameters type. This doesn’t look like it will be a problem for any of our AWS instrumentation as S3 is the only one that reports externals with HttpParameters and I don’t think we can actually build an ARN for S3 anyways. This might be a problem in the future though with other AWS SDKs or other vendors, so maybe we should also just setCloudResourceId on HttpParameters as well?

@kford-newrelic
Copy link
Contributor

Move to draft since the approach of auto account ID discovery is still under discussion (and may have to change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add span attribute support for AWS_DYNAMO_DB_TABLE
4 participants