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

feat(agent): Add AWS Lambda Relationship #1023

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 172 additions & 0 deletions agent/lib_aws_sdk_php.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,16 @@
#include "fw_support.h"
#include "util_logging.h"
#include "nr_segment_message.h"
#include "nr_segment_external.h"
#include "lib_aws_sdk_php.h"

#define PHP_PACKAGE_NAME "aws/aws-sdk-php"
#define AWS_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-]+))?"

#if ZEND_MODULE_API_NO >= ZEND_8_1_X_API_NO /* PHP8.1+ */
/* Service instrumentation only supported above PHP 8.1+*/
Expand Down Expand Up @@ -295,6 +302,166 @@
cloud_attrs->cloud_region = region;
}

void nr_lib_aws_sdk_php_lambda_handle(nr_segment_t* auto_segment,
char* command_name_string,
size_t command_name_len,
NR_EXECUTE_PROTO) {
nr_segment_t* external_segment = NULL;
zval** retval_ptr = NR_GET_RETURN_VALUE_PTR;

nr_segment_cloud_attrs_t cloud_attrs = {
.cloud_platform = "aws_lambda"
};

if (NULL == auto_segment) {
return;
}

if (NULL == command_name_string || 0 == command_name_len) {
return;
}

if (NULL == retval_ptr) {

Check failure

Code scanning / CodeQL

Redundant null check due to previous dereference High

This null check is redundant because
the value is dereferenced
in any case.

Copilot Autofix AI 2 days ago

To fix the problem, we need to move the null check for retval_ptr before its dereference. This will ensure that we do not dereference a null pointer, which could lead to undefined behavior or a crash. Specifically, we should move the null check from line 324 to before line 364 where retval_ptr is first dereferenced.

Suggested changeset 1
agent/lib_aws_sdk_php.c

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/agent/lib_aws_sdk_php.c b/agent/lib_aws_sdk_php.c
--- a/agent/lib_aws_sdk_php.c
+++ b/agent/lib_aws_sdk_php.c
@@ -323,7 +323,2 @@
 
-  if (NULL == retval_ptr) {
-    /* Do not instrument when an exception has happened */
-    return;
-  }
-
 #define AWS_COMMAND_IS(CMD) \
@@ -360,2 +355,7 @@
 
+  if (NULL == retval_ptr) {
+    /* Do not instrument when an exception has happened */
+    return;
+  }
+
   /* end the segment */
EOF
@@ -323,7 +323,2 @@

if (NULL == retval_ptr) {
/* Do not instrument when an exception has happened */
return;
}

#define AWS_COMMAND_IS(CMD) \
@@ -360,2 +355,7 @@

if (NULL == retval_ptr) {
/* Do not instrument when an exception has happened */
return;
}

/* end the segment */
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
/* Do not instrument when an exception has happened */
return;
}

#define AWS_COMMAND_IS(CMD) \
(command_name_len == (sizeof(CMD) - 1) && nr_streq(CMD, command_name_string))

/* Determine if we instrument this command. */
if (AWS_COMMAND_IS("invoke")) {
// Command can be saved here if in the future
// we instrument more than 1 lambda command
} else {
return;
}
#undef AWS_COMMAND_IS

/* reconstruct the ARN */
nr_aws_lambda_invoke(NR_EXECUTE_ORIG_ARGS, &cloud_attrs);
if (!cloud_attrs.cloud_resource_id) {
/* we do not want to instrument if we cannot reconstruct the ARN */
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's still fine to instrument in this case.
It won't create the relationship, but the external segment can still be created. The cloud attributes are extra attributes, not core external segment attributes.

}

/*
* By this point, it's been determined that this call will be instrumented so
* only create the segment now, grab the parent segment start time, add our
* special segment attributes/metrics then close the newly created segment.
*/
external_segment = nr_segment_start(NRPRG(txn), NULL, NULL);
if (NULL == external_segment) {
return;
}
/* re-use start time from auto_segment started in func_begin */
external_segment->start_time = auto_segment->start_time;
cloud_attrs.aws_operation = command_name_string;

/* end the segment */
nr_segment_traces_add_cloud_attributes(external_segment, &cloud_attrs);
nr_segment_external_params_t external_params = {.library = "aws_sdk"};
zval* data = nr_php_get_zval_object_property(*retval_ptr, "data");
if (NULL != data && IS_ARRAY == Z_TYPE_P(data)) {
zval* status_code = nr_php_zend_hash_find(Z_ARRVAL_P(data), "StatusCode");
if (NULL != status_code && IS_LONG == Z_TYPE_P(status_code)) {
external_params.status = Z_LVAL_P(status_code);
}
zval* metadata = nr_php_zend_hash_find(Z_ARRVAL_P(data), "@metadata");
if (NULL != metadata && IS_REFERENCE == Z_TYPE_P(metadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could simplify this a bit by something like just adding this check:

  if (IS_REFERENCE == Z_TYPE_P(metadata)) {
    metadata = Z_REFVAL_P(metadata);
  }

Then the code could look something like:

if (NULL != metadata && IS_REFERENCE == Z_TYPE_P(metadata)) {
    metadata = Z_REFVAL_P(metadata);
}
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_non_empty_string(uri)) {
        external_params.uri = Z_STRVAL_P(uri);
      }
    }

&& IS_ARRAY == Z_TYPE_P(Z_REFVAL_P(metadata))) {
zval* uri = nr_php_zend_hash_find(Z_ARRVAL_P(Z_REFVAL_P(metadata)), "effectiveUri");
if (NULL != uri && IS_STRING == Z_TYPE_P(uri)) {
external_params.uri = Z_STRVAL_P(uri);
}
}
}
nr_segment_external_end(&auto_segment, &external_params);
}

void nr_aws_lambda_invoke(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 = NR_PHP_USER_FN_THIS();
char* arn = NULL;
char* function_name = NULL;
char* region = NULL;
zval* region_zval = NULL;
char* qualifier = NULL;
zval* qualifier_zval = NULL;
char* accountID = NULL;

/* verify arguments */
if (NULL == call_args || IS_ARRAY != Z_TYPE_P(call_args)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

General comment, there are a lot of helper functions for zval here: https://github.com/newrelic/newrelic-php-agent/blob/main/agent/php_zval.h
of the form nr_php_is_zval_* that will already do the NULL and type checking for you for most zval types.

return;
}
zval* lambda_args = nr_php_zend_hash_index_find(Z_ARRVAL_P(call_args), 0);
if (NULL == lambda_args || IS_ARRAY != Z_TYPE_P(lambda_args)) {
return;
}
zval* lambda_name = nr_php_zend_hash_find(Z_ARRVAL_P(lambda_args), "FunctionName");
if (NULL == lambda_name || IS_STRING != Z_TYPE_P(lambda_name)) {
return;
}
Comment on lines +393 to +404
Copy link
Contributor

@zsistla zsistla Feb 13, 2025

Choose a reason for hiding this comment

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

This function will already do all the checking for you and if 'FunctionName' has a valid string value, it will return the value to you .
https://github.com/newrelic/newrelic-php-agent/blob/main/agent/lib_aws_sdk_php.h#L78


/* Compile the regex */
if (NULL == NRPRG(aws_arn_regex)) {
NRPRG(aws_arn_regex) = nr_regex_create(AWS_ARN_REGEX, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where/when does this get destroyed?

Copy link
Contributor

@zsistla zsistla Feb 13, 2025

Choose a reason for hiding this comment

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

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

}

/* Extract all information possible from the passed lambda name via regex */
nr_regex_substrings_t* matches =
nr_regex_match_capture(NRPRG(aws_arn_regex),
Z_STRVAL_P(lambda_name),
Z_STRLEN_P(lambda_name));
function_name = nr_regex_substrings_get_named(matches, "functionName");
accountID = nr_regex_substrings_get_named(matches, "accountId");
region = nr_regex_substrings_get_named(matches, "region");
qualifier = nr_regex_substrings_get_named(matches, "qualifier");

/* suppliment missing information with API calls */
if (nr_strempty(function_name)) {
/*
* Cannot get the needed data. Function name is required in the
* argument, so this won't happen in normal operation
*/
nr_regex_substrings_destroy(&matches);
return;
}
if (nr_strempty(accountID)) {
accountID = NRINI(aws_account_id);
}
if (nr_strempty(region)) {
region_zval = nr_php_call(this_obj, "getRegion");
Copy link
Contributor

@zsistla zsistla Feb 13, 2025

Choose a reason for hiding this comment

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

Since the region property exists on the object, instead of doing an nr_php_call here (which has higher overhead), you could use something like:

  region_zval
      = nr_php_get_zval_object_property(this_obj, "region");
  if (!nr_php_is_zval_non_empty_string(region_zval)) {
 ... 
}

if (nr_php_is_zval_valid_string(region_zval)) {
region = Z_STRVAL_P(region_zval);
}
}
if (nr_strempty(qualifier)) {
qualifier_zval = NULL;//nr_php_call(this_obj, "getQualifier");
if (nr_php_is_zval_valid_string(qualifier_zval)) {
qualifier = Z_STRVAL_P(qualifier_zval);
}
}

if (!nr_strempty(accountID) && !nr_strempty(region)) {
// construct the ARN
if (qualifier) {
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);
}

// Attatch the ARN
cloud_attrs->cloud_resource_id = arn;
}

nr_regex_substrings_destroy(&matches);
nr_php_zval_free(&region_zval);
nr_php_zval_free(&qualifier_zval);
}

char* nr_lib_aws_sdk_php_get_command_arg_value(char* command_arg_name,
NR_EXECUTE_PROTO) {
zval* param_array = NULL;
Expand Down Expand Up @@ -383,6 +550,10 @@
nr_lib_aws_sdk_php_sqs_handle(auto_segment, command_name_string,
Z_STRLEN_P(command_name),
NR_EXECUTE_ORIG_ARGS);
} else if (AWS_CLASS_IS("Aws\\Lambda\\LambdaClient", "LambdaClient")) {
nr_lib_aws_sdk_php_lambda_handle(auto_segment, command_name_string,
Z_STRLEN_P(command_name),
NR_EXECUTE_ORIG_ARGS);
}

#undef AWS_CLASS_IS
Expand Down Expand Up @@ -560,5 +731,6 @@
nr_php_wrap_user_function_before_after_clean(
NR_PSTR("Aws\\AwsClient::__call"), NULL, nr_aws_client_call,
nr_aws_client_call);

#endif
}
12 changes: 12 additions & 0 deletions agent/lib_aws_sdk_php.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,18 @@ extern void nr_lib_aws_sdk_php_sqs_handle(nr_segment_t* segment,
size_t command_name_len,
NR_EXECUTE_PROTO);

/*
* Purpose : Handle when a LambdaClient::invoke command happens
*
* Params : 1. NR_EXECUTE_ORIG_ARGS (execute_data, func_return_value)
* 2. cloud_attrs : the cloud attributes pointer to be
* populated with the ARN
*
* Returns :
*
*/
void nr_aws_lambda_invoke(NR_EXECUTE_PROTO, nr_segment_cloud_attrs_t* cloud_attrs);

/*
* Purpose : The second argument to the Aws/AwsClient::__call function should be
* an array, the first element of which is itself an array of arguments that
Expand Down
9 changes: 8 additions & 1 deletion agent/php_newrelic.h
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,6 @@ bool wordpress_core; /* set based on
nrinistr_t
wordpress_hooks_skip_filename; /* newrelic.framework.wordpress.hooks_skip_filename
*/

nrinibool_t
analytics_events_enabled; /* DEPRECATED newrelic.analytics_events.enabled */
nrinibool_t
Expand Down Expand Up @@ -383,6 +382,11 @@ nrinibool_t
nrinibool_t
database_name_reporting_enabled; /* newrelic.datastore_tracer.database_name_reporting.enabled
*/
/*
* Cloud relationship settings
*/
nrinistr_t
aws_account_id; /* newrelic.cloud.aws.account_id */

/*
* Deprecated settings that control request parameter capture.
Expand Down Expand Up @@ -464,6 +468,7 @@ nr_stack_t wordpress_tag_states; /* stack of bools indicating
bool check_cufa; /* Whether we need to check cufa because we are
instrumenting hooks, or whether we can skip cufa */
char* wordpress_tag; /* The current WordPress tag */

#endif //OAPI

nr_matcher_t* wordpress_plugin_matcher; /* Matcher for plugin filenames */
Expand All @@ -481,6 +486,8 @@ int php_cur_stack_depth; /* Total current depth of PHP stack, measured in PHP

nrphpcufafn_t
cufa_callback; /* The current call_user_func_array callback, if any */

nr_regex_t* aws_arn_regex; /* The compiled regex to search for ARNs */
/*
* We instrument database connection constructors and store the instance
* information in a hash keyed by a string containing the connection resource
Expand Down
12 changes: 12 additions & 0 deletions agent/php_nrini.c
Original file line number Diff line number Diff line change
Expand Up @@ -3100,6 +3100,18 @@ STD_PHP_INI_ENTRY_EX("newrelic.vulnerability_management.composer_api.enabled",
newrelic_globals,
nr_enabled_disabled_dh)

/*
* Cloud relationship settings
*/
STD_PHP_INI_ENTRY_EX("newrelic.cloud.aws.account_id",
"1",
NR_PHP_REQUEST,
nr_string_mh,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this call a function that verifies the string is always 12 chars long (according to the spec).

aws_account_id,
zend_newrelic_globals,
newrelic_globals,
0)

/*
* Messaging API
*/
Expand Down
9 changes: 9 additions & 0 deletions agent/scripts/newrelic.ini.template
Original file line number Diff line number Diff line change
Expand Up @@ -1352,3 +1352,12 @@ newrelic.daemon.logfile = "/var/log/newrelic/newrelic-daemon.log"
; newrelic.span_events.attributes.include/exclude
;
;newrelic.message_tracer.segment_parameters.enabled = true

; Setting: newrelic.cloud.aws.account_id
; Type : string
; Scope : per-directory
; Default: none
; Info : AWS account id that is used to map entities called
; via the AWS SDK.
;
Comment on lines +1360 to +1362
Copy link
Contributor

Choose a reason for hiding this comment

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

;newrelic.cloud.aws.account_id = ""
37 changes: 37 additions & 0 deletions agent/tests/test_lib_aws_sdk_php.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,19 @@ NR_PHP_WRAPPER(expect_arg_value_null) {
}
NR_PHP_WRAPPER_END

NR_PHP_WRAPPER(aws_lambda_invoke_wrapper) {
nr_segment_cloud_attrs_t cloud_attrs = {0};
zval* expected = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS);
nr_aws_lambda_invoke(NR_EXECUTE_ORIG_ARGS, &cloud_attrs);
(void)wraprec;

tlib_pass_if_str_equal("Expected should match reconstructed arn",
Z_STRVAL_P(expected),
cloud_attrs.cloud_resource_id);
NR_PHP_WRAPPER_CALL;
}
NR_PHP_WRAPPER_END

static void test_nr_lib_aws_sdk_php_get_command_arg_value() {
zval* expr = NULL;
zval* first_arg = NULL;
Expand Down Expand Up @@ -458,6 +471,29 @@ static void test_nr_lib_aws_sdk_php_handle_version(void) {
tlib_php_request_end();
}

static void test_nr_lib_aws_sdk_php_lambda_invoke() {
tlib_php_engine_create("");
tlib_php_request_start();

tlib_php_request_eval("function lambda_invoke($a, $b) { return; }");
nr_php_wrap_user_function(NR_PSTR("lambda_invoke"), aws_lambda_invoke_wrapper);

char* args
= "array("
" 0 => array("
" 'FunctionName' => 'us-east-2:012345678901:function'"
" )"
")";
zval* array_arg = tlib_php_request_eval_expr(args);
char* expect = "'arn:aws:lamba:us-east-2:012345678901:function'";
zval* expect_arg = tlib_php_request_eval_expr(expect);
zval* expr = nr_php_call(NULL, "lambda_invoke", expect_arg, array_arg);
tlib_pass_if_not_null("Expression should evaluate.", expr);

tlib_php_request_end();
tlib_php_engine_destroy();
}

void test_main(void* p NRUNUSED) {
tlib_php_engine_create("");
test_nr_lib_aws_sdk_php_add_supportability_service_metric();
Expand All @@ -466,6 +502,7 @@ void test_main(void* p NRUNUSED) {
#if ZEND_MODULE_API_NO >= ZEND_8_1_X_API_NO
test_nr_lib_aws_sdk_php_sqs_parse_queueurl();
test_nr_lib_aws_sdk_php_get_command_arg_value();
test_nr_lib_aws_sdk_php_lambda_invoke();
#endif /* PHP 8.1+ */
}
#else
Expand Down
2 changes: 2 additions & 0 deletions axiom/nr_segment.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ typedef struct _nr_segment_cloud_attrs_t {
relationship.*/
char* cloud_account_id; /*The cloud provider account ID. Needed for SQS
relationship.*/
char* cloud_platform; /*The platform hosting the cloud. Needed for Lambda
relationship.*/
char* cloud_resource_id; /*Unique cloud provider identifier. For AWS, this is
the ARN of the AWS resource being accessed.*/
char* aws_operation; /*AWS specific operation name.*/
Expand Down
5 changes: 5 additions & 0 deletions axiom/nr_segment_traces.c
Original file line number Diff line number Diff line change
Expand Up @@ -648,4 +648,9 @@ extern void nr_segment_traces_add_cloud_attributes(
segment->attributes, NR_CLOUD_AGENT_ATTRIBUTE_DESTINATION,
NR_ATTR_AWS_OPERATION, cloud_attrs->aws_operation);
}
if (!nr_strempty(cloud_attrs->cloud_platform)) {
nr_attributes_agent_add_string(
segment->attributes, NR_CLOUD_AGENT_ATTRIBUTE_DESTINATION,
NR_ATTR_CLOUD_PLATFORM, cloud_attrs->cloud_platform);
}
}
1 change: 1 addition & 0 deletions axiom/nr_span_event.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define NR_ATTR_CLOUD_REGION "cloud.region"
#define NR_ATTR_CLOUD_ACCOUNT_ID "cloud.account.id"
#define NR_ATTR_CLOUD_RESOURCE_ID "cloud.resource_id"
#define NR_ATTR_CLOUD_PLATFORM "cloud.platform"
#define NR_ATTR_AWS_OPERATION "aws.operation"

typedef struct _nr_span_event_t nr_span_event_t;
Expand Down
Loading