-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat(agent): Add AWS DynamoDb Instrumentation #1037
base: dev
Are you sure you want to change the base?
Conversation
* add db.system * modify datastore to allow for datastore that don't have concept of database name
* Added instrumentation to handle the calls specified in the agent spec * Added unit tests
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1037 +/- ##
==========================================
- Coverage 77.87% 77.79% -0.09%
==========================================
Files 198 198
Lines 27899 27991 +92
==========================================
+ Hits 21727 21775 +48
- Misses 6172 6216 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ewrelic-php-agent into feat/dynamodb_instrumentation
Co-authored-by: bduranleau-nr <[email protected]>
Co-authored-by: bduranleau-nr <[email protected]>
Co-authored-by: bduranleau-nr <[email protected]>
Co-authored-by: bduranleau-nr <[email protected]>
if (NULL != datastore_params->collection && NULL != account_id | ||
&& NULL != cloud_attrs->cloud_region) { | ||
/* Must be freed by caller */ | ||
cloud_attrs->cloud_resource_id = nr_formatf( |
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.
What would be the best behavior here if cloud_attrs->cloud_resource_id
was non-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.
Best behavior? if non-NULL it gets passed back to caller who uses it to end the segment before freeing it.
#define AWS_LAMBDA_ARN_REGEX \ | ||
"(arn:(aws[a-zA-Z-]*)?:lambda:)?" \ | ||
"((?<region>[a-z]{2}((-gov)|(-iso([a-z]?)))?-[a-z]+-\\d{1}):)?" \ | ||
"((?<accountId>\\d{12}):)?" \ | ||
"(function:)?" \ | ||
"(?<functionName>[a-zA-Z0-9-\\.]+)" \ | ||
"(:(?<qualifier>\\$LATEST|[a-zA-Z0-9-]+))?" |
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.
Format only.
} else { | ||
/* In case where host was found but port was not, spec says return | ||
* unknown for port. */ | ||
datastore_params->instance->port_path_or_id = nr_strdup("unknown"); |
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.
Would this just be the default port if unset?
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.
No because we were able to extract a host just not a port. Once we've extracted a host, we cannot assume a port to go with it. Default port only ever pairs with default host.
nr_segment_cloud_attrs_t cloud_attrs = { | ||
.cloud_platform = "aws_lambda" | ||
}; | ||
nr_segment_cloud_attrs_t cloud_attrs = {.cloud_platform = "aws_lambda"}; |
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.
Format only
} | ||
nr_segment_external_end(&external_segment, &external_params); | ||
nr_free(cloud_attrs.cloud_resource_id); | ||
} | ||
|
||
/* This stores the compiled regex to parse AWS ARNs. The compilation happens when | ||
* it is first needed and is destroyed in mshutdown | ||
/* This stores the compiled regex to parse AWS ARNs. The compilation happens |
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.
format only
void nr_aws_sdk_lambda_client_invoke_parse_args( | ||
NR_EXECUTE_PROTO, | ||
nr_segment_cloud_attrs_t* cloud_attrs) { |
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.
format only
zval* lambda_name | ||
= nr_php_zend_hash_find(Z_ARRVAL_P(lambda_args), "FunctionName"); |
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.
format only
nr_regex_substrings_t* matches = nr_regex_match_capture( | ||
aws_arn_regex, Z_STRVAL_P(lambda_name), Z_STRLEN_P(lambda_name)); |
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.
format only
if (NULL != execute_data->func | ||
&& NULL != execute_data->func->common.scope) { | ||
base_class = execute_data->func->common.scope; |
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.
format only
arn = nr_formatf("arn:aws:lambda:%s:%s:function:%s:%s", region, accountID, | ||
function_name, qualifier); | ||
} else { | ||
arn = nr_formatf("arn:aws:lambda:%s:%s:function:%s", | ||
region, accountID, function_name); | ||
arn = nr_formatf("arn:aws:lambda:%s:%s:function:%s", region, accountID, | ||
function_name); |
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.
format only
Added functionality to instrument
Unit tests added.
See also relevant multiverse PR.
Needed a modification to axiom to: