feat(agent): Add AWS Lambda Relationship #1023
Conversation
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
agent/lib_aws_sdk_php.c
Outdated
|
|
||
| /* Compile the regex */ | ||
| if (NULL == NRPRG(aws_arn_regex)) { | ||
| NRPRG(aws_arn_regex) = nr_regex_create(AWS_ARN_REGEX, 0, 0); |
There was a problem hiding this comment.
Where/when does this get destroyed?
There was a problem hiding this comment.
Can we just precompile this in minit and destroy like wordpress does?
https://github.com/newrelic/newrelic-php-agent/blob/main/agent/fw_wordpress.c#L878-L886
There was a problem hiding this comment.
This has been moved to be compiled on first use and destroyed on mshutdown.
There was a problem hiding this comment.
Instead of first use inside an instrumented function (which would make the first instrumented call take much longer than any other call), why not have it setup either in minit or in nr_aws_sdk_php_enable if you want to wait until at least the agent knows aws-sdk is involved.
There was a problem hiding this comment.
In an effort to minimize the number of times that this regex is unnecessarily compiled, I would like to keep the compilation where it is.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1023 +/- ##
==========================================
- Coverage 77.93% 77.82% -0.12%
==========================================
Files 198 198
Lines 27713 27827 +114
==========================================
+ Hits 21599 21657 +58
- Misses 6114 6170 +56
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:
|
ZNeumann
force-pushed
the
zjn/aws-lambda
branch
from
df89224 to
0b7efec
Compare
Co-authored-by: Amber Sistla <[email protected]>
agent/lib_aws_sdk_php.c
Outdated
| } | ||
| if (nr_php_is_zval_valid_array(metadata)) { | ||
| zval* uri = nr_php_zend_hash_find(Z_ARRVAL_P(metadata), "effectiveUri"); | ||
| if (nr_php_is_zval_valid_string(uri)) { |
There was a problem hiding this comment.
Is an empty string acceptable, or should this be checking for a valid, nonempty string instead with nr_php_is_zval_non_empty_string?
There was a problem hiding this comment.
If it is empty, there is no harm in attaching it to the external params, but also no gain. added a check in b72ebc1
There was a problem hiding this comment.
instead of if (nr_php_is_zval_valid_string(uri) && !nr_strempty(Z_STRVAL_P(uri)))
Just use nr_php_is_zval_non_empty_string which does both of those things.
agent/lib_aws_sdk_php.c
Outdated
| return; | ||
| } | ||
| zval* lambda_name = nr_php_zend_hash_find(Z_ARRVAL_P(lambda_args), "FunctionName"); | ||
| if (!nr_php_is_zval_valid_string(lambda_name)) { |
There was a problem hiding this comment.
Is an empty string acceptable, or should this be checking for a valid, nonempty string instead with nr_php_is_zval_non_empty_string?
There was a problem hiding this comment.
If it is empty, the regex will fail to capture anything and the function will return exit at that point.
There was a problem hiding this comment.
True, but looks like we get to at least here, which could be skipped if we used nr_php_is_zval_non_empty_string to check for both valid and not empty string instead of just valid string.
Co-authored-by: Amber Sistla <[email protected]>
Co-authored-by: Amber Sistla <[email protected]>
agent/lib_aws_sdk_php.c
Outdated
|
|
||
| void nr_aws_sdk_lambda_client_invoke_parse_args(NR_EXECUTE_PROTO, nr_segment_cloud_attrs_t* cloud_attrs) { | ||
| zval* call_args = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); | ||
| zval* this_obj = getThis();//NR_PHP_USER_FN_THIS(); |
There was a problem hiding this comment.
Why isn't the NR_PHP_USER_FN_THIS macro used here?
There was a problem hiding this comment.
This was leftover from testing about #1023 (comment). Using NR_PHP_USER_FN_THIS() in fbf408f
| zend_class_entry* base_class = NULL; | ||
| if (NULL != execute_data->func && NULL!= execute_data->func->common.scope) { | ||
| base_class = execute_data->func->common.scope; | ||
| } | ||
| region_zval | ||
| = nr_php_get_zval_object_property_with_class(this_obj, base_class, "region"); |
There was a problem hiding this comment.
Is this construct correct? Take a look at this example. The first argument to nr_php_get_zval_object_property_with_class is the object to extract the property from, and the second argument is the class entry for the context from which the property should be extracted. What is this code trying to do?
There was a problem hiding this comment.
LambdaClient extends AwsClient. region exists on the AwsClient, not the LambdaClient. So if we wish to extract region from this (which is a LambdaClient), we must indicate the base class.
There was a problem hiding this comment.
If the intent is to look for the "region" property either in this (which is Aws\Lambda\LambdaClient) and then fall back to looking for it in this's base class (which is Aws\AwsClient), then wouldn't it be safer to get the base_class like this: base_class = nr_php_find_class("aws\\awsclient");?
There was a problem hiding this comment.
The best strategy to ensure that the agent will be able to retrieve the region is to call getRegion method on this like this: region = nr_php_call(this, "getRegion"); It doesn't rely on any assumptions but rather on publicly available API of AWS SDK for PHP.
Instrument AWS Lambda
invokeand reconstruct the ARN to facilitate a complete service map.