From 2df782fbb8521f1618706203cd68552a1ce6d168 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 17 Jan 2025 17:12:22 -0700 Subject: [PATCH 01/71] feat(agent): Add RabbitMQ instrumentation using the php-amqplib library Initial commit does the following: * Detect library via magic file * Detect package and version information. * Basic unit tests --- agent/Makefile.frag | 1 + agent/config.m4 | 2 +- agent/fw_hooks.h | 1 + agent/lib_php_amqplib.c | 82 +++++++++++++++++++++ agent/lib_php_amqplib.h | 13 ++++ agent/php_execute.c | 4 ++ agent/tests/test_lib_php_amqplib.c | 110 +++++++++++++++++++++++++++++ 7 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 agent/lib_php_amqplib.c create mode 100644 agent/lib_php_amqplib.h create mode 100644 agent/tests/test_lib_php_amqplib.c diff --git a/agent/Makefile.frag b/agent/Makefile.frag index fbff46d69..e10d6d0f8 100644 --- a/agent/Makefile.frag +++ b/agent/Makefile.frag @@ -93,6 +93,7 @@ TEST_BINARIES = \ tests/test_internal_instrument \ tests/test_hash \ tests/test_lib_aws_sdk_php \ + tests/test_lib_php_ampqlib \ tests/test_memcached \ tests/test_mongodb \ tests/test_monolog \ diff --git a/agent/config.m4 b/agent/config.m4 index 6671bcd54..19785b2a3 100644 --- a/agent/config.m4 +++ b/agent/config.m4 @@ -231,7 +231,7 @@ if test "$PHP_NEWRELIC" = "yes"; then LIBRARIES="lib_aws_sdk_php.c lib_monolog.c lib_doctrine2.c lib_guzzle3.c \ lib_guzzle4.c lib_guzzle6.c lib_guzzle_common.c \ lib_mongodb.c lib_phpunit.c lib_predis.c lib_zend_http.c \ - lib_composer.c" + lib_composer.c lib_php_amqplib.c" PHP_NEW_EXTENSION(newrelic, $FRAMEWORKS $LIBRARIES $NEWRELIC_AGENT, $ext_shared,, $(NEWRELIC_CFLAGS)) PHP_SUBST(NEWRELIC_CFLAGS) diff --git a/agent/fw_hooks.h b/agent/fw_hooks.h index 711c3b618..93665862c 100644 --- a/agent/fw_hooks.h +++ b/agent/fw_hooks.h @@ -46,6 +46,7 @@ extern void nr_guzzle4_enable(TSRMLS_D); extern void nr_guzzle6_enable(TSRMLS_D); extern void nr_laminas_http_enable(TSRMLS_D); extern void nr_mongodb_enable(TSRMLS_D); +extern void nr_php_amqplib_enable(); extern void nr_phpunit_enable(TSRMLS_D); extern void nr_predis_enable(TSRMLS_D); extern void nr_zend_http_enable(TSRMLS_D); diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c new file mode 100644 index 000000000..07af6d959 --- /dev/null +++ b/agent/lib_php_amqplib.c @@ -0,0 +1,82 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +/* + * Functions relating to instrumenting the AWS-SDK-PHP. + * https://github.com/aws/aws-sdk-php + */ +#include "php_agent.h" +#include "php_call.h" +#include "php_hash.h" +#include "php_wrapper.h" +#include "fw_hooks.h" +#include "fw_support.h" +#include "util_logging.h" +#include "lib_php_amqplib.h" + +#define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" + +/* + * nr_php_amqplib_handle_version will automatically load the class if it isn't + * loaded yet and then evaluate the string. To avoid the VERY unlikely but not + * impossible fatal error if the file/class isn't loaded yet, we need to wrap + * the call in a try/catch block and make it a lambda so that we avoid fatal + * errors. + */ +void nr_php_amqplib_handle_version() { + char* version = NULL; + zval retval; + int result = FAILURE; + + result = zend_eval_string( + "(function() {" + " $nr_php_amqplib_version = '';" + " try {" + " $nr_php_amqplib_version = PhpAmqpLib\\Package::VERSION;" + " } catch (Throwable $e) {" + " }" + " return $nr_php_amqplib_version;" + "})();", + &retval, "Get nr_php_amqplib_version"); + + /* See if we got a non-empty/non-null string for version. */ + if (SUCCESS == result) { + if (nr_php_is_zval_non_empty_string(&retval)) { + version = Z_STRVAL(retval); + } + } + + if (NRINI(vulnerability_management_package_detection_enabled)) { + /* Add php package to transaction */ + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); + } + + nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, + version); + + zval_dtor(&retval); +} + +/* + * + * Version detection will be called directly from Aws\Sdk.php + */ +void nr_php_amqplib_enable() { + /* + * Set the UNKNOWN package first, so it doesn't overwrite what we find with + * nr_lib_aws_sdk_php_handle_version. + */ + if (NRINI(vulnerability_management_package_detection_enabled)) { + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, + PHP_PACKAGE_VERSION_UNKNOWN); + } + + /* Extract the version for aws-sdk 3+ */ + nr_php_amqplib_handle_version(); + + /* Called when initializing all Clients */ + // nr_php_wrap_user_function(NR_PSTR("Aws\\AwsClient::parseClass"), + // nr_create_aws_service_metric); +} diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h new file mode 100644 index 000000000..6942c827b --- /dev/null +++ b/agent/lib_php_amqplib.h @@ -0,0 +1,13 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Functions relating to instrumenting AWS-SDK-PHP. + */ +#ifndef LIB_PHP_AMQPLIB +#define LIB_PHP_AMQPLIB + +extern void nr_aws_php_amqplib_enable(); +extern void nr_php_amqplib_handle_version(); + +#endif /* LIB_PHP_AMQPLIB */ diff --git a/agent/php_execute.c b/agent/php_execute.c index 058fea30c..442fe8352 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -491,6 +491,10 @@ static nr_library_table_t libraries[] = { {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, + /* php-amqplib RabbitMQ >= 3.7 */ + {"php-amqplib", NR_PSTR("phpamqplib/connection/abstractconnection.php"), + nr_php_amqplib_enable}, + /* * The first path is for Composer installs, the second is for * /usr/local/bin. diff --git a/agent/tests/test_lib_php_amqplib.c b/agent/tests/test_lib_php_amqplib.c new file mode 100644 index 000000000..723c4089d --- /dev/null +++ b/agent/tests/test_lib_php_amqplib.c @@ -0,0 +1,110 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "tlib_php.h" + +#include "php_agent.h" +#include "lib_php_amqplib.h" +#include "fw_support.h" + +tlib_parallel_info_t parallel_info + = {.suggested_nthreads = -1, .state_size = 0}; + +#if ZEND_MODULE_API_NO > ZEND_7_1_X_API_NO + +static void declare_php_amqplib_package_class(const char* ns, + const char* klass, + const char* sdk_version) { + char* source = nr_formatf( + "namespace %s;" + "class %s{" + "const VERSION = '%s';" + "}", + ns, klass, sdk_version); + + tlib_php_request_eval(source); + + nr_free(source); +} + +static void test_nr_lib_php_amqplib_handle_version(void) { +#define LIBRARY_NAME "php-amqplib/php-amqplib" + const char* library_versions[] + = {"7", "10", "100", "4.23", "55.34", "6123.45", "0.4.5"}; + nr_php_package_t* p = NULL; +#define TEST_DESCRIPTION_FMT \ + "nr_lib_php_amqplib_handle_version with library_versions[%ld]=%s: package " \ + "major version metric - %s" + char* test_description = NULL; + size_t i = 0; + + /* + * If lib_php_amqplib_handle_version function is ever called, we have already + * detected the php-amqplib library. + */ + + /* + * PhpAmqpLib/Package class exists. Should create php-amqplib package metric + * suggestion with version + */ + for (i = 0; i < sizeof(library_versions) / sizeof(library_versions[0]); i++) { + tlib_php_request_start(); + + declare_php_amqplib_package_class("PhpAmqpLib", "Package", + library_versions[i]); + nr_lib_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, + LIBRARY_NAME); + + test_description = nr_formatf(TEST_DESCRIPTION_FMT, i, library_versions[i], + "suggestion created"); + tlib_pass_if_not_null(test_description, p); + nr_free(test_description); + + test_description = nr_formatf(TEST_DESCRIPTION_FMT, i, library_versions[i], + "suggested version set"); + tlib_pass_if_str_equal(test_description, library_versions[i], + p->package_version); + nr_free(test_description); + + tlib_php_request_end(); + } + + /* + * PhpAmqpLib/Package class does not exist, should create package metric + * suggestion with PHP_PACKAGE_VERSION_UNKNOWN version. This case should never + * happen in real situations. + */ + tlib_php_request_start(); + + nr_lib_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); + + tlib_pass_if_not_null( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is not " + "defined - " + "suggestion created", + p); + tlib_pass_if_str_equal( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is not " + "defined - " + "suggested version set to PHP_PACKAGE_VERSION_UNKNOWN", + PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); + + tlib_php_request_end(); +} + +void test_main(void* p NRUNUSED) { + tlib_php_engine_create(""); + test_nr_lib_php_amqplib_handle_version(); + tlib_php_engine_destroy(); +} +#else +void test_main(void* p NRUNUSED) {} +#endif From 0b184ca6b789e57275193d9d105df070736aedda Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 17 Jan 2025 17:12:22 -0700 Subject: [PATCH 02/71] feat(agent): Add RabbitMQ instrumentation using the php-amqplib library Initial commit does the following: * Detect library via magic file * Detect package and version information. * Basic unit tests --- agent/Makefile.frag | 1 + agent/config.m4 | 2 +- agent/fw_hooks.h | 1 + agent/lib_php_amqplib.c | 82 +++++++++++++++++++++ agent/lib_php_amqplib.h | 13 ++++ agent/php_execute.c | 4 ++ agent/tests/test_lib_php_amqplib.c | 110 +++++++++++++++++++++++++++++ 7 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 agent/lib_php_amqplib.c create mode 100644 agent/lib_php_amqplib.h create mode 100644 agent/tests/test_lib_php_amqplib.c diff --git a/agent/Makefile.frag b/agent/Makefile.frag index fbff46d69..e10d6d0f8 100644 --- a/agent/Makefile.frag +++ b/agent/Makefile.frag @@ -93,6 +93,7 @@ TEST_BINARIES = \ tests/test_internal_instrument \ tests/test_hash \ tests/test_lib_aws_sdk_php \ + tests/test_lib_php_ampqlib \ tests/test_memcached \ tests/test_mongodb \ tests/test_monolog \ diff --git a/agent/config.m4 b/agent/config.m4 index 6671bcd54..19785b2a3 100644 --- a/agent/config.m4 +++ b/agent/config.m4 @@ -231,7 +231,7 @@ if test "$PHP_NEWRELIC" = "yes"; then LIBRARIES="lib_aws_sdk_php.c lib_monolog.c lib_doctrine2.c lib_guzzle3.c \ lib_guzzle4.c lib_guzzle6.c lib_guzzle_common.c \ lib_mongodb.c lib_phpunit.c lib_predis.c lib_zend_http.c \ - lib_composer.c" + lib_composer.c lib_php_amqplib.c" PHP_NEW_EXTENSION(newrelic, $FRAMEWORKS $LIBRARIES $NEWRELIC_AGENT, $ext_shared,, $(NEWRELIC_CFLAGS)) PHP_SUBST(NEWRELIC_CFLAGS) diff --git a/agent/fw_hooks.h b/agent/fw_hooks.h index 711c3b618..93665862c 100644 --- a/agent/fw_hooks.h +++ b/agent/fw_hooks.h @@ -46,6 +46,7 @@ extern void nr_guzzle4_enable(TSRMLS_D); extern void nr_guzzle6_enable(TSRMLS_D); extern void nr_laminas_http_enable(TSRMLS_D); extern void nr_mongodb_enable(TSRMLS_D); +extern void nr_php_amqplib_enable(); extern void nr_phpunit_enable(TSRMLS_D); extern void nr_predis_enable(TSRMLS_D); extern void nr_zend_http_enable(TSRMLS_D); diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c new file mode 100644 index 000000000..07af6d959 --- /dev/null +++ b/agent/lib_php_amqplib.c @@ -0,0 +1,82 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +/* + * Functions relating to instrumenting the AWS-SDK-PHP. + * https://github.com/aws/aws-sdk-php + */ +#include "php_agent.h" +#include "php_call.h" +#include "php_hash.h" +#include "php_wrapper.h" +#include "fw_hooks.h" +#include "fw_support.h" +#include "util_logging.h" +#include "lib_php_amqplib.h" + +#define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" + +/* + * nr_php_amqplib_handle_version will automatically load the class if it isn't + * loaded yet and then evaluate the string. To avoid the VERY unlikely but not + * impossible fatal error if the file/class isn't loaded yet, we need to wrap + * the call in a try/catch block and make it a lambda so that we avoid fatal + * errors. + */ +void nr_php_amqplib_handle_version() { + char* version = NULL; + zval retval; + int result = FAILURE; + + result = zend_eval_string( + "(function() {" + " $nr_php_amqplib_version = '';" + " try {" + " $nr_php_amqplib_version = PhpAmqpLib\\Package::VERSION;" + " } catch (Throwable $e) {" + " }" + " return $nr_php_amqplib_version;" + "})();", + &retval, "Get nr_php_amqplib_version"); + + /* See if we got a non-empty/non-null string for version. */ + if (SUCCESS == result) { + if (nr_php_is_zval_non_empty_string(&retval)) { + version = Z_STRVAL(retval); + } + } + + if (NRINI(vulnerability_management_package_detection_enabled)) { + /* Add php package to transaction */ + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); + } + + nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, + version); + + zval_dtor(&retval); +} + +/* + * + * Version detection will be called directly from Aws\Sdk.php + */ +void nr_php_amqplib_enable() { + /* + * Set the UNKNOWN package first, so it doesn't overwrite what we find with + * nr_lib_aws_sdk_php_handle_version. + */ + if (NRINI(vulnerability_management_package_detection_enabled)) { + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, + PHP_PACKAGE_VERSION_UNKNOWN); + } + + /* Extract the version for aws-sdk 3+ */ + nr_php_amqplib_handle_version(); + + /* Called when initializing all Clients */ + // nr_php_wrap_user_function(NR_PSTR("Aws\\AwsClient::parseClass"), + // nr_create_aws_service_metric); +} diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h new file mode 100644 index 000000000..6942c827b --- /dev/null +++ b/agent/lib_php_amqplib.h @@ -0,0 +1,13 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Functions relating to instrumenting AWS-SDK-PHP. + */ +#ifndef LIB_PHP_AMQPLIB +#define LIB_PHP_AMQPLIB + +extern void nr_aws_php_amqplib_enable(); +extern void nr_php_amqplib_handle_version(); + +#endif /* LIB_PHP_AMQPLIB */ diff --git a/agent/php_execute.c b/agent/php_execute.c index 4a8765f41..f8d75628b 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -491,6 +491,10 @@ static nr_library_table_t libraries[] = { {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, + /* php-amqplib RabbitMQ >= 3.7 */ + {"php-amqplib", NR_PSTR("phpamqplib/connection/abstractconnection.php"), + nr_php_amqplib_enable}, + /* * The first path is for Composer installs, the second is for * /usr/local/bin. diff --git a/agent/tests/test_lib_php_amqplib.c b/agent/tests/test_lib_php_amqplib.c new file mode 100644 index 000000000..723c4089d --- /dev/null +++ b/agent/tests/test_lib_php_amqplib.c @@ -0,0 +1,110 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "tlib_php.h" + +#include "php_agent.h" +#include "lib_php_amqplib.h" +#include "fw_support.h" + +tlib_parallel_info_t parallel_info + = {.suggested_nthreads = -1, .state_size = 0}; + +#if ZEND_MODULE_API_NO > ZEND_7_1_X_API_NO + +static void declare_php_amqplib_package_class(const char* ns, + const char* klass, + const char* sdk_version) { + char* source = nr_formatf( + "namespace %s;" + "class %s{" + "const VERSION = '%s';" + "}", + ns, klass, sdk_version); + + tlib_php_request_eval(source); + + nr_free(source); +} + +static void test_nr_lib_php_amqplib_handle_version(void) { +#define LIBRARY_NAME "php-amqplib/php-amqplib" + const char* library_versions[] + = {"7", "10", "100", "4.23", "55.34", "6123.45", "0.4.5"}; + nr_php_package_t* p = NULL; +#define TEST_DESCRIPTION_FMT \ + "nr_lib_php_amqplib_handle_version with library_versions[%ld]=%s: package " \ + "major version metric - %s" + char* test_description = NULL; + size_t i = 0; + + /* + * If lib_php_amqplib_handle_version function is ever called, we have already + * detected the php-amqplib library. + */ + + /* + * PhpAmqpLib/Package class exists. Should create php-amqplib package metric + * suggestion with version + */ + for (i = 0; i < sizeof(library_versions) / sizeof(library_versions[0]); i++) { + tlib_php_request_start(); + + declare_php_amqplib_package_class("PhpAmqpLib", "Package", + library_versions[i]); + nr_lib_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, + LIBRARY_NAME); + + test_description = nr_formatf(TEST_DESCRIPTION_FMT, i, library_versions[i], + "suggestion created"); + tlib_pass_if_not_null(test_description, p); + nr_free(test_description); + + test_description = nr_formatf(TEST_DESCRIPTION_FMT, i, library_versions[i], + "suggested version set"); + tlib_pass_if_str_equal(test_description, library_versions[i], + p->package_version); + nr_free(test_description); + + tlib_php_request_end(); + } + + /* + * PhpAmqpLib/Package class does not exist, should create package metric + * suggestion with PHP_PACKAGE_VERSION_UNKNOWN version. This case should never + * happen in real situations. + */ + tlib_php_request_start(); + + nr_lib_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); + + tlib_pass_if_not_null( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is not " + "defined - " + "suggestion created", + p); + tlib_pass_if_str_equal( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is not " + "defined - " + "suggested version set to PHP_PACKAGE_VERSION_UNKNOWN", + PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); + + tlib_php_request_end(); +} + +void test_main(void* p NRUNUSED) { + tlib_php_engine_create(""); + test_nr_lib_php_amqplib_handle_version(); + tlib_php_engine_destroy(); +} +#else +void test_main(void* p NRUNUSED) {} +#endif From feba725d6e9ba383d7a0d3f9352abe9fed7a6b50 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 11:57:17 -0700 Subject: [PATCH 03/71] fix(agent): Typo remove AWS references --- agent/lib_php_amqplib.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 07af6d959..ec0a05c74 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -4,8 +4,8 @@ */ /* - * Functions relating to instrumenting the AWS-SDK-PHP. - * https://github.com/aws/aws-sdk-php + * Functions relating to instrumenting the php-ampqlib + * https://github.com/php-amqplib/php-amqplib */ #include "php_agent.h" #include "php_call.h" @@ -19,6 +19,7 @@ #define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" /* + * Version detection will be called directly from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not * impossible fatal error if the file/class isn't loaded yet, we need to wrap @@ -59,14 +60,10 @@ void nr_php_amqplib_handle_version() { zval_dtor(&retval); } -/* - * - * Version detection will be called directly from Aws\Sdk.php - */ void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with - * nr_lib_aws_sdk_php_handle_version. + * nr_php_amqplib_handle_version. */ if (NRINI(vulnerability_management_package_detection_enabled)) { nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, @@ -75,8 +72,4 @@ void nr_php_amqplib_enable() { /* Extract the version for aws-sdk 3+ */ nr_php_amqplib_handle_version(); - - /* Called when initializing all Clients */ - // nr_php_wrap_user_function(NR_PSTR("Aws\\AwsClient::parseClass"), - // nr_create_aws_service_metric); } From feef4be9712cd24c443b4b09172566a49e4e9722 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 13:10:57 -0700 Subject: [PATCH 04/71] fix(agent): typo --- agent/Makefile.frag | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/Makefile.frag b/agent/Makefile.frag index e10d6d0f8..2bfc562a8 100644 --- a/agent/Makefile.frag +++ b/agent/Makefile.frag @@ -93,7 +93,7 @@ TEST_BINARIES = \ tests/test_internal_instrument \ tests/test_hash \ tests/test_lib_aws_sdk_php \ - tests/test_lib_php_ampqlib \ + tests/test_lib_php_amqplib \ tests/test_memcached \ tests/test_mongodb \ tests/test_monolog \ From 761de36198b4abbd2d6cfa7e61155fa000638a74 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 13:21:51 -0700 Subject: [PATCH 05/71] feat(agent): Add additional unit test Test version detection when class exists but version const doesn't. Fixed typos. --- agent/tests/test_lib_php_amqplib.c | 42 +++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/agent/tests/test_lib_php_amqplib.c b/agent/tests/test_lib_php_amqplib.c index 723c4089d..c0261cfc2 100644 --- a/agent/tests/test_lib_php_amqplib.c +++ b/agent/tests/test_lib_php_amqplib.c @@ -16,13 +16,13 @@ tlib_parallel_info_t parallel_info static void declare_php_amqplib_package_class(const char* ns, const char* klass, - const char* sdk_version) { + const char* package_version) { char* source = nr_formatf( "namespace %s;" "class %s{" "const VERSION = '%s';" "}", - ns, klass, sdk_version); + ns, klass, package_version); tlib_php_request_eval(source); @@ -54,7 +54,7 @@ static void test_nr_lib_php_amqplib_handle_version(void) { declare_php_amqplib_package_class("PhpAmqpLib", "Package", library_versions[i]); - nr_lib_php_amqplib_handle_version(); + nr_php_amqplib_handle_version(); p = nr_php_packages_get_package( NRPRG(txn)->php_package_major_version_metrics_suggestions, @@ -81,7 +81,7 @@ static void test_nr_lib_php_amqplib_handle_version(void) { */ tlib_php_request_start(); - nr_lib_php_amqplib_handle_version(); + nr_php_amqplib_handle_version(); p = nr_php_packages_get_package( NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); @@ -98,6 +98,40 @@ static void test_nr_lib_php_amqplib_handle_version(void) { PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); tlib_php_request_end(); + + /* + * PhpAmqpLib\\Package class exists but VERSION does not. + * Should create package metric suggestion with PHP_PACKAGE_VERSION_UNKNOWN + * version. This case should never happen in real situations. + */ + tlib_php_request_start(); + + char* source + = "namespace PhpAmqpLib;" + "class Package{" + "const SADLY_DEPRECATED = 5.4;" + "}"; + + tlib_php_request_eval(source); + + nr_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); + + tlib_pass_if_not_null( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is SET " + "but the const VERSION does not exist - " + "suggestion created", + p); + tlib_pass_if_str_equal( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is SET " + "but the const VERSION does not exist - " + "defined - " + "suggested version set to PHP_PACKAGE_VERSION_UNKNOWN", + PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); + + tlib_php_request_end(); } void test_main(void* p NRUNUSED) { From 80be7a50eaa4fe2d3d3be04b2cf03b1ecdd28acd Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 13:49:30 -0700 Subject: [PATCH 06/71] chore(agent): Clarifying comment. --- agent/lib_aws_sdk_php.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agent/lib_aws_sdk_php.c b/agent/lib_aws_sdk_php.c index b706df518..b1025c256 100644 --- a/agent/lib_aws_sdk_php.c +++ b/agent/lib_aws_sdk_php.c @@ -441,6 +441,12 @@ void nr_lib_aws_sdk_php_handle_version() { zval retval; int result = FAILURE; + /* + * The following block initializes nr_aws_sdk_version to the empty string. + * If it is able to extract the version, nr_aws_sdk_version is set to that. + * Nothing is needed in the catch block. + * The final return will either return a proper version or an empty string. + */ result = zend_eval_string( "(function() {" " $nr_aws_sdk_version = '';" From b50a5d2d9129caf1900d60d9d8c4719e9e3169cb Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 24 Jan 2025 17:49:44 -0700 Subject: [PATCH 07/71] feat(axiom): Update message segment to handle rabbitmq --- axiom/nr_segment.c | 12 ++ axiom/nr_segment.h | 13 ++ axiom/nr_segment_message.c | 5 + axiom/nr_segment_message.h | 14 ++ axiom/nr_segment_private.c | 2 + axiom/nr_segment_traces.c | 10 ++ axiom/nr_span_event.c | 71 ++++++++++ axiom/nr_span_event.h | 23 +++- axiom/nr_span_event_private.h | 3 + axiom/tests/test_segment_message.c | 206 +++++++++++++++++++++++++++++ axiom/tests/test_span_event.c | 73 ++++++++-- 11 files changed, 423 insertions(+), 9 deletions(-) diff --git a/axiom/nr_segment.c b/axiom/nr_segment.c index 4aaf7c120..ade63d454 100644 --- a/axiom/nr_segment.c +++ b/axiom/nr_segment.c @@ -331,6 +331,15 @@ static void nr_populate_message_spans(nr_span_event_t* span_event, segment->typed_attributes->message.messaging_system); nr_span_event_set_message(span_event, NR_SPAN_MESSAGE_SERVER_ADDRESS, segment->typed_attributes->message.server_address); + nr_span_event_set_message( + span_event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, + segment->typed_attributes->message.messaging_destination_routing_key); + nr_span_event_set_message( + span_event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME, + segment->typed_attributes->message.messaging_destination_publish_name); + nr_span_event_set_message_ulong( + span_event, NR_SPAN_MESSAGE_SERVER_PORT, + segment->typed_attributes->message.server_port); } static nr_status_t add_user_attribute_to_span_event(const char* key, @@ -640,7 +649,10 @@ bool nr_segment_set_message(nr_segment_t* segment, .message_action = message->message_action, .destination_name = nr_strempty(message->destination_name) ? NULL: nr_strdup(message->destination_name), .messaging_system = nr_strempty(message->messaging_system) ? NULL: nr_strdup(message->messaging_system), + .messaging_destination_routing_key = nr_strempty(message->messaging_destination_routing_key) ? NULL: nr_strdup(message->messaging_destination_routing_key), + .messaging_destination_publish_name = nr_strempty(message->messaging_destination_publish_name) ? NULL: nr_strdup(message->messaging_destination_publish_name), .server_address = nr_strempty(message->server_address) ? NULL: nr_strdup(message->server_address), + .server_port = message->server_port, }; // clang-format on diff --git a/axiom/nr_segment.h b/axiom/nr_segment.h index f35ab6224..646f11e5f 100644 --- a/axiom/nr_segment.h +++ b/axiom/nr_segment.h @@ -128,6 +128,19 @@ typedef struct _nr_segment_message_t { char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/ char* server_address; /*The server domain name or IP address. Needed for MQBROKER relationship.*/ + char* + messaging_destination_publish_name; /* Otel attribute for message + consumers. (In the agent, this + means Action is Consume in the span + name). This attribute is equal to + the corresponding attribute + messaging.destination.name from the + producer. This attribute is needed + for apps using RabbitMQ and it + represents the exchange name.*/ + char* messaging_destination_routing_key; /* The routing key for a RabbitMQ + operation.*/ + uint64_t server_port; /*The server port.*/ } nr_segment_message_t; typedef struct _nr_segment_cloud_attrs_t { diff --git a/axiom/nr_segment_message.c b/axiom/nr_segment_message.c index 92f8babd1..2c8207138 100644 --- a/axiom/nr_segment_message.c +++ b/axiom/nr_segment_message.c @@ -39,6 +39,11 @@ static void nr_segment_message_set_attrs( message_attributes.destination_name = params->destination_name; message_attributes.messaging_system = params->messaging_system; message_attributes.server_address = params->server_address; + message_attributes.messaging_destination_routing_key + = params->messaging_destination_routing_key; + message_attributes.messaging_destination_publish_name + = params->messaging_destination_publish_name; + message_attributes.server_port = params->server_port; } nr_segment_set_message(segment, &message_attributes); diff --git a/axiom/nr_segment_message.h b/axiom/nr_segment_message.h index 2104e3f35..70580697b 100644 --- a/axiom/nr_segment_message.h +++ b/axiom/nr_segment_message.h @@ -38,6 +38,20 @@ typedef struct { char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/ char* server_address; /*The server domain name or IP address. Needed for MQBROKER relationship.*/ + char* + messaging_destination_publish_name; /* Otel attribute for message + consumers. (In the agent, this + means Action is Consume in the span + name). This attribute is equal to + the corresponding attribute + messaging.destination.name from the + producer. This attribute is needed + for apps using RabbitMQ and it + represents the exchange name.*/ + char* messaging_destination_routing_key; /* The routing key for a RabbitMQ + operation.*/ + uint64_t server_port; /*The server port.*/ + } nr_segment_message_params_t; /* diff --git a/axiom/nr_segment_private.c b/axiom/nr_segment_private.c index f063862b7..ea5bbf8cc 100644 --- a/axiom/nr_segment_private.c +++ b/axiom/nr_segment_private.c @@ -47,6 +47,8 @@ void nr_segment_message_destroy_fields(nr_segment_message_t* message) { nr_free(message->destination_name); nr_free(message->messaging_system); nr_free(message->server_address); + nr_free(message->messaging_destination_publish_name); + nr_free(message->messaging_destination_routing_key); } void nr_segment_destroy_typed_attributes( diff --git a/axiom/nr_segment_traces.c b/axiom/nr_segment_traces.c index 153ccd5c2..d2f1fddac 100644 --- a/axiom/nr_segment_traces.c +++ b/axiom/nr_segment_traces.c @@ -170,6 +170,16 @@ static void add_typed_attributes_to_buffer(nrbuf_t* buf, message->messaging_system, false); add_hash_key_value_to_buffer(buf, "server_address", message->server_address, false); + add_hash_key_value_to_buffer(buf, "messaging_destination_publish_name", + message->messaging_destination_publish_name, + false); + add_hash_key_value_to_buffer(buf, "messaging_destination_routing_key", + message->messaging_destination_routing_key, + false); + if (0 != message->server_port) { + add_hash_key_value_to_buffer_int(buf, "server_port", + &message->server_port); + } } break; case NR_SEGMENT_CUSTOM: default: diff --git a/axiom/nr_span_event.c b/axiom/nr_span_event.c index 00987881a..7c8c2dbe0 100644 --- a/axiom/nr_span_event.c +++ b/axiom/nr_span_event.c @@ -377,6 +377,42 @@ void nr_span_event_set_message(nr_span_event_t* event, nro_set_hash_string(event->agent_attributes, NR_ATTR_SERVER_ADDRESS, new_value); break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + nro_set_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_ROUTING_KEY, new_value); + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + nro_set_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_PUBLISH_NAME, + new_value); + break; + case NR_SPAN_MESSAGE_SERVER_PORT: + break; + } +} + +void nr_span_event_set_message_ulong(nr_span_event_t* event, + nr_span_event_message_member_t member, + const uint64_t new_value) { + if (NULL == event || 0 == new_value) { + return; + } + + switch (member) { + case NR_SPAN_MESSAGE_SERVER_PORT: + nro_set_hash_ulong(event->agent_attributes, NR_ATTR_SERVER_PORT, + new_value); + break; + case NR_SPAN_MESSAGE_DESTINATION_NAME: + break; + case NR_SPAN_MESSAGE_MESSAGING_SYSTEM: + break; + case NR_SPAN_MESSAGE_SERVER_ADDRESS: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + break; } } @@ -535,10 +571,45 @@ const char* nr_span_event_get_message(const nr_span_event_t* event, case NR_SPAN_MESSAGE_SERVER_ADDRESS: return nro_get_hash_string(event->agent_attributes, NR_ATTR_SERVER_ADDRESS, NULL); + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + return nro_get_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_ROUTING_KEY, + NULL); + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + return nro_get_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_PUBLISH_NAME, + NULL); + case NR_SPAN_MESSAGE_SERVER_PORT: + break; } return NULL; } +uint64_t nr_span_event_get_message_ulong( + const nr_span_event_t* event, + nr_span_event_message_member_t member) { + if (NULL == event) { + return 0; + } + + switch (member) { + case NR_SPAN_MESSAGE_SERVER_PORT: + return nro_get_hash_ulong(event->agent_attributes, NR_ATTR_SERVER_PORT, + NULL); + case NR_SPAN_MESSAGE_DESTINATION_NAME: + break; + case NR_SPAN_MESSAGE_MESSAGING_SYSTEM: + break; + case NR_SPAN_MESSAGE_SERVER_ADDRESS: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + break; + } + return 0; +} + void nr_span_event_set_attribute_user(nr_span_event_t* event, const char* name, const nrobj_t* value) { diff --git a/axiom/nr_span_event.h b/axiom/nr_span_event.h index 2a87b2306..33dced5c2 100644 --- a/axiom/nr_span_event.h +++ b/axiom/nr_span_event.h @@ -15,7 +15,12 @@ #define NR_ATTR_MESSAGING_DESTINATION_NAME "messaging.destination.name" #define NR_ATTR_MESSAGING_SYSTEM "messaging.system" +#define NR_ATTR_MESSAGING_DESTINATION_ROUTING_KEY \ + "messaging.rabbitmq.destination.routing_key" +#define NR_ATTR_MESSAGING_DESTINATION_PUBLISH_NAME \ + "messaging.destination_publish.name" #define NR_ATTR_SERVER_ADDRESS "server.address" +#define NR_ATTR_SERVER_PORT "server.port" #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" @@ -80,6 +85,9 @@ typedef enum { NR_SPAN_MESSAGE_DESTINATION_NAME, NR_SPAN_MESSAGE_MESSAGING_SYSTEM, NR_SPAN_MESSAGE_SERVER_ADDRESS, + NR_SPAN_MESSAGE_SERVER_PORT, + NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, + NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME } nr_span_event_message_member_t; /* @@ -211,7 +219,7 @@ extern void nr_span_event_set_external_status(nr_span_event_t* event, const uint64_t status); /* - * Purpose : Set a message attribute. + * Purpose : Set a message attribute with a given string new_value. * * Params : 1. The target Span Event that should be changed. * 2. The message attribute to be set. @@ -222,6 +230,19 @@ extern void nr_span_event_set_message(nr_span_event_t* event, nr_span_event_message_member_t member, const char* new_value); +/* + * Purpose : Set a message attribute with a given ulong new_value. + * + * Params : 1. The target Span Event that should be changed. + * 2. The message attribute to be set. + * 3. The ulong value that the field will be after the function has + * executed. + */ +extern void nr_span_event_set_message_ulong( + nr_span_event_t* event, + nr_span_event_message_member_t member, + const uint64_t new_value); + /* * Purpose : Set a user attribute. * diff --git a/axiom/nr_span_event_private.h b/axiom/nr_span_event_private.h index 4f55c6f2f..01d544fc2 100644 --- a/axiom/nr_span_event_private.h +++ b/axiom/nr_span_event_private.h @@ -48,6 +48,9 @@ extern uint64_t nr_span_event_get_external_status(const nr_span_event_t* event); extern const char* nr_span_event_get_message( const nr_span_event_t* event, nr_span_event_message_member_t member); +extern uint64_t nr_span_event_get_message_ulong( + const nr_span_event_t* event, + nr_span_event_message_member_t member); extern const char* nr_span_event_get_error_message( const nr_span_event_t* event); extern const char* nr_span_event_get_error_class(const nr_span_event_t* event); diff --git a/axiom/tests/test_segment_message.c b/axiom/tests/test_segment_message.c index a68403caf..9c1d3a293 100644 --- a/axiom/tests/test_segment_message.c +++ b/axiom/tests/test_segment_message.c @@ -31,6 +31,9 @@ typedef struct { const char* cloud_resource_id; const char* server_address; const char* aws_operation; + char* messaging_destination_publish_name; + char* messaging_destination_routing_key; + uint64_t server_port; } segment_message_expecteds_t; static nr_segment_t* mock_txn_segment(void) { @@ -927,6 +930,194 @@ static void test_segment_message_aws_operation(void) { .aws_operation = "sendMessage"}); } +static void test_segment_message_server_port(void) { + /* + * server port values should NOT impact the creation + * of metrics. + */ + + /* Test server port not set, implicitly unset */ + test_message_segment( + &(nr_segment_message_params_t){ + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "server port not set, implicitly unset", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .server_port = 0}); + + /* Test server port explicitly set to 0 (unset) */ + test_message_segment( + &(nr_segment_message_params_t){ + .server_port = 0, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "server port explicitly set to 0 (unset)", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .server_port = 0}); + + /* Test valid server_port */ + test_message_segment( + &(nr_segment_message_params_t){ + .server_port = 1234, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "Test valid aws_operation", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .server_port = 1234}); +} + +static void test_segment_messaging_destination_publishing_name(void) { + /* + * messaging_destination_publish_name values should NOT impact the creation + * of metrics. + */ + + /* Test messaging_destination_publish_name is NULL */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_publish_name = NULL, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_publish_name is NULL, attribute " + "should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_publish_name = NULL}); + + /* Test destination_publishing_name is empty string */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_publish_name = "", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_publish_name is empty string, " + "attribute should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_publish_name = NULL}); + + /* Test valid messaging_destination_publish_name */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_publish_name = "publish_name", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "Test valid messaging_destination_publish_name is " + "non-empty string, attribute should be the string", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_publish_name = "publish_name"}); +} + +static void test_segment_messaging_destination_routing_key(void) { + /* + * messaging_destination_routing_key values should NOT impact the creation + * of metrics. + */ + + /* Test messaging_destination_routing_key is NULL */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_routing_key = NULL, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_routing_key is NULL, attribute " + "should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_routing_key = NULL}); + + /* Test messaging_destination_routing_key is empty string */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_routing_key is empty string, " + "attribute should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_routing_key = NULL}); + + /* Test valid messaging_destination_routing_key */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "key to the kingdom", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "Test valid messaging_destination_routing_key is " + "non-empty string, attribute should be the string", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_routing_key = "key to the kingdom"}); +} + static void test_segment_message_parameters_enabled(void) { /* * Attributes should be set based on value of parameters_enabled. @@ -935,6 +1126,9 @@ static void test_segment_message_parameters_enabled(void) { /* Test true message_parameters_enabled */ test_message_segment( &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "key to the kingdom", + .server_port = 1234, + .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", @@ -958,6 +1152,9 @@ static void test_segment_message_parameters_enabled(void) { .messaging_system = "my_system", .cloud_resource_id = "my_resource_id", .server_address = "localhost", + .messaging_destination_routing_key = "key to the kingdom", + .server_port = 1234, + .messaging_destination_publish_name = "publish name", .aws_operation = "sendMessage"}); /* @@ -966,6 +1163,9 @@ static void test_segment_message_parameters_enabled(void) { */ test_message_segment( &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "key to the kingdom", + .server_port = 1234, + .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", @@ -989,6 +1189,9 @@ static void test_segment_message_parameters_enabled(void) { .messaging_system = NULL, .cloud_resource_id = "my_resource_id", .server_address = NULL, + .messaging_destination_routing_key = NULL, + .server_port = 0, + .messaging_destination_publish_name = NULL, .aws_operation = "sendMessage"}); } @@ -1005,6 +1208,9 @@ void test_main(void* p NRUNUSED) { test_segment_message_messaging_system(); test_segment_message_cloud_resource_id(); test_segment_message_server_address(); + test_segment_message_server_port(); + test_segment_messaging_destination_publishing_name(); + test_segment_messaging_destination_routing_key(); test_segment_message_aws_operation(); test_segment_message_parameters_enabled(); } diff --git a/axiom/tests/test_span_event.c b/axiom/tests/test_span_event.c index ff44ab1cd..a00700a6b 100644 --- a/axiom/tests/test_span_event.c +++ b/axiom/tests/test_span_event.c @@ -483,7 +483,7 @@ static void test_span_events_extern_get_and_set(void) { nr_span_event_destroy(&span); } -static void test_span_event_message_string_get_and_set(void) { +static void test_span_event_message_get_and_set(void) { nr_span_event_t* event = nr_span_event_create(); // Test : that is does not crash when we give the setter a NULL pointer @@ -506,36 +506,93 @@ static void test_span_event_message_string_get_and_set(void) { tlib_pass_if_null("invalid range sent to nr_span_event_get_message", nr_span_event_get_message(event, 54321)); + // Test: the ulong getter should return 0 (unset) for any string values passed + // in + nr_span_event_set_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME, "chicken"); + tlib_pass_if_uint_equal( + "nr_span_event_get_message_ulong should return 0(unset) if given the " + "enum for a string value", + 0, + nr_span_event_get_message_ulong(event, NR_SPAN_MESSAGE_DESTINATION_NAME)); + + // Test: the string getter should return NULL if given the enum for a + // non-string value + nr_span_event_set_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT, 1234); + tlib_pass_if_null( + "nr_span_event_get_message should return NULL if given the enum for a " + "non-string value", + nr_span_event_get_message(event, NR_SPAN_MESSAGE_SERVER_PORT)); + // Test : setting the destination name back and forth behaves as expected nr_span_event_set_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME, "chicken"); tlib_pass_if_str_equal( - "should be the component we set 1", "chicken", + "should be the destination name we set first", "chicken", nr_span_event_get_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME)); nr_span_event_set_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME, "oracle"); tlib_pass_if_str_equal( - "should be the component we set 2", "oracle", + "should be the destination name we set second", "oracle", nr_span_event_get_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME)); // Test : setting the messaging system back and forth behaves as expected nr_span_event_set_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM, "chicken"); tlib_pass_if_str_equal( - "should be the component we set 1", "chicken", + "should be the messaging system we set first", "chicken", nr_span_event_get_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM)); nr_span_event_set_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM, "oracle"); tlib_pass_if_str_equal( - "should be the component we set 2", "oracle", + "should be the messaging system we set second", "oracle", nr_span_event_get_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM)); // Test : setting the server address back and forth behaves as expected nr_span_event_set_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS, "chicken"); tlib_pass_if_str_equal( - "should be the component we set 1", "chicken", + "should be the server address we set first", "chicken", nr_span_event_get_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS)); nr_span_event_set_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS, "oracle"); tlib_pass_if_str_equal( - "should be the component we set 2", "oracle", + "should be the server address we set second", "oracle", nr_span_event_get_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS)); + // Test : setting the destination pubishing name back and forth behaves as + // expected + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME, "chicken"); + tlib_pass_if_str_equal( + "should be the destination publish name we set first", "chicken", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME)); + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME, "oracle"); + tlib_pass_if_str_equal( + "should be the destination publish name we set second", "oracle", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME)); + + // Test : setting the destination routing key back and forth behaves as + // expected + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, "chicken"); + tlib_pass_if_str_equal( + "should be the destination routing key we set first", "chicken", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY)); + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, "oracle"); + tlib_pass_if_str_equal( + "should be the destination routing key we set second", "oracle", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY)); + + // Test : setting the server port back and forth behaves as expected + nr_span_event_set_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT, 1234); + tlib_pass_if_ulong_equal( + "should be the server port we set first", 1234, + nr_span_event_get_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT)); + nr_span_event_set_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT, 4321); + tlib_pass_if_ulong_equal( + "should be the server port we set first", 4321, + nr_span_event_get_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT)); + nr_span_event_destroy(&event); } @@ -724,7 +781,7 @@ void test_main(void* p NRUNUSED) { test_span_event_duration(); test_span_event_datastore_string_get_and_set(); test_span_events_extern_get_and_set(); - test_span_event_message_string_get_and_set(); + test_span_event_message_get_and_set(); test_span_event_error(); test_span_event_set_attribute_user(); test_span_event_txn_parent_attributes(); From 075e2971d6c9f56208aaa37597cdbfc4fc681ecd Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 28 Jan 2025 18:37:44 -0700 Subject: [PATCH 08/71] feat(agent): Add php_amqplib basic_publish instrumentation. * Creates message segment on basic_publish call. --- agent/lib_php_amqplib.c | 172 ++++++++++++++++++++++++++++++++++++++++ agent/lib_php_amqplib.h | 6 ++ 2 files changed, 178 insertions(+) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index ec0a05c74..b1c34e0a7 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -15,9 +15,28 @@ #include "fw_support.h" #include "util_logging.h" #include "lib_php_amqplib.h" +#include "nr_segment_message.h" #define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" +/* + * With PHP 8+, we have access to all the zend_execute_data structures both + * before and after the function call so we can just maintain pointers into the + * struct. With PHP 7.x, without doing special handling, we don't have access + * to the values afterwards. Sometimes nr_php_arg_get is used as that DUPs the + * zval which then later needs to be freed with nr_php_arg_release. In this + * case, we don't need to go through the extra trouble of duplicating a ZVAL + * when we don't need to duplicate anything if there is no valid value. We + * check for a valid value, and if we want to store it, we'll strdup it. So + * instead of doing multiple zval dups all of the time, we do some strdups some + * of the time. + */ +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP8.0+ */ +#define ENSURE_PERSISTENCE(x) x +#else +#define ENSURE_PERSISTENCE(x) nr_strdup(x) +#endif + /* * Version detection will be called directly from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't @@ -60,6 +79,149 @@ void nr_php_amqplib_handle_version() { zval_dtor(&retval); } +static inline void nr_php_amqplib_get_host_and_port( + zval* amqp_connection, + nr_segment_message_params_t* message_params) { + zval* amqp_server = NULL; + zval* amqp_port = NULL; + zval* connect_constructor_params = NULL; + + if (NULL == amqp_connection || NULL == message_params) { + return; + } + + if (nr_php_is_zval_valid_object(amqp_connection)) { + connect_constructor_params + = nr_php_get_zval_object_property(amqp_connection, "construct_params"); + if (nr_php_is_zval_valid_array(connect_constructor_params)) { + amqp_server + = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), + AMQP_CONSTRUCT_PARAMS_SERVER_INDEX); + if (nr_php_is_zval_non_empty_string(amqp_server)) { + message_params->server_address + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_server)); + } + amqp_port + = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), + AMQP_CONSTRUCT_PARAMS_PORT_INDEX); + if (nr_php_is_zval_valid_scalar(amqp_port)) { + message_params->server_port = Z_LVAL_P(amqp_port); + } + } + } +} + +/* + * PhpAmqpLib\Channel\AMQPChannel::basic_publish + * Publishes a message + * + * @param AMQPMessage $msg + * @param string $exchange + * @param string $routing_key + * @param bool $mandatory + * @param bool $immediate + * @param int|null $ticket + * @throws AMQPChannelClosedException + * @throws AMQPConnectionClosedException + * @throws AMQPConnectionBlockedException + * + */ +NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { + zval* amqp_exchange = NULL; + zval* amqp_routing_key = NULL; + zval* amqp_connection = NULL; + nr_segment_t* message_segment = NULL; + + nr_segment_message_params_t message_params = { + .library = RABBITMQ_LIBRARY_NAME, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_EXCHANGE, + .message_action = NR_SPANKIND_PRODUCER, + .messaging_system = RABBITMQ_MESSAGING_SYSTEM, + }; + + (void)wraprec; + + amqp_exchange = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); + if (nr_php_is_zval_non_empty_string(amqp_exchange)) { + /* + * In PHP 7.x, the following will create a strdup in + * message_params.destination_name that needs to be freed. + */ + message_params.destination_name + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); + } else { + /* For producer, this is exchange name. Exchange name is Default in case of + * empty string. */ + message_params.destination_name = ENSURE_PERSISTENCE("Default"); + } + + amqp_routing_key = nr_php_get_user_func_arg(3, NR_EXECUTE_ORIG_ARGS); + if (nr_php_is_zval_non_empty_string(amqp_routing_key)) { + /* + * In PHP 7.x, the following will create a strdup in + * message_params.messaging_destination_routing_key that needs to be freed. + */ + message_params.messaging_destination_routing_key + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_routing_key)); + } + + amqp_connection = nr_php_get_zval_object_property( + nr_php_execute_scope(execute_data), "connection"); + /* + * In PHP 7.x, the following will create a strdup in + * message_params.server_address that needs to be freed. + */ + nr_php_amqplib_get_host_and_port(amqp_connection, &message_params); + + /* For PHP 7.x compatibility. */ + NR_PHP_WRAPPER_CALL + + /* + * Now create and end the instrumented segment as a message segment. + * + * By this point, it's been determined that this call will be instrumented so + * only create the message segment now, grab the parent segment start time, + * add our message segment attributes/metrics then close the newly created + * message segment. + */ + + if (NULL == auto_segment) { + /* + * Must be checked after PHP_WRAPPER_CALL to ensure txn didn't end during + * the call. + */ + goto end; + } + message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); + + if (NULL != message_segment) { + /* re-use start time from auto_segment started in func_begin */ + message_segment->start_time = auto_segment->start_time; + + nr_segment_message_end(&message_segment, &message_params); + } + +end: +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO + /* PHP 8+ */ + /* gnu compiler needed closure. */ + ; +#else + /* + * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, + * now we destroy them. There isn't a separate function to destroy all since + * some of the params are string literals and we don't want to strdup + * everything if we don't have to. RabbitMQ basic_publish PHP 7.x will only + * strdup server_address, destination_name, and + * messaging_destination_routing_key. + */ + nr_free(message_params.server_address); + nr_free(message_params.destination_name); + nr_free(message_params.messaging_destination_routing_key); +#endif +} +NR_PHP_WRAPPER_END + void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with @@ -72,4 +234,14 @@ void nr_php_amqplib_enable() { /* Extract the version for aws-sdk 3+ */ nr_php_amqplib_handle_version(); + +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* less than PHP8.0 */ + nr_php_wrap_user_function_before_after_clean( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), NULL, + nr_rabbitmq_basic_publish, nr_rabbitmq_basic_publish); +#else + nr_php_wrap_user_function( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), + nr_rabbitmq_basic_publish); +#endif } diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h index 6942c827b..4ca9e05e7 100644 --- a/agent/lib_php_amqplib.h +++ b/agent/lib_php_amqplib.h @@ -7,6 +7,12 @@ #ifndef LIB_PHP_AMQPLIB #define LIB_PHP_AMQPLIB +#define RABBITMQ_LIBRARY_NAME "RabbitMQ" +#define RABBITMQ_MESSAGING_SYSTEM "rabbitmq" + +#define AMQP_CONSTRUCT_PARAMS_SERVER_INDEX 0 +#define AMQP_CONSTRUCT_PARAMS_PORT_INDEX 1 + extern void nr_aws_php_amqplib_enable(); extern void nr_php_amqplib_handle_version(); From f106a205e282d26d81457e390d8932eb411ed6bb Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 28 Jan 2025 19:30:03 -0700 Subject: [PATCH 09/71] fix(axiom): Handle final metrix/txn naming when publish_name exists. --- axiom/nr_segment_message.c | 23 +++++++++++++++++------ axiom/nr_segment_message.h | 3 ++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/axiom/nr_segment_message.c b/axiom/nr_segment_message.c index 2c8207138..e2c56b966 100644 --- a/axiom/nr_segment_message.c +++ b/axiom/nr_segment_message.c @@ -101,6 +101,8 @@ static char* nr_segment_message_create_metrics( const char* action_string = NULL; const char* destination_type_string = NULL; const char* library_string = NULL; + const char* final_destination_string = NULL; + const char* destination_string = NULL; char* rollup_metric = NULL; char* scoped_metric = NULL; @@ -161,6 +163,18 @@ static char* nr_segment_message_create_metrics( destination_type_string = ""; break; } + + destination_string = nr_strempty(message_params->destination_name) + ? "" + : message_params->destination_name; + /* + * messaging_destination_publish_name is only used if it exists; In all other + * cases, we use the value from destination_string. + */ + final_destination_string = message_params->messaging_destination_publish_name + ? message_params->messaging_destination_publish_name + : destination_string; + /* * Create the scoped metric * MessageBroker/{Library}/{DestinationType}/{Action}/Named/{DestinationName} @@ -172,12 +186,9 @@ static char* nr_segment_message_create_metrics( scoped_metric = nr_formatf("MessageBroker/%s/%s/%s/Temp", library_string, destination_type_string, action_string); } else { - scoped_metric - = nr_formatf("MessageBroker/%s/%s/%s/Named/%s", library_string, - destination_type_string, action_string, - nr_strempty(message_params->destination_name) - ? "" - : message_params->destination_name); + scoped_metric = nr_formatf("MessageBroker/%s/%s/%s/Named/%s", + library_string, destination_type_string, + action_string, final_destination_string); } nr_segment_add_metric(segment, scoped_metric, true); diff --git a/axiom/nr_segment_message.h b/axiom/nr_segment_message.h index 70580697b..6917f78de 100644 --- a/axiom/nr_segment_message.h +++ b/axiom/nr_segment_message.h @@ -57,7 +57,8 @@ typedef struct { /* * Purpose : End a message segment and record metrics. * - * Params : 1. nr_segment_message_params_t + * Params : 1. nr_segment_t** segment: Segment to apply message params to and end + * 2. const nr_segment_message_params_t* params: params to apply to segment * * Returns: true on success. */ From 631ae54cd38d5c2317a8558535da7a9dffed647c Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 28 Jan 2025 19:31:08 -0700 Subject: [PATCH 10/71] feat(agent): Add php_libamqp basic_get instrumentation. --- agent/lib_php_amqplib.c | 125 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index b1c34e0a7..4f663c851 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -222,6 +222,123 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { } NR_PHP_WRAPPER_END +/* + * PhpAmqpLib\Channel\AMQPChannel::basic_get + * Direct access to a queue if no message was available in the queue, return + * null + * + * @param string $queue + * @param bool $no_ack + * @param int|null $ticket + * @throws \PhpAmqpLib\Exception\AMQPTimeoutException if the specified + * operation timeout was exceeded + * @return AMQPMessage|null + */ +NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { + zval* amqp_queue = NULL; + zval* amqp_exchange = NULL; + zval* amqp_routing_key = NULL; + zval* amqp_connection = NULL; + nr_segment_t* message_segment = NULL; + zval** retval_ptr = NR_GET_RETURN_VALUE_PTR; + + nr_segment_message_params_t message_params = { + .library = RABBITMQ_LIBRARY_NAME, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_EXCHANGE, + .message_action = NR_SPANKIND_CONSUMER, + .messaging_system = RABBITMQ_MESSAGING_SYSTEM, + }; + + (void)wraprec; + + amqp_queue = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); + if (nr_php_is_zval_non_empty_string(amqp_queue)) { + /* For consumer, this is queue name. */ + message_params.destination_name + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_queue)); + } + + amqp_connection = nr_php_get_zval_object_property( + nr_php_execute_scope(execute_data), "connection"); + /* + * In PHP 7.x, the following will create a strdup in + * message_params.server_address that needs to be freed. + */ + nr_php_amqplib_get_host_and_port(amqp_connection, &message_params); + + /* Compatibility with PHP 7.x */ + NR_PHP_WRAPPER_CALL; + + if (NULL == auto_segment) { + /* + * Must be checked after PHP_WRAPPER_CALL to ensure txn didn't end during + * the call. + */ + goto end; + } + /* + *The retval should be an AMQPMessage. nr_php_is_zval_* ops do NULL checks + * as well. + */ + if (nr_php_is_zval_valid_object(*retval_ptr)) { + /* + * Get the exchange and routing key from the AMQPMessage + */ + amqp_exchange = nr_php_get_zval_object_property(*retval_ptr, "exchange"); + if (nr_php_is_zval_non_empty_string(amqp_exchange)) { + /* Used with consumer only; his is exchange name. Exchange name is + * Default in case of empty string. */ + message_params.messaging_destination_publish_name + = Z_STRVAL_P(amqp_exchange); + } else { + message_params.messaging_destination_publish_name = "Default"; + } + + amqp_routing_key + = nr_php_get_zval_object_property(*retval_ptr, "routingKey"); + if (nr_php_is_zval_non_empty_string(amqp_routing_key)) { + message_params.messaging_destination_routing_key + = Z_STRVAL_P(amqp_routing_key); + } + } + + /* Now create and end the instrumented segment as a message segment. */ + /* + * By this point, it's been determined that this call will be instrumented so + * only create the message segment now, grab the parent segment start time, + * add our message segment attributes/metrics then close the newly created + * message segment. + */ + message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); + + if (NULL == message_segment) { + goto end; + } + + /* re-use start time from auto_segment started in func_begin */ + message_segment->start_time = auto_segment->start_time; + + nr_segment_message_end(&message_segment, &message_params); + +end: +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO + /* PHP 8+ */ + /* gnu compiler needed closure. */ + ; +#else + /* + * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, + * now we destroy them. There isn't a separate function to destroy all since + * some of the params are string literals and we don't want to strdup + * everything if we don't have to. RabbitMQ basic_get PHP 7.x will only strdup + * server_address and destination_name. + */ + nr_free(message_params.server_address); + nr_free(message_params.destination_name); +#endif +} +NR_PHP_WRAPPER_END + void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with @@ -239,9 +356,17 @@ void nr_php_amqplib_enable() { nr_php_wrap_user_function_before_after_clean( NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), NULL, nr_rabbitmq_basic_publish, nr_rabbitmq_basic_publish); + + nr_php_wrap_user_function_before_after_clean( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_get"), NULL, + nr_rabbitmq_basic_get, nr_rabbitmq_basic_get); #else nr_php_wrap_user_function( NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), nr_rabbitmq_basic_publish); + + nr_php_wrap_user_function( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_get"), + nr_rabbitmq_basic_get); #endif } From f3502b5fe3426b550977a0da8928d9f5f076cbe3 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 29 Jan 2025 09:43:06 -0700 Subject: [PATCH 11/71] fix(tests): Update tests to understand the precedence of publish_name --- axiom/nr_segment_message.c | 8 +++++--- axiom/tests/test_segment_message.c | 33 +++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/axiom/nr_segment_message.c b/axiom/nr_segment_message.c index e2c56b966..d07a3217e 100644 --- a/axiom/nr_segment_message.c +++ b/axiom/nr_segment_message.c @@ -12,6 +12,7 @@ #include "nr_segment_private.h" #include "util_strings.h" #include "util_url.h" +#include "util_logging.h" /* * Purpose : Set all the typed message attributes on the segment. @@ -171,9 +172,10 @@ static char* nr_segment_message_create_metrics( * messaging_destination_publish_name is only used if it exists; In all other * cases, we use the value from destination_string. */ - final_destination_string = message_params->messaging_destination_publish_name - ? message_params->messaging_destination_publish_name - : destination_string; + final_destination_string + = nr_strempty(message_params->messaging_destination_publish_name) + ? destination_string + : message_params->messaging_destination_publish_name; /* * Create the scoped metric diff --git a/axiom/tests/test_segment_message.c b/axiom/tests/test_segment_message.c index 9c1d3a293..bdb944f39 100644 --- a/axiom/tests/test_segment_message.c +++ b/axiom/tests/test_segment_message.c @@ -89,6 +89,17 @@ static void test_message_segment(nr_segment_message_params_t* params, tlib_pass_if_str_equal(expecteds.test_name, seg->typed_attributes->message.server_address, expecteds.server_address); + tlib_pass_if_str_equal( + expecteds.test_name, + seg->typed_attributes->message.messaging_destination_publish_name, + expecteds.messaging_destination_publish_name); + tlib_pass_if_str_equal( + expecteds.test_name, + seg->typed_attributes->message.messaging_destination_routing_key, + expecteds.messaging_destination_routing_key); + tlib_pass_if_int_equal(expecteds.test_name, + seg->typed_attributes->message.server_port, + expecteds.server_port); nr_txn_destroy(&txn); } @@ -1006,8 +1017,9 @@ static void test_segment_messaging_destination_publishing_name(void) { .destination_name = "my_destination"}, &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, (segment_message_expecteds_t){ - .test_name = "messaging_destination_publish_name is NULL, attribute " - "should be NULL", + .test_name + = "messaging_destination_publish_name is NULL, attribute " + "should be NULL, destination_name is used for metric/txn", .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", @@ -1025,8 +1037,9 @@ static void test_segment_messaging_destination_publishing_name(void) { .destination_name = "my_destination"}, &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, (segment_message_expecteds_t){ - .test_name = "messaging_destination_publish_name is empty string, " - "attribute should be NULL", + .test_name + = "messaging_destination_publish_name is empty string, " + "attribute should be NULL, destination_name is used for metric/txn", .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", @@ -1045,8 +1058,9 @@ static void test_segment_messaging_destination_publishing_name(void) { &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, (segment_message_expecteds_t){ .test_name = "Test valid messaging_destination_publish_name is " - "non-empty string, attribute should be the string", - .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + "non-empty string, attribute should be the string, " + "should be used for metric/txn", + .name = "MessageBroker/SQS/Topic/Produce/Named/publish_name", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", .num_metrics = 1, @@ -1127,8 +1141,8 @@ static void test_segment_message_parameters_enabled(void) { test_message_segment( &(nr_segment_message_params_t){ .messaging_destination_routing_key = "key to the kingdom", + .messaging_destination_publish_name = "publish_name", .server_port = 1234, - .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", @@ -1142,7 +1156,7 @@ static void test_segment_message_parameters_enabled(void) { true /* enable attributes */, (segment_message_expecteds_t){ .test_name = "Test true message_parameters_enabled", - .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .name = "MessageBroker/SQS/Topic/Produce/Named/publish_name", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", .num_metrics = 1, @@ -1154,7 +1168,7 @@ static void test_segment_message_parameters_enabled(void) { .server_address = "localhost", .messaging_destination_routing_key = "key to the kingdom", .server_port = 1234, - .messaging_destination_publish_name = "publish name", + .messaging_destination_publish_name = "publish_name", .aws_operation = "sendMessage"}); /* @@ -1165,7 +1179,6 @@ static void test_segment_message_parameters_enabled(void) { &(nr_segment_message_params_t){ .messaging_destination_routing_key = "key to the kingdom", .server_port = 1234, - .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", From eaac00d24ef56613199fe5d6f86629f4d493fede Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 30 Jan 2025 12:20:54 -0700 Subject: [PATCH 12/71] fix(agent): Handle vagaries of different Connection classes * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for * the agent to wrap the instrumented functions, there are AWS MQ_BROKER * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class * file does not explicitly load or does not load in time, and the instrumented * functions are NEVER wrapped regardless of how many times they are called in * one txn. Specifically, this centered around the very slight but impactful * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which * causes an explicit load of the AMQPChannel class/file and * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped. --- agent/lib_php_amqplib.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 4f663c851..7458083ae 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -37,6 +37,34 @@ #define ENSURE_PERSISTENCE(x) nr_strdup(x) #endif +/* + * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup + * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for + * the agent to wrap the instrumented functions, there are AWS MQ_BROKER + * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class + * file does not explicitly load or does not load in time, and the instrumented + * functions are NEVER wrapped regardless of how many times they are called in + * one txn. Specifically, this centered around the very slight but impactful + * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which + * causes an explicit load of the AMQPChannel class/file and + * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load + * of the AMQPChannelclass/file. The following method is thus the only way to + * ensure the class is loaded in time for the functions to be wrapped. + */ +static void nr_php_amqplib_ensure_class() { + zval retval; + int result = FAILURE; + + result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", + &retval, "Get nr_php_amqplib_class_exists"); + /* + * We don't need to check anything else at this point. If this fails, there's + * nothing else we can do anyway. + */ + + zval_dtor(&retval); +} + /* * Version detection will be called directly from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't @@ -351,6 +379,7 @@ void nr_php_amqplib_enable() { /* Extract the version for aws-sdk 3+ */ nr_php_amqplib_handle_version(); + nr_php_amqplib_ensure_class(); #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* less than PHP8.0 */ nr_php_wrap_user_function_before_after_clean( From 095f82dd24439b86044da7e882094468003b91df Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 30 Jan 2025 17:43:01 -0700 Subject: [PATCH 13/71] fix(agent): check long not scalar --- agent/lib_php_amqplib.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 7458083ae..153b436f4 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -38,6 +38,21 @@ #endif /* + * Managing Amazon MQ for RabbitMQ engine versions +https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/rabbitmq-version-management.html +RabbitMQ version End of support on Amazon MQ +3.13 (recommended) +3.12 March 17, 2025 +3.11 February 17, 2025 +3.10 October 15, 2024 +3.9 September 16, 2024 + +4.0.5 +The latest release of RabbitMQ is 4.0.5. See change log for release notes. See +RabbitMQ support timeline to find out what release series are supported. + +Installing RabbitMQ + * * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for * the agent to wrap the instrumented functions, there are AWS MQ_BROKER @@ -45,11 +60,13 @@ * file does not explicitly load or does not load in time, and the instrumented * functions are NEVER wrapped regardless of how many times they are called in * one txn. Specifically, this centered around the very slight but impactful - * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which + * differences when using managing the AWS MQ Broker connect vs * causes an explicit load of the AMQPChannel class/file and * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped. + * + */ static void nr_php_amqplib_ensure_class() { zval retval; @@ -132,7 +149,7 @@ static inline void nr_php_amqplib_get_host_and_port( amqp_port = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (nr_php_is_zval_valid_scalar(amqp_port)) { + if (IS_LONG != Z_TYPE_P((amqp_port))) { message_params->server_port = Z_LVAL_P(amqp_port); } } From 71e5e02e7565d93c37274ae1a45b7e77d0f6779a Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 30 Jan 2025 17:57:07 -0700 Subject: [PATCH 14/71] fix(agent): add an unperist macro --- agent/lib_php_amqplib.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 153b436f4..cf008d541 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -33,8 +33,10 @@ */ #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP8.0+ */ #define ENSURE_PERSISTENCE(x) x +#define UNDO_PERSISTENCE(x) #else #define ENSURE_PERSISTENCE(x) nr_strdup(x) +#define UNDO_PERSISTENCE(x) nr_free(x); #endif /* @@ -247,11 +249,6 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { } end: -#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO - /* PHP 8+ */ - /* gnu compiler needed closure. */ - ; -#else /* * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, * now we destroy them. There isn't a separate function to destroy all since @@ -260,10 +257,9 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { * strdup server_address, destination_name, and * messaging_destination_routing_key. */ - nr_free(message_params.server_address); - nr_free(message_params.destination_name); - nr_free(message_params.messaging_destination_routing_key); -#endif + UNDO_PERSISTENCE(message_params.server_address); + UNDO_PERSISTENCE(message_params.destination_name); + UNDO_PERSISTENCE(message_params.messaging_destination_routing_key); } NR_PHP_WRAPPER_END @@ -366,11 +362,6 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { nr_segment_message_end(&message_segment, &message_params); end: -#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO - /* PHP 8+ */ - /* gnu compiler needed closure. */ - ; -#else /* * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, * now we destroy them. There isn't a separate function to destroy all since @@ -378,9 +369,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { * everything if we don't have to. RabbitMQ basic_get PHP 7.x will only strdup * server_address and destination_name. */ - nr_free(message_params.server_address); - nr_free(message_params.destination_name); -#endif + UNDO_PERSISTENCE(message_params.server_address); + UNDO_PERSISTENCE(message_params.destination_name); } NR_PHP_WRAPPER_END From ead28c21a438e64e0da8dc8dc53a1926246e73df Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 31 Jan 2025 13:17:39 -0700 Subject: [PATCH 15/71] fix(agent): Only set to 'default' in case of valid but empty string --- agent/lib_php_amqplib.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index cf008d541..a8e968c2d 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -197,9 +197,13 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { message_params.destination_name = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); } else { - /* For producer, this is exchange name. Exchange name is Default in case of - * empty string. */ - message_params.destination_name = ENSURE_PERSISTENCE("Default"); + /* + * For producer, this is exchange name. Exchange name is Default in case of + * empty string. + */ + if (nr_php_is_zval_valid_string(amqp_exchange)) { + message_params.destination_name = ENSURE_PERSISTENCE("Default"); + } } amqp_routing_key = nr_php_get_user_func_arg(3, NR_EXECUTE_ORIG_ARGS); @@ -327,12 +331,18 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { */ amqp_exchange = nr_php_get_zval_object_property(*retval_ptr, "exchange"); if (nr_php_is_zval_non_empty_string(amqp_exchange)) { - /* Used with consumer only; his is exchange name. Exchange name is + /* Used with consumer only; this is exchange name. Exchange name is * Default in case of empty string. */ message_params.messaging_destination_publish_name = Z_STRVAL_P(amqp_exchange); } else { - message_params.messaging_destination_publish_name = "Default"; + /* + * For consumer, this is exchange name. Exchange name is Default in case + * of empty string. + */ + if (nr_php_is_zval_valid_string(amqp_exchange)) { + message_params.messaging_destination_publish_name = "Default"; + } } amqp_routing_key @@ -373,7 +383,6 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { UNDO_PERSISTENCE(message_params.destination_name); } NR_PHP_WRAPPER_END - void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with From 0d708cb68e8926816caa6dc09536998407ac3c3f Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 08:44:47 -0700 Subject: [PATCH 16/71] feat(agent): Insert/retrieve DT headers on rabbitmq --- agent/lib_php_amqplib.c | 469 ++++++++++++++++++++++++++++++++++------ agent/lib_php_amqplib.h | 15 ++ 2 files changed, 421 insertions(+), 63 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a8e968c2d..372ae427d 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -8,6 +8,7 @@ * https://github.com/php-amqplib/php-amqplib */ #include "php_agent.h" +#include "php_api_distributed_trace.h" #include "php_call.h" #include "php_hash.h" #include "php_wrapper.h" @@ -16,6 +17,7 @@ #include "util_logging.h" #include "lib_php_amqplib.h" #include "nr_segment_message.h" +#include "nr_header.h" #define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" @@ -40,20 +42,17 @@ #endif /* - * Managing Amazon MQ for RabbitMQ engine versions -https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/rabbitmq-version-management.html -RabbitMQ version End of support on Amazon MQ -3.13 (recommended) -3.12 March 17, 2025 -3.11 February 17, 2025 -3.10 October 15, 2024 -3.9 September 16, 2024 - -4.0.5 -The latest release of RabbitMQ is 4.0.5. See change log for release notes. See -RabbitMQ support timeline to find out what release series are supported. - -Installing RabbitMQ + * See here for supported Amazon MQ for RabbitMQ engine versions + * https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/rabbitmq-version-management.html + * For instance: + * As of Feb 2025, 3.13 (recommended) + * + * See here for latest RabbitMQ Server https://www.rabbitmq.com/docs/download + * For instance: + * As of Feb 2025, the latest release of RabbitMQ Server is 4.0.5. + * + * https://www.rabbitmq.com/tutorials/tutorial-one-php + * Installing RabbitMQ * * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for @@ -68,20 +67,31 @@ Installing RabbitMQ * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped. * + */ +/* + * Purpose : Retrieves host and port from an AMQP Connection and sets the + * host/port values in the message_params. + * + * Params : 1. PhpAmqpLib\Connection family of connections that inherit from + * AbstractConnection + * 2. nr_segment_message_params_t* message_params that will be + * modified with port and host info, if available + * + * Returns : None */ static void nr_php_amqplib_ensure_class() { - zval retval; + zval retval_dtor; int result = FAILURE; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - &retval, "Get nr_php_amqplib_class_exists"); + &retval_dtor, "Get nr_php_amqplib_class_exists"); /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. */ - zval_dtor(&retval); + zval_dtor(&retval_dtor); } /* @@ -94,7 +104,7 @@ static void nr_php_amqplib_ensure_class() { */ void nr_php_amqplib_handle_version() { char* version = NULL; - zval retval; + zval retval_dtor; int result = FAILURE; result = zend_eval_string( @@ -106,12 +116,12 @@ void nr_php_amqplib_handle_version() { " }" " return $nr_php_amqplib_version;" "})();", - &retval, "Get nr_php_amqplib_version"); + &retval_dtor, "Get nr_php_amqplib_version"); /* See if we got a non-empty/non-null string for version. */ if (SUCCESS == result) { - if (nr_php_is_zval_non_empty_string(&retval)) { - version = Z_STRVAL(retval); + if (nr_php_is_zval_non_empty_string(&retval_dtor)) { + version = Z_STRVAL(retval_dtor); } } @@ -123,9 +133,24 @@ void nr_php_amqplib_handle_version() { nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, version); - zval_dtor(&retval); + zval_dtor(&retval_dtor); } +/* + * Purpose : Retrieves host and port from an AMQP Connection and sets the + * host/port values in the message_params. + * + * Params : 1. PhpAmqpLib\Connection family of connections that inherit from + * AbstractConnection + * 2. nr_segment_message_params_t* message_params that will be + * modified with port and host info, if available + * + * Returns : None + * + * See here for more information about the AbstractConnection class that all + * Connection classes inherit from: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Connection/AbstractConnection.php + */ static inline void nr_php_amqplib_get_host_and_port( zval* amqp_connection, nr_segment_message_params_t* message_params) { @@ -137,28 +162,332 @@ static inline void nr_php_amqplib_get_host_and_port( return; } - if (nr_php_is_zval_valid_object(amqp_connection)) { - connect_constructor_params - = nr_php_get_zval_object_property(amqp_connection, "construct_params"); - if (nr_php_is_zval_valid_array(connect_constructor_params)) { - amqp_server - = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), - AMQP_CONSTRUCT_PARAMS_SERVER_INDEX); - if (nr_php_is_zval_non_empty_string(amqp_server)) { - message_params->server_address - = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_server)); - } - amqp_port - = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), - AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (IS_LONG != Z_TYPE_P((amqp_port))) { - message_params->server_port = Z_LVAL_P(amqp_port); + /* construct params are always saved to use for cloning purposes. */ + + if (!nr_php_is_zval_valid_object(amqp_connection)) { + return; + } + + connect_constructor_params + = nr_php_get_zval_object_property(amqp_connection, "construct_params"); + if (nr_php_is_zval_valid_array(connect_constructor_params)) { + return; + } + + amqp_server + = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), + AMQP_CONSTRUCT_PARAMS_SERVER_INDEX); + if (nr_php_is_zval_non_empty_string(amqp_server)) { + message_params->server_address + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_server)); + } + + amqp_port = nr_php_zend_hash_index_find( + Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); + if (IS_LONG != Z_TYPE_P((amqp_port))) { + message_params->server_port = Z_LVAL_P(amqp_port); + } +} + +/* + * Purpose : Applies DT headers to an inbound AMQPMessage if + * newrelic.distributed_tracing_exclude_newrelic_header INI setting is false and + * if the headers don't already exist on the AMQPMessage. + * + * Params : PhpAmqpLib\Message\AMQPMessage + * + * Returns : None + * + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ + +static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { + zval* amqp_properties_array = NULL; + zval* dt_headers_zvf = NULL; + zval* amqp_headers_table = NULL; + zval* retval_set_property_zvf = NULL; + zval* retval_set_table_zvf = NULL; + zval application_headers_dtor; + zval key_zval_dtor; + zval amqp_table_retval_dtor; + zval* key_exists = NULL; + zval* amqp_table_data = NULL; + zend_ulong key_num = 0; + nr_php_string_hash_key_t* key_str = NULL; + zval* val = NULL; + int retval = FAILURE; + + /* + * Note, this functionality can be disabled by toggling the + * newrelic.distributed_tracing_exclude_newrelic_header INI setting. + */ + + /* + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ + if (!nr_php_is_zval_valid_object(amqp_msg)) { + return; + } + + amqp_properties_array + = nr_php_get_zval_object_property(amqp_msg, "properties"); + if (!nr_php_is_zval_valid_array(amqp_properties_array)) { + nrl_verbosedebug( + NRL_INSTRUMENT, + "AMQPMessage properties are invalid. AMQPMessage always sets " + "this to empty arrray by default so something is seriously wrong with " + "the message object. Exit."); + return; + } + + /* + * newrelic_get_request_metadata is an internal API that will only return DT + * headers if newrelic.distributed_tracing_exclude_newrelic_header is false. + */ + dt_headers_zvf = nr_php_call(NULL, "newrelic_get_request_metadata"); + if (!nr_php_is_zval_valid_array(dt_headers_zvf)) { + nr_php_zval_free(&dt_headers_zvf); + return; + } + + /* + * Get application+_headers string in zval form for use with nr_php_call + */ + ZVAL_STRING(&application_headers_dtor, "application_headers"); + + /* + * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable + * object + */ + amqp_headers_table = nr_php_zend_hash_find(Z_ARRVAL_P(amqp_properties_array), + "application_headers"); + + /* + * If the application_headers AMQPTable object doesn't exist, we'll have to + * create it with an empty array. + */ + if (!nr_php_is_zval_valid_object(amqp_headers_table)) { + retval = zend_eval_string( + "(function() {" + " try {" + " return new PhpAmqpLib\\Wire\\AMQPTable(array());" + " } catch (Throwable $e) {" + " return null;" + " }" + "})();", + &amqp_table_retval_dtor, "newrelic.amqplib.add_empty_headers"); + + if (FAILURE == retval + || !nr_php_is_zval_valid_object(&amqp_table_retval_dtor)) { + nrl_verbosedebug(NRL_INSTRUMENT, + "No application headers in AMQPTable, but couldn't " + "create one. Exit."); + goto end; + } + + /* + * Set the valid AMQPTable on the AMQPMessage. + */ + retval_set_property_zvf = nr_php_call( + amqp_msg, "set", &application_headers_dtor, &amqp_table_retval_dtor); + if (NULL == retval_set_property_zvf) { + nrl_verbosedebug(NRL_INSTRUMENT, + "AMQPMessage had no application_headers AMQPTable, but " + "set failed for the AMQPTable wthat was just created " + "for the application headers. Unable to proceed, exit."); + goto end; + } + /* Should have valid AMQPTable objec on the AMQPMessage at this point. */ + amqp_headers_table = nr_php_zend_hash_find( + Z_ARRVAL_P(amqp_properties_array), "application_headers"); + if (!nr_php_is_zval_valid_object(amqp_headers_table)) { + nrl_info( + NRL_INSTRUMENT, + "AMQPMessage had no application_headers AMQPTable, but unable to " + "retrieve even after creating and setting. Unable to proceed, exit."); + goto end; + } + } + + /* + * This contains the application_headers data. It is an array of + * key/encoded_array_val pairs. + */ + amqp_table_data = nr_php_get_zval_object_property(amqp_headers_table, "data"); + + /* + * First check if it's a reference to another zval, and if so, get point to + * the actual zval. + */ + + if (IS_REFERENCE == Z_TYPE_P(amqp_table_data)) { + amqp_table_data = Z_REFVAL_P(amqp_table_data); + } + if (!nr_php_is_zval_valid_array(amqp_table_data)) { + /* + * This is a basic part of the AMQPTable, if this doesn't exist, something + * is seriously wrong. Cannot proceed, exit. + */ + goto end; + } + + /* + * Loop through the DT Header array and set the headers in the + * application_header AMQPTable if they do not already exist. + */ + + ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(dt_headers_zvf), key_num, key_str, val) { + (void)key_num; + + if (NULL != key_str && nr_php_is_zval_valid_string(val)) { + key_exists + = nr_php_zend_hash_find(HASH_OF(amqp_table_data), ZSTR_VAL(key_str)); + if (NULL == key_exists) { + /* key_str is a zend_string. It needs to be a zval to pass to + * nr_php_call. */ + ZVAL_STR_COPY(&key_zval_dtor, key_str); + /* Key doesn't exist, so set the value in the AMQPTable. */ + retval_set_table_zvf + = nr_php_call(amqp_headers_table, "set", &key_zval_dtor, val); + + if (NULL == retval_set_table_zvf) { + nrl_verbosedebug(NRL_INSTRUMENT, + "%s didn't exist in the AMQPTable, but couldn't " + "set the key/val to the table.", + NRSAFESTR(Z_STRVAL(key_zval_dtor))); + } + nr_php_zval_free(&retval_set_table_zvf); } } } + ZEND_HASH_FOREACH_END(); + +end: + nr_php_zval_free(&dt_headers_zvf); + nr_php_zval_free(&retval_set_property_zvf); + zval_ptr_dtor(&application_headers_dtor); + zval_ptr_dtor(&amqp_table_retval_dtor); + zval_ptr_dtor(&key_zval_dtor); +} + +/* + * Purpose : Retrieve any DT headers from an inbound AMQPMessage if + * newrelic.distributed_tracing_exclude_newrelic_header INI setting is false + * and apply to txn. + * + * Params : PhpAmqpLib\Message\AMQPMessage + * + * Returns : None + * + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ +static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { + zval* amqp_headers_native_data_zvf = NULL; + zval* amqp_properties_array = NULL; + zval* amqp_headers_table = NULL; + zval* amqp_table_data = NULL; + zval* dt_payload = NULL; + zval* traceparent = NULL; + zval* tracestate = NULL; + char* dt_payload_string = NULL; + char* traceparent_string = NULL; + char* tracestate_string = NULL; + zend_ulong key_num = 0; + nr_php_string_hash_key_t* key_str = NULL; + zval* val = NULL; + int retval = FAILURE; + + /* + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ + if (!nr_php_is_zval_valid_object(amqp_msg)) { + return; + } + + amqp_properties_array + = nr_php_get_zval_object_property(amqp_msg, "properties"); + if (!nr_php_is_zval_valid_array(amqp_properties_array)) { + nrl_verbosedebug( + NRL_INSTRUMENT, + "AMQPMessage properties not valid. AMQPMessage always sets " + "this to empty arrray by default. something seriously wrong with " + "the message object. Unable to proceed, Exit"); + return; + } + + /* PhpAmqpLib\Wire\AMQPTable object*/ + amqp_headers_table = nr_php_zend_hash_find(Z_ARRVAL_P(amqp_properties_array), + "application_headers"); + if (!nr_php_is_zval_valid_object(amqp_headers_table)) { + /* No headers here, exit. */ + return; + } + + /* + * We can't use amqp table "data" property here because while it has the + * correct keys, the vals are encoded arrays. We need to use getNativeData + * so it will decode the values for us since it formats the AMQPTable as an + * array of unencoded key/val pairs. */ + amqp_headers_native_data_zvf + = nr_php_call(amqp_headers_table, "getNativeData"); + + if (!nr_php_is_zval_valid_array(amqp_headers_native_data_zvf)) { + nr_php_zval_free(&amqp_headers_native_data_zvf); + return; + } + + dt_payload + = nr_php_zend_hash_find(HASH_OF(amqp_headers_native_data_zvf), NEWRELIC); + dt_payload_string + = nr_php_is_zval_valid_string(dt_payload) ? Z_STRVAL_P(dt_payload) : NULL; + + traceparent = nr_php_zend_hash_find(HASH_OF(amqp_headers_native_data_zvf), + W3C_TRACEPARENT); + traceparent_string = nr_php_is_zval_valid_string(traceparent) + ? Z_STRVAL_P(traceparent) + : NULL; + + tracestate = nr_php_zend_hash_find(HASH_OF(amqp_headers_native_data_zvf), + W3C_TRACESTATE); + tracestate_string + = nr_php_is_zval_valid_string(tracestate) ? Z_STRVAL_P(tracestate) : NULL; + + if (NULL != dt_payload || NULL != traceparent) { + nr_hashmap_t* header_map = nr_header_create_distributed_trace_map( + dt_payload_string, traceparent_string, tracestate_string); + + /* + * nr_php_api_accept_distributed_trace_payload_httpsafe will add the headers + * to the txn if there have been no other inbound/outbound headers added + * already. + */ + nr_php_api_accept_distributed_trace_payload_httpsafe(NRPRG(txn), header_map, + "Queue"); + nr_hashmap_destroy(&header_map); + } + nr_php_zval_free(&amqp_headers_native_data_zvf); + + return; } /* + * Purpose : A wrapper to instrument the php-amqplib basic_publish. This + * retrieves values to populate a message segment. If + * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also + * insert the DT headers. + * * PhpAmqpLib\Channel\AMQPChannel::basic_publish * Publishes a message * @@ -174,6 +503,7 @@ static inline void nr_php_amqplib_get_host_and_port( * */ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { + zval* amqp_msg = NULL; zval* amqp_exchange = NULL; zval* amqp_routing_key = NULL; zval* amqp_connection = NULL; @@ -188,6 +518,11 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { (void)wraprec; + amqp_msg = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); + /* + * nr_php_amqplib_insert_dt_headers will check the validity of the object. + */ + nr_php_amqplib_insert_dt_headers(amqp_msg); amqp_exchange = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); if (nr_php_is_zval_non_empty_string(amqp_exchange)) { /* @@ -198,8 +533,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); } else { /* - * For producer, this is exchange name. Exchange name is Default in case of - * empty string. + * For producer, this is exchange name. Exchange name is Default in case + * of empty string. */ if (nr_php_is_zval_valid_string(amqp_exchange)) { message_params.destination_name = ENSURE_PERSISTENCE("Default"); @@ -210,7 +545,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { if (nr_php_is_zval_non_empty_string(amqp_routing_key)) { /* * In PHP 7.x, the following will create a strdup in - * message_params.messaging_destination_routing_key that needs to be freed. + * message_params.messaging_destination_routing_key that needs to be + * freed. */ message_params.messaging_destination_routing_key = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_routing_key)); @@ -230,10 +566,10 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { /* * Now create and end the instrumented segment as a message segment. * - * By this point, it's been determined that this call will be instrumented so - * only create the message segment now, grab the parent segment start time, - * add our message segment attributes/metrics then close the newly created - * message segment. + * By this point, it's been determined that this call will be instrumented + * so only create the message segment now, grab the parent segment start + * time, add our message segment attributes/metrics then close the newly + * created message segment. */ if (NULL == auto_segment) { @@ -243,22 +579,21 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { */ goto end; } - message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); + message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); if (NULL != message_segment) { /* re-use start time from auto_segment started in func_begin */ message_segment->start_time = auto_segment->start_time; - nr_segment_message_end(&message_segment, &message_params); } end: /* - * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, - * now we destroy them. There isn't a separate function to destroy all since - * some of the params are string literals and we don't want to strdup - * everything if we don't have to. RabbitMQ basic_publish PHP 7.x will only - * strdup server_address, destination_name, and + * Because we had to strdup values to persist them beyond + * NR_PHP_WRAPPER_CALL, now we destroy them. There isn't a separate function + * to destroy all since some of the params are string literals and we don't + * want to strdup everything if we don't have to. RabbitMQ basic_publish + * PHP 7.x will only strdup server_address, destination_name, and * messaging_destination_routing_key. */ UNDO_PERSISTENCE(message_params.server_address); @@ -268,6 +603,11 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { NR_PHP_WRAPPER_END /* + * Purpose : A wrapper to instrument the php-amqplib basic_get. This + * retrieves values to populate a message segment. If + * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also + * retrieve the DT headers and, if applicable, apply to the txn. + * * PhpAmqpLib\Channel\AMQPChannel::basic_get * Direct access to a queue if no message was available in the queue, return * null @@ -337,8 +677,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { = Z_STRVAL_P(amqp_exchange); } else { /* - * For consumer, this is exchange name. Exchange name is Default in case - * of empty string. + * For consumer, this is exchange name. Exchange name is Default in + * case of empty string. */ if (nr_php_is_zval_valid_string(amqp_exchange)) { message_params.messaging_destination_publish_name = "Default"; @@ -353,12 +693,14 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { } } + nr_php_amqplib_retrieve_dt_headers(*retval_ptr); + /* Now create and end the instrumented segment as a message segment. */ /* - * By this point, it's been determined that this call will be instrumented so - * only create the message segment now, grab the parent segment start time, - * add our message segment attributes/metrics then close the newly created - * message segment. + * By this point, it's been determined that this call will be instrumented + * so only create the message segment now, grab the parent segment start + * time, add our message segment attributes/metrics then close the newly + * created message segment. */ message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); @@ -373,16 +715,17 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { end: /* - * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, - * now we destroy them. There isn't a separate function to destroy all since - * some of the params are string literals and we don't want to strdup - * everything if we don't have to. RabbitMQ basic_get PHP 7.x will only strdup - * server_address and destination_name. + * Because we had to strdup values to persist them beyond + * NR_PHP_WRAPPER_CALL, now we destroy them. There isn't a separate function + * to destroy all since some of the params are string literals and we don't + * want to strdup everything if we don't have to. RabbitMQ basic_get PHP 7.x + * will only strdup server_address and destination_name. */ UNDO_PERSISTENCE(message_params.server_address); UNDO_PERSISTENCE(message_params.destination_name); } NR_PHP_WRAPPER_END + void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h index 4ca9e05e7..b4e565b40 100644 --- a/agent/lib_php_amqplib.h +++ b/agent/lib_php_amqplib.h @@ -13,7 +13,22 @@ #define AMQP_CONSTRUCT_PARAMS_SERVER_INDEX 0 #define AMQP_CONSTRUCT_PARAMS_PORT_INDEX 1 +/* + * Purpose : Enable the library after detection. + * + * Params : None + * + * Returns : None + */ extern void nr_aws_php_amqplib_enable(); + +/* + * Purpose : Detect the version and create package and metrics. + * + * Params : None + * + * Returns : None + */ extern void nr_php_amqplib_handle_version(); #endif /* LIB_PHP_AMQPLIB */ From 0009a56d2412b3060104d30d04603aa28e005b9d Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 10:43:08 -0700 Subject: [PATCH 17/71] fix(agent): Update docstring --- agent/lib_php_amqplib.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 372ae427d..e8a7e62cf 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -70,13 +70,9 @@ */ /* - * Purpose : Retrieves host and port from an AMQP Connection and sets the - * host/port values in the message_params. + * Purpose : Ensures the php-amqplib instrumentation gets wrapped. * - * Params : 1. PhpAmqpLib\Connection family of connections that inherit from - * AbstractConnection - * 2. nr_segment_message_params_t* message_params that will be - * modified with port and host info, if available + * Params : None * * Returns : None */ From 88c904dc4a857774c78c998af3b579f3ef3652a7 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 15:14:27 -0700 Subject: [PATCH 18/71] style(agent): rename variable --- agent/lib_php_amqplib.c | 44 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index e8a7e62cf..0e552d90b 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -77,17 +77,19 @@ * Returns : None */ static void nr_php_amqplib_ensure_class() { - zval retval_dtor; + zval retval_zpd; int result = FAILURE; +//amber +return; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - &retval_dtor, "Get nr_php_amqplib_class_exists"); + &retval_zpd, "Get nr_php_amqplib_class_exists"); /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. */ - zval_dtor(&retval_dtor); + zval_dtor(&retval_zpd); } /* @@ -100,7 +102,7 @@ static void nr_php_amqplib_ensure_class() { */ void nr_php_amqplib_handle_version() { char* version = NULL; - zval retval_dtor; + zval retval_zpd; int result = FAILURE; result = zend_eval_string( @@ -112,12 +114,12 @@ void nr_php_amqplib_handle_version() { " }" " return $nr_php_amqplib_version;" "})();", - &retval_dtor, "Get nr_php_amqplib_version"); + &retval_zpd, "Get nr_php_amqplib_version"); /* See if we got a non-empty/non-null string for version. */ if (SUCCESS == result) { - if (nr_php_is_zval_non_empty_string(&retval_dtor)) { - version = Z_STRVAL(retval_dtor); + if (nr_php_is_zval_non_empty_string(&retval_zpd)) { + version = Z_STRVAL(retval_zpd); } } @@ -129,7 +131,7 @@ void nr_php_amqplib_handle_version() { nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, version); - zval_dtor(&retval_dtor); + zval_dtor(&retval_zpd); } /* @@ -206,9 +208,9 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { zval* amqp_headers_table = NULL; zval* retval_set_property_zvf = NULL; zval* retval_set_table_zvf = NULL; - zval application_headers_dtor; - zval key_zval_dtor; - zval amqp_table_retval_dtor; + zval application_headers_zpd; + zval key_zval_zpd; + zval amqp_table_retval_zpd; zval* key_exists = NULL; zval* amqp_table_data = NULL; zend_ulong key_num = 0; @@ -255,7 +257,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { /* * Get application+_headers string in zval form for use with nr_php_call */ - ZVAL_STRING(&application_headers_dtor, "application_headers"); + ZVAL_STRING(&application_headers_zpd, "application_headers"); /* * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable @@ -277,10 +279,10 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { " return null;" " }" "})();", - &amqp_table_retval_dtor, "newrelic.amqplib.add_empty_headers"); + &amqp_table_retval_zpd, "newrelic.amqplib.add_empty_headers"); if (FAILURE == retval - || !nr_php_is_zval_valid_object(&amqp_table_retval_dtor)) { + || !nr_php_is_zval_valid_object(&amqp_table_retval_zpd)) { nrl_verbosedebug(NRL_INSTRUMENT, "No application headers in AMQPTable, but couldn't " "create one. Exit."); @@ -291,7 +293,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { * Set the valid AMQPTable on the AMQPMessage. */ retval_set_property_zvf = nr_php_call( - amqp_msg, "set", &application_headers_dtor, &amqp_table_retval_dtor); + amqp_msg, "set", &application_headers_zpd, &amqp_table_retval_zpd); if (NULL == retval_set_property_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "AMQPMessage had no application_headers AMQPTable, but " @@ -347,16 +349,16 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { if (NULL == key_exists) { /* key_str is a zend_string. It needs to be a zval to pass to * nr_php_call. */ - ZVAL_STR_COPY(&key_zval_dtor, key_str); + ZVAL_STR_COPY(&key_zval_zpd, key_str); /* Key doesn't exist, so set the value in the AMQPTable. */ retval_set_table_zvf - = nr_php_call(amqp_headers_table, "set", &key_zval_dtor, val); + = nr_php_call(amqp_headers_table, "set", &key_zval_zpd, val); if (NULL == retval_set_table_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "%s didn't exist in the AMQPTable, but couldn't " "set the key/val to the table.", - NRSAFESTR(Z_STRVAL(key_zval_dtor))); + NRSAFESTR(Z_STRVAL(key_zval_zpd))); } nr_php_zval_free(&retval_set_table_zvf); } @@ -367,9 +369,9 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { end: nr_php_zval_free(&dt_headers_zvf); nr_php_zval_free(&retval_set_property_zvf); - zval_ptr_dtor(&application_headers_dtor); - zval_ptr_dtor(&amqp_table_retval_dtor); - zval_ptr_dtor(&key_zval_dtor); + zval_ptr_dtor(&application_headers_zpd); + zval_ptr_dtor(&amqp_table_retval_zpd); + zval_ptr_dtor(&key_zval_zpd); } /* From 4aa25223a683824afbcfabe17905fd6bdd24314d Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 17:27:07 -0700 Subject: [PATCH 19/71] style(agent): Comment clarification --- agent/lib_php_amqplib.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 0e552d90b..213b08db3 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -79,8 +79,6 @@ static void nr_php_amqplib_ensure_class() { zval retval_zpd; int result = FAILURE; -//amber -return; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", &retval_zpd, "Get nr_php_amqplib_class_exists"); @@ -93,7 +91,7 @@ return; } /* - * Version detection will be called directly from PhpAmqpLib\\Package::VERSION + * Version detection will be called pulled from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not * impossible fatal error if the file/class isn't loaded yet, we need to wrap From b5c7def9055f80ca86695ac6f3fb546d22bec5ba Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 17:34:27 -0700 Subject: [PATCH 20/71] style(agent): Comment update --- agent/lib_php_amqplib.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 213b08db3..788b3524c 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -94,9 +94,11 @@ static void nr_php_amqplib_ensure_class() { * Version detection will be called pulled from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not - * impossible fatal error if the file/class isn't loaded yet, we need to wrap - * the call in a try/catch block and make it a lambda so that we avoid fatal - * errors. + * impossible fatal error if the file/class doesn't exist, we need to wrap + * the call in a try/catch block and make it a lambda so that we avoid errors. + * This won't load the file if it doesn't exist, but by the time this is called, + * the existence of the php-amqplib is a known quantity so calling the following + * lambda will result in the PhpAmqpLib\\Package class being loaded. */ void nr_php_amqplib_handle_version() { char* version = NULL; From 0e6ec907de79c045fd659b95f846bdd9da26fd8b Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 08:46:43 -0700 Subject: [PATCH 21/71] fix(agent): Update comment --- agent/lib_php_amqplib.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 788b3524c..95518b82d 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -56,17 +56,29 @@ * * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for - * the agent to wrap the instrumented functions, there are AWS MQ_BROKER + * the agent to wrap the instrumented functions, with AWS MQ_BROKER * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class - * file does not explicitly load or does not load in time, and the instrumented + * file does not explicitly load and the instrumented * functions are NEVER wrapped regardless of how many times they are called in - * one txn. Specifically, this centered around the very slight but impactful - * differences when using managing the AWS MQ Broker connect vs - * causes an explicit load of the AMQPChannel class/file and - * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load - * of the AMQPChannelclass/file. The following method is thus the only way to - * ensure the class is loaded in time for the functions to be wrapped. + * one txn. + * Specifically, this centered around the very slight but impactful + * differences when using managing the AWS MQ_BROKER connect vs using the + * official RabbitMq Server, and this function is needed ONLY to support AWS's + * MQ_BROKER. * + * When connecting via SSL with rabbitmq's official server is explicitly loaded. + * Hoever, when connecting via SSL with an MQ_BROKER that uses RabbitMQ(using + * the exact same php file and with only changes in the server name for the + * connection), the AMQPChannel file (and therefore class), the AMQPChannel file + * (and therefore class) is NOT explicitly loaded. + * + * Because the very key `PhpAmqpLib/Channel/AMQPChannel.php` file never gets + * explicitly loaded when interacting with the AWS MQ_BROKER, the class is not + * automatically loaded even though it is available and can be resolved if + * called from within PHP. Because of this, the instrumented functions NEVER + * get wrapped when connecting to the MQ_BROKER and therefore the + * instrumentation is never triggered. The explicit loading of the class is + * needed to work with MQ_BROKER. */ /* From 127e62afa3d31e4861d051567c0746357c21f5cd Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 10:21:47 -0700 Subject: [PATCH 22/71] fix(agent): Comment about support. --- agent/php_execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/php_execute.c b/agent/php_execute.c index f8d75628b..41c430d11 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -491,7 +491,7 @@ static nr_library_table_t libraries[] = { {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, - /* php-amqplib RabbitMQ >= 3.7 */ + /* php-amqplib RabbitMQ; PHP Agent supports php-amqplib >= 3.7 */ {"php-amqplib", NR_PSTR("phpamqplib/connection/abstractconnection.php"), nr_php_amqplib_enable}, From a8569a23a1761a960c76ff41440f01b937d5cc00 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 13:13:38 -0700 Subject: [PATCH 23/71] Update agent/lib_php_amqplib.c Co-authored-by: Michal Nowacki --- agent/lib_php_amqplib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 95518b82d..c485ccf0f 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -675,7 +675,7 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { *The retval should be an AMQPMessage. nr_php_is_zval_* ops do NULL checks * as well. */ - if (nr_php_is_zval_valid_object(*retval_ptr)) { + if (NULL != retval_ptr && nr_php_is_zval_valid_object(*retval_ptr)) { /* * Get the exchange and routing key from the AMQPMessage */ From d6f40fafa082543e28cc3f0f3e8b35f1718be0d0 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 14:17:29 -0700 Subject: [PATCH 24/71] fix(agent): belongs inside the block that checked for valid retval --- agent/lib_php_amqplib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index c485ccf0f..91415b0b6 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -701,9 +701,9 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { message_params.messaging_destination_routing_key = Z_STRVAL_P(amqp_routing_key); } - } - nr_php_amqplib_retrieve_dt_headers(*retval_ptr); + nr_php_amqplib_retrieve_dt_headers(*retval_ptr); + } /* Now create and end the instrumented segment as a message segment. */ /* From e27a78e2ca5e5e4ee16dca85de4f2b3c7efea091 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 15:46:22 -0700 Subject: [PATCH 25/71] fix(agent): Expectations needed to be flipped after a previous refactor --- agent/lib_php_amqplib.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 91415b0b6..9365c1dd9 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -172,15 +172,14 @@ static inline void nr_php_amqplib_get_host_and_port( return; } - /* construct params are always saved to use for cloning purposes. */ - if (!nr_php_is_zval_valid_object(amqp_connection)) { return; } + /* construct_params are always saved to use for cloning purposes. */ connect_constructor_params = nr_php_get_zval_object_property(amqp_connection, "construct_params"); - if (nr_php_is_zval_valid_array(connect_constructor_params)) { + if (!nr_php_is_zval_valid_array(connect_constructor_params)) { return; } @@ -194,7 +193,7 @@ static inline void nr_php_amqplib_get_host_and_port( amqp_port = nr_php_zend_hash_index_find( Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (IS_LONG != Z_TYPE_P((amqp_port))) { + if (IS_LONG == Z_TYPE_P((amqp_port))) { message_params->server_port = Z_LVAL_P(amqp_port); } } From 23a8a34bc492bae2e63976440addb80ac7318286 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 16:20:31 -0700 Subject: [PATCH 26/71] fix(agent): unneeded var --- agent/lib_php_amqplib.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 9365c1dd9..a4d5971a2 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -89,17 +89,14 @@ * Returns : None */ static void nr_php_amqplib_ensure_class() { - zval retval_zpd; int result = FAILURE; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - &retval_zpd, "Get nr_php_amqplib_class_exists"); + NULL, "Get nr_php_amqplib_class_exists"); /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. */ - - zval_dtor(&retval_zpd); } /* From 10858da42a449bb5f72f4072f1b2feeae259680a Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 17:50:24 -0700 Subject: [PATCH 27/71] fix(agent): Update comments regarding DT vs w3c header behavior --- agent/lib_php_amqplib.c | 51 +++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a4d5971a2..a30e1e7b1 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -196,9 +196,14 @@ static inline void nr_php_amqplib_get_host_and_port( } /* - * Purpose : Applies DT headers to an inbound AMQPMessage if - * newrelic.distributed_tracing_exclude_newrelic_header INI setting is false and - * if the headers don't already exist on the AMQPMessage. + * Purpose : Applies DT headers to an inbound AMQPMessage. + * Note: + * The DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be added if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. * * Params : PhpAmqpLib\Message\AMQPMessage * @@ -227,8 +232,13 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { int retval = FAILURE; /* - * Note, this functionality can be disabled by toggling the - * newrelic.distributed_tracing_exclude_newrelic_header INI setting. + * Note: + * The DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be added if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. */ /* @@ -253,8 +263,13 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { } /* - * newrelic_get_request_metadata is an internal API that will only return DT - * headers if newrelic.distributed_tracing_exclude_newrelic_header is false. + * newrelic_get_request_metadata is an internal API that will only return the + * DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be returned if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. */ dt_headers_zvf = nr_php_call(NULL, "newrelic_get_request_metadata"); if (!nr_php_is_zval_valid_array(dt_headers_zvf)) { @@ -490,9 +505,15 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { /* * Purpose : A wrapper to instrument the php-amqplib basic_publish. This - * retrieves values to populate a message segment. If - * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also - * insert the DT headers. + * retrieves values to populate a message segment and insert the DT headers, if + * applicable. + * + * Note: The DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be added if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. * * PhpAmqpLib\Channel\AMQPChannel::basic_publish * Publishes a message @@ -610,8 +631,14 @@ NR_PHP_WRAPPER_END /* * Purpose : A wrapper to instrument the php-amqplib basic_get. This - * retrieves values to populate a message segment. If - * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also + * retrieves values to populate a message segment. + * Note: + * The DT header 'newrelic' will only be considered if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be considered if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. If settings are correct, it will * retrieve the DT headers and, if applicable, apply to the txn. * * PhpAmqpLib\Channel\AMQPChannel::basic_get From 89d2937264d6c36dc40eef055e6a0f464d8155c3 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 20:42:06 -0700 Subject: [PATCH 28/71] fix(agent): just bail early from headers if DT isn't on --- agent/lib_php_amqplib.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a30e1e7b1..80d3e85da 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -251,6 +251,10 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { return; } + if (!NRPRG(txn)->options.distributed_tracing_enabled) { + return; + } + amqp_properties_array = nr_php_get_zval_object_property(amqp_msg, "properties"); if (!nr_php_is_zval_valid_array(amqp_properties_array)) { @@ -286,9 +290,9 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable * object */ + amqp_headers_table = nr_php_zend_hash_find(Z_ARRVAL_P(amqp_properties_array), "application_headers"); - /* * If the application_headers AMQPTable object doesn't exist, we'll have to * create it with an empty array. @@ -311,7 +315,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { "create one. Exit."); goto end; } - /* * Set the valid AMQPTable on the AMQPMessage. */ @@ -384,6 +387,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { NRSAFESTR(Z_STRVAL(key_zval_zpd))); } nr_php_zval_free(&retval_set_table_zvf); + zval_ptr_dtor(&key_zval_zpd); } } } @@ -394,7 +398,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { nr_php_zval_free(&retval_set_property_zvf); zval_ptr_dtor(&application_headers_zpd); zval_ptr_dtor(&amqp_table_retval_zpd); - zval_ptr_dtor(&key_zval_zpd); } /* @@ -437,6 +440,10 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { return; } + if (!NRPRG(txn)->options.distributed_tracing_enabled) { + return; + } + amqp_properties_array = nr_php_get_zval_object_property(amqp_msg, "properties"); if (!nr_php_is_zval_valid_array(amqp_properties_array)) { @@ -769,7 +776,7 @@ void nr_php_amqplib_enable() { PHP_PACKAGE_VERSION_UNKNOWN); } - /* Extract the version for aws-sdk 3+ */ + /* Extract the version */ nr_php_amqplib_handle_version(); nr_php_amqplib_ensure_class(); From 90da3b7c75ffb66a1d1548f89231d44f36edf022 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 21:28:51 -0700 Subject: [PATCH 29/71] fix(agent): use zval is integer function --- agent/lib_php_amqplib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 80d3e85da..92407fde1 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -190,7 +190,7 @@ static inline void nr_php_amqplib_get_host_and_port( amqp_port = nr_php_zend_hash_index_find( Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (IS_LONG == Z_TYPE_P((amqp_port))) { + if (nr_php_is_zval_valid_integer(amqp_port)) { message_params->server_port = Z_LVAL_P(amqp_port); } } From f160b3b7b1cf2a88bf6e8a2b0a975adf03273e1f Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 09:11:16 -0700 Subject: [PATCH 30/71] fix(agent): Move dtors closer to where the values are initialized --- agent/lib_php_amqplib.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 92407fde1..bfc49fdf9 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -281,11 +281,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { return; } - /* - * Get application+_headers string in zval form for use with nr_php_call - */ - ZVAL_STRING(&application_headers_zpd, "application_headers"); - /* * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable * object @@ -308,18 +303,32 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { "})();", &amqp_table_retval_zpd, "newrelic.amqplib.add_empty_headers"); - if (FAILURE == retval - || !nr_php_is_zval_valid_object(&amqp_table_retval_zpd)) { + if (FAILURE == retval) { + nrl_verbosedebug(NRL_INSTRUMENT, + "No application headers in AMQPTable, but couldn't " + "create one. Exit."); + goto end; + } + if (!nr_php_is_zval_valid_object(&amqp_table_retval_zpd)) { nrl_verbosedebug(NRL_INSTRUMENT, "No application headers in AMQPTable, but couldn't " "create one. Exit."); + zval_ptr_dtor(&amqp_table_retval_zpd); goto end; } + /* + * Get application+_headers string in zval form for use with nr_php_call + */ + ZVAL_STRING(&application_headers_zpd, "application_headers"); /* * Set the valid AMQPTable on the AMQPMessage. */ retval_set_property_zvf = nr_php_call( amqp_msg, "set", &application_headers_zpd, &amqp_table_retval_zpd); + + zval_ptr_dtor(&application_headers_zpd); + zval_ptr_dtor(&amqp_table_retval_zpd); + if (NULL == retval_set_property_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "AMQPMessage had no application_headers AMQPTable, but " @@ -396,8 +405,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { end: nr_php_zval_free(&dt_headers_zvf); nr_php_zval_free(&retval_set_property_zvf); - zval_ptr_dtor(&application_headers_zpd); - zval_ptr_dtor(&amqp_table_retval_zpd); } /* From 6e059475a8575f9f9aa192aeaf9b6c559d0788ab Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 09:31:56 -0700 Subject: [PATCH 31/71] fix(agent): add dtor closer to init --- agent/lib_php_amqplib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index bfc49fdf9..bcd01a608 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -382,21 +382,21 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { key_exists = nr_php_zend_hash_find(HASH_OF(amqp_table_data), ZSTR_VAL(key_str)); if (NULL == key_exists) { + /* Key doesn't exist, so set the value in the AMQPTable. */ + /* key_str is a zend_string. It needs to be a zval to pass to * nr_php_call. */ ZVAL_STR_COPY(&key_zval_zpd, key_str); - /* Key doesn't exist, so set the value in the AMQPTable. */ retval_set_table_zvf = nr_php_call(amqp_headers_table, "set", &key_zval_zpd, val); - + zval_ptr_dtor(&key_zval_zpd); if (NULL == retval_set_table_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "%s didn't exist in the AMQPTable, but couldn't " "set the key/val to the table.", - NRSAFESTR(Z_STRVAL(key_zval_zpd))); + NRSAFESTR(ZSTR_VAL(key_str))); } nr_php_zval_free(&retval_set_table_zvf); - zval_ptr_dtor(&key_zval_zpd); } } } From cd431eca15ae3d7d72d4a3dfb3cb491262ee0c51 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 20:02:25 -0700 Subject: [PATCH 32/71] fix(agent): Calling order --- agent/lib_php_amqplib.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index bcd01a608..9b15ab3c3 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -389,13 +389,13 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { ZVAL_STR_COPY(&key_zval_zpd, key_str); retval_set_table_zvf = nr_php_call(amqp_headers_table, "set", &key_zval_zpd, val); - zval_ptr_dtor(&key_zval_zpd); if (NULL == retval_set_table_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "%s didn't exist in the AMQPTable, but couldn't " "set the key/val to the table.", NRSAFESTR(ZSTR_VAL(key_str))); } + zval_ptr_dtor(&key_zval_zpd); nr_php_zval_free(&retval_set_table_zvf); } } @@ -510,6 +510,7 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { */ nr_php_api_accept_distributed_trace_payload_httpsafe(NRPRG(txn), header_map, "Queue"); + nr_hashmap_destroy(&header_map); } nr_php_zval_free(&amqp_headers_native_data_zvf); @@ -543,8 +544,20 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { * @throws AMQPConnectionBlockedException * */ -NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { + +NR_PHP_WRAPPER(nr_rabbitmq_basic_publish_before) { zval* amqp_msg = NULL; + (void)wraprec; + + amqp_msg = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); + /* + * nr_php_amqplib_insert_dt_headers will check the validity of the object. + */ + nr_php_amqplib_insert_dt_headers(amqp_msg); +} +NR_PHP_WRAPPER_END + +NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { zval* amqp_exchange = NULL; zval* amqp_routing_key = NULL; zval* amqp_connection = NULL; @@ -559,11 +572,15 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { (void)wraprec; +#if ZEND_MODULE_API_NO < ZEND_8_0_X_API_NO /* PHP8.0+ */ + zval* amqp_msg = NULL; amqp_msg = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); /* * nr_php_amqplib_insert_dt_headers will check the validity of the object. */ nr_php_amqplib_insert_dt_headers(amqp_msg); +#endif + amqp_exchange = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); if (nr_php_is_zval_non_empty_string(amqp_exchange)) { /* @@ -789,8 +806,9 @@ void nr_php_amqplib_enable() { #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* less than PHP8.0 */ nr_php_wrap_user_function_before_after_clean( - NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), NULL, - nr_rabbitmq_basic_publish, nr_rabbitmq_basic_publish); + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), + nr_rabbitmq_basic_publish_before, nr_rabbitmq_basic_publish, + nr_rabbitmq_basic_publish); nr_php_wrap_user_function_before_after_clean( NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_get"), NULL, From ebb6ff068487ed4b22517d4736d2cb06e35af7ee Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 17 Jan 2025 17:12:22 -0700 Subject: [PATCH 33/71] feat(agent): Add RabbitMQ instrumentation using the php-amqplib library Initial commit does the following: * Detect library via magic file * Detect package and version information. * Basic unit tests --- agent/Makefile.frag | 1 + agent/config.m4 | 2 +- agent/fw_hooks.h | 1 + agent/lib_php_amqplib.c | 82 +++++++++++++++++++++ agent/lib_php_amqplib.h | 13 ++++ agent/php_execute.c | 4 ++ agent/tests/test_lib_php_amqplib.c | 110 +++++++++++++++++++++++++++++ 7 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 agent/lib_php_amqplib.c create mode 100644 agent/lib_php_amqplib.h create mode 100644 agent/tests/test_lib_php_amqplib.c diff --git a/agent/Makefile.frag b/agent/Makefile.frag index fbff46d69..e10d6d0f8 100644 --- a/agent/Makefile.frag +++ b/agent/Makefile.frag @@ -93,6 +93,7 @@ TEST_BINARIES = \ tests/test_internal_instrument \ tests/test_hash \ tests/test_lib_aws_sdk_php \ + tests/test_lib_php_ampqlib \ tests/test_memcached \ tests/test_mongodb \ tests/test_monolog \ diff --git a/agent/config.m4 b/agent/config.m4 index 6671bcd54..19785b2a3 100644 --- a/agent/config.m4 +++ b/agent/config.m4 @@ -231,7 +231,7 @@ if test "$PHP_NEWRELIC" = "yes"; then LIBRARIES="lib_aws_sdk_php.c lib_monolog.c lib_doctrine2.c lib_guzzle3.c \ lib_guzzle4.c lib_guzzle6.c lib_guzzle_common.c \ lib_mongodb.c lib_phpunit.c lib_predis.c lib_zend_http.c \ - lib_composer.c" + lib_composer.c lib_php_amqplib.c" PHP_NEW_EXTENSION(newrelic, $FRAMEWORKS $LIBRARIES $NEWRELIC_AGENT, $ext_shared,, $(NEWRELIC_CFLAGS)) PHP_SUBST(NEWRELIC_CFLAGS) diff --git a/agent/fw_hooks.h b/agent/fw_hooks.h index 711c3b618..93665862c 100644 --- a/agent/fw_hooks.h +++ b/agent/fw_hooks.h @@ -46,6 +46,7 @@ extern void nr_guzzle4_enable(TSRMLS_D); extern void nr_guzzle6_enable(TSRMLS_D); extern void nr_laminas_http_enable(TSRMLS_D); extern void nr_mongodb_enable(TSRMLS_D); +extern void nr_php_amqplib_enable(); extern void nr_phpunit_enable(TSRMLS_D); extern void nr_predis_enable(TSRMLS_D); extern void nr_zend_http_enable(TSRMLS_D); diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c new file mode 100644 index 000000000..07af6d959 --- /dev/null +++ b/agent/lib_php_amqplib.c @@ -0,0 +1,82 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +/* + * Functions relating to instrumenting the AWS-SDK-PHP. + * https://github.com/aws/aws-sdk-php + */ +#include "php_agent.h" +#include "php_call.h" +#include "php_hash.h" +#include "php_wrapper.h" +#include "fw_hooks.h" +#include "fw_support.h" +#include "util_logging.h" +#include "lib_php_amqplib.h" + +#define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" + +/* + * nr_php_amqplib_handle_version will automatically load the class if it isn't + * loaded yet and then evaluate the string. To avoid the VERY unlikely but not + * impossible fatal error if the file/class isn't loaded yet, we need to wrap + * the call in a try/catch block and make it a lambda so that we avoid fatal + * errors. + */ +void nr_php_amqplib_handle_version() { + char* version = NULL; + zval retval; + int result = FAILURE; + + result = zend_eval_string( + "(function() {" + " $nr_php_amqplib_version = '';" + " try {" + " $nr_php_amqplib_version = PhpAmqpLib\\Package::VERSION;" + " } catch (Throwable $e) {" + " }" + " return $nr_php_amqplib_version;" + "})();", + &retval, "Get nr_php_amqplib_version"); + + /* See if we got a non-empty/non-null string for version. */ + if (SUCCESS == result) { + if (nr_php_is_zval_non_empty_string(&retval)) { + version = Z_STRVAL(retval); + } + } + + if (NRINI(vulnerability_management_package_detection_enabled)) { + /* Add php package to transaction */ + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); + } + + nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, + version); + + zval_dtor(&retval); +} + +/* + * + * Version detection will be called directly from Aws\Sdk.php + */ +void nr_php_amqplib_enable() { + /* + * Set the UNKNOWN package first, so it doesn't overwrite what we find with + * nr_lib_aws_sdk_php_handle_version. + */ + if (NRINI(vulnerability_management_package_detection_enabled)) { + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, + PHP_PACKAGE_VERSION_UNKNOWN); + } + + /* Extract the version for aws-sdk 3+ */ + nr_php_amqplib_handle_version(); + + /* Called when initializing all Clients */ + // nr_php_wrap_user_function(NR_PSTR("Aws\\AwsClient::parseClass"), + // nr_create_aws_service_metric); +} diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h new file mode 100644 index 000000000..6942c827b --- /dev/null +++ b/agent/lib_php_amqplib.h @@ -0,0 +1,13 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Functions relating to instrumenting AWS-SDK-PHP. + */ +#ifndef LIB_PHP_AMQPLIB +#define LIB_PHP_AMQPLIB + +extern void nr_aws_php_amqplib_enable(); +extern void nr_php_amqplib_handle_version(); + +#endif /* LIB_PHP_AMQPLIB */ diff --git a/agent/php_execute.c b/agent/php_execute.c index 4a8765f41..f8d75628b 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -491,6 +491,10 @@ static nr_library_table_t libraries[] = { {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, + /* php-amqplib RabbitMQ >= 3.7 */ + {"php-amqplib", NR_PSTR("phpamqplib/connection/abstractconnection.php"), + nr_php_amqplib_enable}, + /* * The first path is for Composer installs, the second is for * /usr/local/bin. diff --git a/agent/tests/test_lib_php_amqplib.c b/agent/tests/test_lib_php_amqplib.c new file mode 100644 index 000000000..723c4089d --- /dev/null +++ b/agent/tests/test_lib_php_amqplib.c @@ -0,0 +1,110 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "tlib_php.h" + +#include "php_agent.h" +#include "lib_php_amqplib.h" +#include "fw_support.h" + +tlib_parallel_info_t parallel_info + = {.suggested_nthreads = -1, .state_size = 0}; + +#if ZEND_MODULE_API_NO > ZEND_7_1_X_API_NO + +static void declare_php_amqplib_package_class(const char* ns, + const char* klass, + const char* sdk_version) { + char* source = nr_formatf( + "namespace %s;" + "class %s{" + "const VERSION = '%s';" + "}", + ns, klass, sdk_version); + + tlib_php_request_eval(source); + + nr_free(source); +} + +static void test_nr_lib_php_amqplib_handle_version(void) { +#define LIBRARY_NAME "php-amqplib/php-amqplib" + const char* library_versions[] + = {"7", "10", "100", "4.23", "55.34", "6123.45", "0.4.5"}; + nr_php_package_t* p = NULL; +#define TEST_DESCRIPTION_FMT \ + "nr_lib_php_amqplib_handle_version with library_versions[%ld]=%s: package " \ + "major version metric - %s" + char* test_description = NULL; + size_t i = 0; + + /* + * If lib_php_amqplib_handle_version function is ever called, we have already + * detected the php-amqplib library. + */ + + /* + * PhpAmqpLib/Package class exists. Should create php-amqplib package metric + * suggestion with version + */ + for (i = 0; i < sizeof(library_versions) / sizeof(library_versions[0]); i++) { + tlib_php_request_start(); + + declare_php_amqplib_package_class("PhpAmqpLib", "Package", + library_versions[i]); + nr_lib_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, + LIBRARY_NAME); + + test_description = nr_formatf(TEST_DESCRIPTION_FMT, i, library_versions[i], + "suggestion created"); + tlib_pass_if_not_null(test_description, p); + nr_free(test_description); + + test_description = nr_formatf(TEST_DESCRIPTION_FMT, i, library_versions[i], + "suggested version set"); + tlib_pass_if_str_equal(test_description, library_versions[i], + p->package_version); + nr_free(test_description); + + tlib_php_request_end(); + } + + /* + * PhpAmqpLib/Package class does not exist, should create package metric + * suggestion with PHP_PACKAGE_VERSION_UNKNOWN version. This case should never + * happen in real situations. + */ + tlib_php_request_start(); + + nr_lib_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); + + tlib_pass_if_not_null( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is not " + "defined - " + "suggestion created", + p); + tlib_pass_if_str_equal( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is not " + "defined - " + "suggested version set to PHP_PACKAGE_VERSION_UNKNOWN", + PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); + + tlib_php_request_end(); +} + +void test_main(void* p NRUNUSED) { + tlib_php_engine_create(""); + test_nr_lib_php_amqplib_handle_version(); + tlib_php_engine_destroy(); +} +#else +void test_main(void* p NRUNUSED) {} +#endif From 96f15aa4a187a70cda6c90408c9ba711186d4ffc Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 11:57:17 -0700 Subject: [PATCH 34/71] fix(agent): Typo remove AWS references --- agent/lib_php_amqplib.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 07af6d959..ec0a05c74 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -4,8 +4,8 @@ */ /* - * Functions relating to instrumenting the AWS-SDK-PHP. - * https://github.com/aws/aws-sdk-php + * Functions relating to instrumenting the php-ampqlib + * https://github.com/php-amqplib/php-amqplib */ #include "php_agent.h" #include "php_call.h" @@ -19,6 +19,7 @@ #define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" /* + * Version detection will be called directly from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not * impossible fatal error if the file/class isn't loaded yet, we need to wrap @@ -59,14 +60,10 @@ void nr_php_amqplib_handle_version() { zval_dtor(&retval); } -/* - * - * Version detection will be called directly from Aws\Sdk.php - */ void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with - * nr_lib_aws_sdk_php_handle_version. + * nr_php_amqplib_handle_version. */ if (NRINI(vulnerability_management_package_detection_enabled)) { nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, @@ -75,8 +72,4 @@ void nr_php_amqplib_enable() { /* Extract the version for aws-sdk 3+ */ nr_php_amqplib_handle_version(); - - /* Called when initializing all Clients */ - // nr_php_wrap_user_function(NR_PSTR("Aws\\AwsClient::parseClass"), - // nr_create_aws_service_metric); } From 33765b228e6ff7947c584900f28bd067d87f08b4 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 13:10:57 -0700 Subject: [PATCH 35/71] fix(agent): typo --- agent/Makefile.frag | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/Makefile.frag b/agent/Makefile.frag index e10d6d0f8..2bfc562a8 100644 --- a/agent/Makefile.frag +++ b/agent/Makefile.frag @@ -93,7 +93,7 @@ TEST_BINARIES = \ tests/test_internal_instrument \ tests/test_hash \ tests/test_lib_aws_sdk_php \ - tests/test_lib_php_ampqlib \ + tests/test_lib_php_amqplib \ tests/test_memcached \ tests/test_mongodb \ tests/test_monolog \ From 90c8aa92e709dec6789eab93d71338098812fc92 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 13:21:51 -0700 Subject: [PATCH 36/71] feat(agent): Add additional unit test Test version detection when class exists but version const doesn't. Fixed typos. --- agent/tests/test_lib_php_amqplib.c | 42 +++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/agent/tests/test_lib_php_amqplib.c b/agent/tests/test_lib_php_amqplib.c index 723c4089d..c0261cfc2 100644 --- a/agent/tests/test_lib_php_amqplib.c +++ b/agent/tests/test_lib_php_amqplib.c @@ -16,13 +16,13 @@ tlib_parallel_info_t parallel_info static void declare_php_amqplib_package_class(const char* ns, const char* klass, - const char* sdk_version) { + const char* package_version) { char* source = nr_formatf( "namespace %s;" "class %s{" "const VERSION = '%s';" "}", - ns, klass, sdk_version); + ns, klass, package_version); tlib_php_request_eval(source); @@ -54,7 +54,7 @@ static void test_nr_lib_php_amqplib_handle_version(void) { declare_php_amqplib_package_class("PhpAmqpLib", "Package", library_versions[i]); - nr_lib_php_amqplib_handle_version(); + nr_php_amqplib_handle_version(); p = nr_php_packages_get_package( NRPRG(txn)->php_package_major_version_metrics_suggestions, @@ -81,7 +81,7 @@ static void test_nr_lib_php_amqplib_handle_version(void) { */ tlib_php_request_start(); - nr_lib_php_amqplib_handle_version(); + nr_php_amqplib_handle_version(); p = nr_php_packages_get_package( NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); @@ -98,6 +98,40 @@ static void test_nr_lib_php_amqplib_handle_version(void) { PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); tlib_php_request_end(); + + /* + * PhpAmqpLib\\Package class exists but VERSION does not. + * Should create package metric suggestion with PHP_PACKAGE_VERSION_UNKNOWN + * version. This case should never happen in real situations. + */ + tlib_php_request_start(); + + char* source + = "namespace PhpAmqpLib;" + "class Package{" + "const SADLY_DEPRECATED = 5.4;" + "}"; + + tlib_php_request_eval(source); + + nr_php_amqplib_handle_version(); + + p = nr_php_packages_get_package( + NRPRG(txn)->php_package_major_version_metrics_suggestions, LIBRARY_NAME); + + tlib_pass_if_not_null( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is SET " + "but the const VERSION does not exist - " + "suggestion created", + p); + tlib_pass_if_str_equal( + "nr_lib_php_amqplib_handle_version when PhpAmqpLib\\Package class is SET " + "but the const VERSION does not exist - " + "defined - " + "suggested version set to PHP_PACKAGE_VERSION_UNKNOWN", + PHP_PACKAGE_VERSION_UNKNOWN, p->package_version); + + tlib_php_request_end(); } void test_main(void* p NRUNUSED) { From 32a46a19d2ef3140e293101a5aa754583027fc30 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 23 Jan 2025 13:49:30 -0700 Subject: [PATCH 37/71] chore(agent): Clarifying comment. --- agent/lib_aws_sdk_php.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agent/lib_aws_sdk_php.c b/agent/lib_aws_sdk_php.c index 1bd144cca..46a71ce3c 100644 --- a/agent/lib_aws_sdk_php.c +++ b/agent/lib_aws_sdk_php.c @@ -429,6 +429,12 @@ void nr_lib_aws_sdk_php_handle_version() { zval retval; int result = FAILURE; + /* + * The following block initializes nr_aws_sdk_version to the empty string. + * If it is able to extract the version, nr_aws_sdk_version is set to that. + * Nothing is needed in the catch block. + * The final return will either return a proper version or an empty string. + */ result = zend_eval_string( "(function() {" " $nr_aws_sdk_version = '';" From ba1ab3f137c40b45161bd00c41734c3c3dbf601e Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 24 Jan 2025 17:49:44 -0700 Subject: [PATCH 38/71] feat(axiom): Update message segment to handle rabbitmq --- axiom/nr_segment.c | 12 ++ axiom/nr_segment.h | 13 ++ axiom/nr_segment_message.c | 5 + axiom/nr_segment_message.h | 14 ++ axiom/nr_segment_private.c | 2 + axiom/nr_segment_traces.c | 10 ++ axiom/nr_span_event.c | 71 ++++++++++ axiom/nr_span_event.h | 23 +++- axiom/nr_span_event_private.h | 3 + axiom/tests/test_segment_message.c | 206 +++++++++++++++++++++++++++++ axiom/tests/test_span_event.c | 73 ++++++++-- 11 files changed, 423 insertions(+), 9 deletions(-) diff --git a/axiom/nr_segment.c b/axiom/nr_segment.c index 4aaf7c120..ade63d454 100644 --- a/axiom/nr_segment.c +++ b/axiom/nr_segment.c @@ -331,6 +331,15 @@ static void nr_populate_message_spans(nr_span_event_t* span_event, segment->typed_attributes->message.messaging_system); nr_span_event_set_message(span_event, NR_SPAN_MESSAGE_SERVER_ADDRESS, segment->typed_attributes->message.server_address); + nr_span_event_set_message( + span_event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, + segment->typed_attributes->message.messaging_destination_routing_key); + nr_span_event_set_message( + span_event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME, + segment->typed_attributes->message.messaging_destination_publish_name); + nr_span_event_set_message_ulong( + span_event, NR_SPAN_MESSAGE_SERVER_PORT, + segment->typed_attributes->message.server_port); } static nr_status_t add_user_attribute_to_span_event(const char* key, @@ -640,7 +649,10 @@ bool nr_segment_set_message(nr_segment_t* segment, .message_action = message->message_action, .destination_name = nr_strempty(message->destination_name) ? NULL: nr_strdup(message->destination_name), .messaging_system = nr_strempty(message->messaging_system) ? NULL: nr_strdup(message->messaging_system), + .messaging_destination_routing_key = nr_strempty(message->messaging_destination_routing_key) ? NULL: nr_strdup(message->messaging_destination_routing_key), + .messaging_destination_publish_name = nr_strempty(message->messaging_destination_publish_name) ? NULL: nr_strdup(message->messaging_destination_publish_name), .server_address = nr_strempty(message->server_address) ? NULL: nr_strdup(message->server_address), + .server_port = message->server_port, }; // clang-format on diff --git a/axiom/nr_segment.h b/axiom/nr_segment.h index f35ab6224..646f11e5f 100644 --- a/axiom/nr_segment.h +++ b/axiom/nr_segment.h @@ -128,6 +128,19 @@ typedef struct _nr_segment_message_t { char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/ char* server_address; /*The server domain name or IP address. Needed for MQBROKER relationship.*/ + char* + messaging_destination_publish_name; /* Otel attribute for message + consumers. (In the agent, this + means Action is Consume in the span + name). This attribute is equal to + the corresponding attribute + messaging.destination.name from the + producer. This attribute is needed + for apps using RabbitMQ and it + represents the exchange name.*/ + char* messaging_destination_routing_key; /* The routing key for a RabbitMQ + operation.*/ + uint64_t server_port; /*The server port.*/ } nr_segment_message_t; typedef struct _nr_segment_cloud_attrs_t { diff --git a/axiom/nr_segment_message.c b/axiom/nr_segment_message.c index 92f8babd1..2c8207138 100644 --- a/axiom/nr_segment_message.c +++ b/axiom/nr_segment_message.c @@ -39,6 +39,11 @@ static void nr_segment_message_set_attrs( message_attributes.destination_name = params->destination_name; message_attributes.messaging_system = params->messaging_system; message_attributes.server_address = params->server_address; + message_attributes.messaging_destination_routing_key + = params->messaging_destination_routing_key; + message_attributes.messaging_destination_publish_name + = params->messaging_destination_publish_name; + message_attributes.server_port = params->server_port; } nr_segment_set_message(segment, &message_attributes); diff --git a/axiom/nr_segment_message.h b/axiom/nr_segment_message.h index 2104e3f35..70580697b 100644 --- a/axiom/nr_segment_message.h +++ b/axiom/nr_segment_message.h @@ -38,6 +38,20 @@ typedef struct { char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/ char* server_address; /*The server domain name or IP address. Needed for MQBROKER relationship.*/ + char* + messaging_destination_publish_name; /* Otel attribute for message + consumers. (In the agent, this + means Action is Consume in the span + name). This attribute is equal to + the corresponding attribute + messaging.destination.name from the + producer. This attribute is needed + for apps using RabbitMQ and it + represents the exchange name.*/ + char* messaging_destination_routing_key; /* The routing key for a RabbitMQ + operation.*/ + uint64_t server_port; /*The server port.*/ + } nr_segment_message_params_t; /* diff --git a/axiom/nr_segment_private.c b/axiom/nr_segment_private.c index f063862b7..ea5bbf8cc 100644 --- a/axiom/nr_segment_private.c +++ b/axiom/nr_segment_private.c @@ -47,6 +47,8 @@ void nr_segment_message_destroy_fields(nr_segment_message_t* message) { nr_free(message->destination_name); nr_free(message->messaging_system); nr_free(message->server_address); + nr_free(message->messaging_destination_publish_name); + nr_free(message->messaging_destination_routing_key); } void nr_segment_destroy_typed_attributes( diff --git a/axiom/nr_segment_traces.c b/axiom/nr_segment_traces.c index 153ccd5c2..d2f1fddac 100644 --- a/axiom/nr_segment_traces.c +++ b/axiom/nr_segment_traces.c @@ -170,6 +170,16 @@ static void add_typed_attributes_to_buffer(nrbuf_t* buf, message->messaging_system, false); add_hash_key_value_to_buffer(buf, "server_address", message->server_address, false); + add_hash_key_value_to_buffer(buf, "messaging_destination_publish_name", + message->messaging_destination_publish_name, + false); + add_hash_key_value_to_buffer(buf, "messaging_destination_routing_key", + message->messaging_destination_routing_key, + false); + if (0 != message->server_port) { + add_hash_key_value_to_buffer_int(buf, "server_port", + &message->server_port); + } } break; case NR_SEGMENT_CUSTOM: default: diff --git a/axiom/nr_span_event.c b/axiom/nr_span_event.c index 00987881a..7c8c2dbe0 100644 --- a/axiom/nr_span_event.c +++ b/axiom/nr_span_event.c @@ -377,6 +377,42 @@ void nr_span_event_set_message(nr_span_event_t* event, nro_set_hash_string(event->agent_attributes, NR_ATTR_SERVER_ADDRESS, new_value); break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + nro_set_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_ROUTING_KEY, new_value); + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + nro_set_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_PUBLISH_NAME, + new_value); + break; + case NR_SPAN_MESSAGE_SERVER_PORT: + break; + } +} + +void nr_span_event_set_message_ulong(nr_span_event_t* event, + nr_span_event_message_member_t member, + const uint64_t new_value) { + if (NULL == event || 0 == new_value) { + return; + } + + switch (member) { + case NR_SPAN_MESSAGE_SERVER_PORT: + nro_set_hash_ulong(event->agent_attributes, NR_ATTR_SERVER_PORT, + new_value); + break; + case NR_SPAN_MESSAGE_DESTINATION_NAME: + break; + case NR_SPAN_MESSAGE_MESSAGING_SYSTEM: + break; + case NR_SPAN_MESSAGE_SERVER_ADDRESS: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + break; } } @@ -535,10 +571,45 @@ const char* nr_span_event_get_message(const nr_span_event_t* event, case NR_SPAN_MESSAGE_SERVER_ADDRESS: return nro_get_hash_string(event->agent_attributes, NR_ATTR_SERVER_ADDRESS, NULL); + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + return nro_get_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_ROUTING_KEY, + NULL); + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + return nro_get_hash_string(event->agent_attributes, + NR_ATTR_MESSAGING_DESTINATION_PUBLISH_NAME, + NULL); + case NR_SPAN_MESSAGE_SERVER_PORT: + break; } return NULL; } +uint64_t nr_span_event_get_message_ulong( + const nr_span_event_t* event, + nr_span_event_message_member_t member) { + if (NULL == event) { + return 0; + } + + switch (member) { + case NR_SPAN_MESSAGE_SERVER_PORT: + return nro_get_hash_ulong(event->agent_attributes, NR_ATTR_SERVER_PORT, + NULL); + case NR_SPAN_MESSAGE_DESTINATION_NAME: + break; + case NR_SPAN_MESSAGE_MESSAGING_SYSTEM: + break; + case NR_SPAN_MESSAGE_SERVER_ADDRESS: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY: + break; + case NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME: + break; + } + return 0; +} + void nr_span_event_set_attribute_user(nr_span_event_t* event, const char* name, const nrobj_t* value) { diff --git a/axiom/nr_span_event.h b/axiom/nr_span_event.h index 2a87b2306..33dced5c2 100644 --- a/axiom/nr_span_event.h +++ b/axiom/nr_span_event.h @@ -15,7 +15,12 @@ #define NR_ATTR_MESSAGING_DESTINATION_NAME "messaging.destination.name" #define NR_ATTR_MESSAGING_SYSTEM "messaging.system" +#define NR_ATTR_MESSAGING_DESTINATION_ROUTING_KEY \ + "messaging.rabbitmq.destination.routing_key" +#define NR_ATTR_MESSAGING_DESTINATION_PUBLISH_NAME \ + "messaging.destination_publish.name" #define NR_ATTR_SERVER_ADDRESS "server.address" +#define NR_ATTR_SERVER_PORT "server.port" #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" @@ -80,6 +85,9 @@ typedef enum { NR_SPAN_MESSAGE_DESTINATION_NAME, NR_SPAN_MESSAGE_MESSAGING_SYSTEM, NR_SPAN_MESSAGE_SERVER_ADDRESS, + NR_SPAN_MESSAGE_SERVER_PORT, + NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, + NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME } nr_span_event_message_member_t; /* @@ -211,7 +219,7 @@ extern void nr_span_event_set_external_status(nr_span_event_t* event, const uint64_t status); /* - * Purpose : Set a message attribute. + * Purpose : Set a message attribute with a given string new_value. * * Params : 1. The target Span Event that should be changed. * 2. The message attribute to be set. @@ -222,6 +230,19 @@ extern void nr_span_event_set_message(nr_span_event_t* event, nr_span_event_message_member_t member, const char* new_value); +/* + * Purpose : Set a message attribute with a given ulong new_value. + * + * Params : 1. The target Span Event that should be changed. + * 2. The message attribute to be set. + * 3. The ulong value that the field will be after the function has + * executed. + */ +extern void nr_span_event_set_message_ulong( + nr_span_event_t* event, + nr_span_event_message_member_t member, + const uint64_t new_value); + /* * Purpose : Set a user attribute. * diff --git a/axiom/nr_span_event_private.h b/axiom/nr_span_event_private.h index 4f55c6f2f..01d544fc2 100644 --- a/axiom/nr_span_event_private.h +++ b/axiom/nr_span_event_private.h @@ -48,6 +48,9 @@ extern uint64_t nr_span_event_get_external_status(const nr_span_event_t* event); extern const char* nr_span_event_get_message( const nr_span_event_t* event, nr_span_event_message_member_t member); +extern uint64_t nr_span_event_get_message_ulong( + const nr_span_event_t* event, + nr_span_event_message_member_t member); extern const char* nr_span_event_get_error_message( const nr_span_event_t* event); extern const char* nr_span_event_get_error_class(const nr_span_event_t* event); diff --git a/axiom/tests/test_segment_message.c b/axiom/tests/test_segment_message.c index a68403caf..9c1d3a293 100644 --- a/axiom/tests/test_segment_message.c +++ b/axiom/tests/test_segment_message.c @@ -31,6 +31,9 @@ typedef struct { const char* cloud_resource_id; const char* server_address; const char* aws_operation; + char* messaging_destination_publish_name; + char* messaging_destination_routing_key; + uint64_t server_port; } segment_message_expecteds_t; static nr_segment_t* mock_txn_segment(void) { @@ -927,6 +930,194 @@ static void test_segment_message_aws_operation(void) { .aws_operation = "sendMessage"}); } +static void test_segment_message_server_port(void) { + /* + * server port values should NOT impact the creation + * of metrics. + */ + + /* Test server port not set, implicitly unset */ + test_message_segment( + &(nr_segment_message_params_t){ + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "server port not set, implicitly unset", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .server_port = 0}); + + /* Test server port explicitly set to 0 (unset) */ + test_message_segment( + &(nr_segment_message_params_t){ + .server_port = 0, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "server port explicitly set to 0 (unset)", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .server_port = 0}); + + /* Test valid server_port */ + test_message_segment( + &(nr_segment_message_params_t){ + .server_port = 1234, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "Test valid aws_operation", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .server_port = 1234}); +} + +static void test_segment_messaging_destination_publishing_name(void) { + /* + * messaging_destination_publish_name values should NOT impact the creation + * of metrics. + */ + + /* Test messaging_destination_publish_name is NULL */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_publish_name = NULL, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_publish_name is NULL, attribute " + "should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_publish_name = NULL}); + + /* Test destination_publishing_name is empty string */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_publish_name = "", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_publish_name is empty string, " + "attribute should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_publish_name = NULL}); + + /* Test valid messaging_destination_publish_name */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_publish_name = "publish_name", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "Test valid messaging_destination_publish_name is " + "non-empty string, attribute should be the string", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_publish_name = "publish_name"}); +} + +static void test_segment_messaging_destination_routing_key(void) { + /* + * messaging_destination_routing_key values should NOT impact the creation + * of metrics. + */ + + /* Test messaging_destination_routing_key is NULL */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_routing_key = NULL, + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_routing_key is NULL, attribute " + "should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_routing_key = NULL}); + + /* Test messaging_destination_routing_key is empty string */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "messaging_destination_routing_key is empty string, " + "attribute should be NULL", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_routing_key = NULL}); + + /* Test valid messaging_destination_routing_key */ + test_message_segment( + &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "key to the kingdom", + .library = "SQS", + .message_action = NR_SPANKIND_PRODUCER, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_TOPIC, + .destination_name = "my_destination"}, + &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, + (segment_message_expecteds_t){ + .test_name = "Test valid messaging_destination_routing_key is " + "non-empty string, attribute should be the string", + .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .txn_rollup_metric = "MessageBroker/all", + .library_metric = "MessageBroker/SQS/all", + .num_metrics = 1, + .destination_name = "my_destination", + .messaging_destination_routing_key = "key to the kingdom"}); +} + static void test_segment_message_parameters_enabled(void) { /* * Attributes should be set based on value of parameters_enabled. @@ -935,6 +1126,9 @@ static void test_segment_message_parameters_enabled(void) { /* Test true message_parameters_enabled */ test_message_segment( &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "key to the kingdom", + .server_port = 1234, + .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", @@ -958,6 +1152,9 @@ static void test_segment_message_parameters_enabled(void) { .messaging_system = "my_system", .cloud_resource_id = "my_resource_id", .server_address = "localhost", + .messaging_destination_routing_key = "key to the kingdom", + .server_port = 1234, + .messaging_destination_publish_name = "publish name", .aws_operation = "sendMessage"}); /* @@ -966,6 +1163,9 @@ static void test_segment_message_parameters_enabled(void) { */ test_message_segment( &(nr_segment_message_params_t){ + .messaging_destination_routing_key = "key to the kingdom", + .server_port = 1234, + .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", @@ -989,6 +1189,9 @@ static void test_segment_message_parameters_enabled(void) { .messaging_system = NULL, .cloud_resource_id = "my_resource_id", .server_address = NULL, + .messaging_destination_routing_key = NULL, + .server_port = 0, + .messaging_destination_publish_name = NULL, .aws_operation = "sendMessage"}); } @@ -1005,6 +1208,9 @@ void test_main(void* p NRUNUSED) { test_segment_message_messaging_system(); test_segment_message_cloud_resource_id(); test_segment_message_server_address(); + test_segment_message_server_port(); + test_segment_messaging_destination_publishing_name(); + test_segment_messaging_destination_routing_key(); test_segment_message_aws_operation(); test_segment_message_parameters_enabled(); } diff --git a/axiom/tests/test_span_event.c b/axiom/tests/test_span_event.c index ff44ab1cd..a00700a6b 100644 --- a/axiom/tests/test_span_event.c +++ b/axiom/tests/test_span_event.c @@ -483,7 +483,7 @@ static void test_span_events_extern_get_and_set(void) { nr_span_event_destroy(&span); } -static void test_span_event_message_string_get_and_set(void) { +static void test_span_event_message_get_and_set(void) { nr_span_event_t* event = nr_span_event_create(); // Test : that is does not crash when we give the setter a NULL pointer @@ -506,36 +506,93 @@ static void test_span_event_message_string_get_and_set(void) { tlib_pass_if_null("invalid range sent to nr_span_event_get_message", nr_span_event_get_message(event, 54321)); + // Test: the ulong getter should return 0 (unset) for any string values passed + // in + nr_span_event_set_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME, "chicken"); + tlib_pass_if_uint_equal( + "nr_span_event_get_message_ulong should return 0(unset) if given the " + "enum for a string value", + 0, + nr_span_event_get_message_ulong(event, NR_SPAN_MESSAGE_DESTINATION_NAME)); + + // Test: the string getter should return NULL if given the enum for a + // non-string value + nr_span_event_set_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT, 1234); + tlib_pass_if_null( + "nr_span_event_get_message should return NULL if given the enum for a " + "non-string value", + nr_span_event_get_message(event, NR_SPAN_MESSAGE_SERVER_PORT)); + // Test : setting the destination name back and forth behaves as expected nr_span_event_set_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME, "chicken"); tlib_pass_if_str_equal( - "should be the component we set 1", "chicken", + "should be the destination name we set first", "chicken", nr_span_event_get_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME)); nr_span_event_set_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME, "oracle"); tlib_pass_if_str_equal( - "should be the component we set 2", "oracle", + "should be the destination name we set second", "oracle", nr_span_event_get_message(event, NR_SPAN_MESSAGE_DESTINATION_NAME)); // Test : setting the messaging system back and forth behaves as expected nr_span_event_set_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM, "chicken"); tlib_pass_if_str_equal( - "should be the component we set 1", "chicken", + "should be the messaging system we set first", "chicken", nr_span_event_get_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM)); nr_span_event_set_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM, "oracle"); tlib_pass_if_str_equal( - "should be the component we set 2", "oracle", + "should be the messaging system we set second", "oracle", nr_span_event_get_message(event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM)); // Test : setting the server address back and forth behaves as expected nr_span_event_set_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS, "chicken"); tlib_pass_if_str_equal( - "should be the component we set 1", "chicken", + "should be the server address we set first", "chicken", nr_span_event_get_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS)); nr_span_event_set_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS, "oracle"); tlib_pass_if_str_equal( - "should be the component we set 2", "oracle", + "should be the server address we set second", "oracle", nr_span_event_get_message(event, NR_SPAN_MESSAGE_SERVER_ADDRESS)); + // Test : setting the destination pubishing name back and forth behaves as + // expected + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME, "chicken"); + tlib_pass_if_str_equal( + "should be the destination publish name we set first", "chicken", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME)); + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME, "oracle"); + tlib_pass_if_str_equal( + "should be the destination publish name we set second", "oracle", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_PUBLISH_NAME)); + + // Test : setting the destination routing key back and forth behaves as + // expected + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, "chicken"); + tlib_pass_if_str_equal( + "should be the destination routing key we set first", "chicken", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY)); + nr_span_event_set_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY, "oracle"); + tlib_pass_if_str_equal( + "should be the destination routing key we set second", "oracle", + nr_span_event_get_message( + event, NR_SPAN_MESSAGE_MESSAGING_DESTINATION_ROUTING_KEY)); + + // Test : setting the server port back and forth behaves as expected + nr_span_event_set_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT, 1234); + tlib_pass_if_ulong_equal( + "should be the server port we set first", 1234, + nr_span_event_get_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT)); + nr_span_event_set_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT, 4321); + tlib_pass_if_ulong_equal( + "should be the server port we set first", 4321, + nr_span_event_get_message_ulong(event, NR_SPAN_MESSAGE_SERVER_PORT)); + nr_span_event_destroy(&event); } @@ -724,7 +781,7 @@ void test_main(void* p NRUNUSED) { test_span_event_duration(); test_span_event_datastore_string_get_and_set(); test_span_events_extern_get_and_set(); - test_span_event_message_string_get_and_set(); + test_span_event_message_get_and_set(); test_span_event_error(); test_span_event_set_attribute_user(); test_span_event_txn_parent_attributes(); From 22f918530db8406f58d6c03f819caa93c76747bd Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 28 Jan 2025 18:37:44 -0700 Subject: [PATCH 39/71] feat(agent): Add php_amqplib basic_publish instrumentation. * Creates message segment on basic_publish call. --- agent/lib_php_amqplib.c | 172 ++++++++++++++++++++++++++++++++++++++++ agent/lib_php_amqplib.h | 6 ++ 2 files changed, 178 insertions(+) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index ec0a05c74..b1c34e0a7 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -15,9 +15,28 @@ #include "fw_support.h" #include "util_logging.h" #include "lib_php_amqplib.h" +#include "nr_segment_message.h" #define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" +/* + * With PHP 8+, we have access to all the zend_execute_data structures both + * before and after the function call so we can just maintain pointers into the + * struct. With PHP 7.x, without doing special handling, we don't have access + * to the values afterwards. Sometimes nr_php_arg_get is used as that DUPs the + * zval which then later needs to be freed with nr_php_arg_release. In this + * case, we don't need to go through the extra trouble of duplicating a ZVAL + * when we don't need to duplicate anything if there is no valid value. We + * check for a valid value, and if we want to store it, we'll strdup it. So + * instead of doing multiple zval dups all of the time, we do some strdups some + * of the time. + */ +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP8.0+ */ +#define ENSURE_PERSISTENCE(x) x +#else +#define ENSURE_PERSISTENCE(x) nr_strdup(x) +#endif + /* * Version detection will be called directly from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't @@ -60,6 +79,149 @@ void nr_php_amqplib_handle_version() { zval_dtor(&retval); } +static inline void nr_php_amqplib_get_host_and_port( + zval* amqp_connection, + nr_segment_message_params_t* message_params) { + zval* amqp_server = NULL; + zval* amqp_port = NULL; + zval* connect_constructor_params = NULL; + + if (NULL == amqp_connection || NULL == message_params) { + return; + } + + if (nr_php_is_zval_valid_object(amqp_connection)) { + connect_constructor_params + = nr_php_get_zval_object_property(amqp_connection, "construct_params"); + if (nr_php_is_zval_valid_array(connect_constructor_params)) { + amqp_server + = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), + AMQP_CONSTRUCT_PARAMS_SERVER_INDEX); + if (nr_php_is_zval_non_empty_string(amqp_server)) { + message_params->server_address + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_server)); + } + amqp_port + = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), + AMQP_CONSTRUCT_PARAMS_PORT_INDEX); + if (nr_php_is_zval_valid_scalar(amqp_port)) { + message_params->server_port = Z_LVAL_P(amqp_port); + } + } + } +} + +/* + * PhpAmqpLib\Channel\AMQPChannel::basic_publish + * Publishes a message + * + * @param AMQPMessage $msg + * @param string $exchange + * @param string $routing_key + * @param bool $mandatory + * @param bool $immediate + * @param int|null $ticket + * @throws AMQPChannelClosedException + * @throws AMQPConnectionClosedException + * @throws AMQPConnectionBlockedException + * + */ +NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { + zval* amqp_exchange = NULL; + zval* amqp_routing_key = NULL; + zval* amqp_connection = NULL; + nr_segment_t* message_segment = NULL; + + nr_segment_message_params_t message_params = { + .library = RABBITMQ_LIBRARY_NAME, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_EXCHANGE, + .message_action = NR_SPANKIND_PRODUCER, + .messaging_system = RABBITMQ_MESSAGING_SYSTEM, + }; + + (void)wraprec; + + amqp_exchange = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); + if (nr_php_is_zval_non_empty_string(amqp_exchange)) { + /* + * In PHP 7.x, the following will create a strdup in + * message_params.destination_name that needs to be freed. + */ + message_params.destination_name + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); + } else { + /* For producer, this is exchange name. Exchange name is Default in case of + * empty string. */ + message_params.destination_name = ENSURE_PERSISTENCE("Default"); + } + + amqp_routing_key = nr_php_get_user_func_arg(3, NR_EXECUTE_ORIG_ARGS); + if (nr_php_is_zval_non_empty_string(amqp_routing_key)) { + /* + * In PHP 7.x, the following will create a strdup in + * message_params.messaging_destination_routing_key that needs to be freed. + */ + message_params.messaging_destination_routing_key + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_routing_key)); + } + + amqp_connection = nr_php_get_zval_object_property( + nr_php_execute_scope(execute_data), "connection"); + /* + * In PHP 7.x, the following will create a strdup in + * message_params.server_address that needs to be freed. + */ + nr_php_amqplib_get_host_and_port(amqp_connection, &message_params); + + /* For PHP 7.x compatibility. */ + NR_PHP_WRAPPER_CALL + + /* + * Now create and end the instrumented segment as a message segment. + * + * By this point, it's been determined that this call will be instrumented so + * only create the message segment now, grab the parent segment start time, + * add our message segment attributes/metrics then close the newly created + * message segment. + */ + + if (NULL == auto_segment) { + /* + * Must be checked after PHP_WRAPPER_CALL to ensure txn didn't end during + * the call. + */ + goto end; + } + message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); + + if (NULL != message_segment) { + /* re-use start time from auto_segment started in func_begin */ + message_segment->start_time = auto_segment->start_time; + + nr_segment_message_end(&message_segment, &message_params); + } + +end: +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO + /* PHP 8+ */ + /* gnu compiler needed closure. */ + ; +#else + /* + * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, + * now we destroy them. There isn't a separate function to destroy all since + * some of the params are string literals and we don't want to strdup + * everything if we don't have to. RabbitMQ basic_publish PHP 7.x will only + * strdup server_address, destination_name, and + * messaging_destination_routing_key. + */ + nr_free(message_params.server_address); + nr_free(message_params.destination_name); + nr_free(message_params.messaging_destination_routing_key); +#endif +} +NR_PHP_WRAPPER_END + void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with @@ -72,4 +234,14 @@ void nr_php_amqplib_enable() { /* Extract the version for aws-sdk 3+ */ nr_php_amqplib_handle_version(); + +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* less than PHP8.0 */ + nr_php_wrap_user_function_before_after_clean( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), NULL, + nr_rabbitmq_basic_publish, nr_rabbitmq_basic_publish); +#else + nr_php_wrap_user_function( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), + nr_rabbitmq_basic_publish); +#endif } diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h index 6942c827b..4ca9e05e7 100644 --- a/agent/lib_php_amqplib.h +++ b/agent/lib_php_amqplib.h @@ -7,6 +7,12 @@ #ifndef LIB_PHP_AMQPLIB #define LIB_PHP_AMQPLIB +#define RABBITMQ_LIBRARY_NAME "RabbitMQ" +#define RABBITMQ_MESSAGING_SYSTEM "rabbitmq" + +#define AMQP_CONSTRUCT_PARAMS_SERVER_INDEX 0 +#define AMQP_CONSTRUCT_PARAMS_PORT_INDEX 1 + extern void nr_aws_php_amqplib_enable(); extern void nr_php_amqplib_handle_version(); From 7453159eba0b7d7cbe0b9183015655737f880951 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 28 Jan 2025 19:30:03 -0700 Subject: [PATCH 40/71] fix(axiom): Handle final metrix/txn naming when publish_name exists. --- axiom/nr_segment_message.c | 23 +++++++++++++++++------ axiom/nr_segment_message.h | 3 ++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/axiom/nr_segment_message.c b/axiom/nr_segment_message.c index 2c8207138..e2c56b966 100644 --- a/axiom/nr_segment_message.c +++ b/axiom/nr_segment_message.c @@ -101,6 +101,8 @@ static char* nr_segment_message_create_metrics( const char* action_string = NULL; const char* destination_type_string = NULL; const char* library_string = NULL; + const char* final_destination_string = NULL; + const char* destination_string = NULL; char* rollup_metric = NULL; char* scoped_metric = NULL; @@ -161,6 +163,18 @@ static char* nr_segment_message_create_metrics( destination_type_string = ""; break; } + + destination_string = nr_strempty(message_params->destination_name) + ? "" + : message_params->destination_name; + /* + * messaging_destination_publish_name is only used if it exists; In all other + * cases, we use the value from destination_string. + */ + final_destination_string = message_params->messaging_destination_publish_name + ? message_params->messaging_destination_publish_name + : destination_string; + /* * Create the scoped metric * MessageBroker/{Library}/{DestinationType}/{Action}/Named/{DestinationName} @@ -172,12 +186,9 @@ static char* nr_segment_message_create_metrics( scoped_metric = nr_formatf("MessageBroker/%s/%s/%s/Temp", library_string, destination_type_string, action_string); } else { - scoped_metric - = nr_formatf("MessageBroker/%s/%s/%s/Named/%s", library_string, - destination_type_string, action_string, - nr_strempty(message_params->destination_name) - ? "" - : message_params->destination_name); + scoped_metric = nr_formatf("MessageBroker/%s/%s/%s/Named/%s", + library_string, destination_type_string, + action_string, final_destination_string); } nr_segment_add_metric(segment, scoped_metric, true); diff --git a/axiom/nr_segment_message.h b/axiom/nr_segment_message.h index 70580697b..6917f78de 100644 --- a/axiom/nr_segment_message.h +++ b/axiom/nr_segment_message.h @@ -57,7 +57,8 @@ typedef struct { /* * Purpose : End a message segment and record metrics. * - * Params : 1. nr_segment_message_params_t + * Params : 1. nr_segment_t** segment: Segment to apply message params to and end + * 2. const nr_segment_message_params_t* params: params to apply to segment * * Returns: true on success. */ From cdbafd3883ae9668e4c6e96d0726fb8d997930bf Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 28 Jan 2025 19:31:08 -0700 Subject: [PATCH 41/71] feat(agent): Add php_libamqp basic_get instrumentation. --- agent/lib_php_amqplib.c | 125 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index b1c34e0a7..4f663c851 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -222,6 +222,123 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { } NR_PHP_WRAPPER_END +/* + * PhpAmqpLib\Channel\AMQPChannel::basic_get + * Direct access to a queue if no message was available in the queue, return + * null + * + * @param string $queue + * @param bool $no_ack + * @param int|null $ticket + * @throws \PhpAmqpLib\Exception\AMQPTimeoutException if the specified + * operation timeout was exceeded + * @return AMQPMessage|null + */ +NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { + zval* amqp_queue = NULL; + zval* amqp_exchange = NULL; + zval* amqp_routing_key = NULL; + zval* amqp_connection = NULL; + nr_segment_t* message_segment = NULL; + zval** retval_ptr = NR_GET_RETURN_VALUE_PTR; + + nr_segment_message_params_t message_params = { + .library = RABBITMQ_LIBRARY_NAME, + .destination_type = NR_MESSAGE_DESTINATION_TYPE_EXCHANGE, + .message_action = NR_SPANKIND_CONSUMER, + .messaging_system = RABBITMQ_MESSAGING_SYSTEM, + }; + + (void)wraprec; + + amqp_queue = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); + if (nr_php_is_zval_non_empty_string(amqp_queue)) { + /* For consumer, this is queue name. */ + message_params.destination_name + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_queue)); + } + + amqp_connection = nr_php_get_zval_object_property( + nr_php_execute_scope(execute_data), "connection"); + /* + * In PHP 7.x, the following will create a strdup in + * message_params.server_address that needs to be freed. + */ + nr_php_amqplib_get_host_and_port(amqp_connection, &message_params); + + /* Compatibility with PHP 7.x */ + NR_PHP_WRAPPER_CALL; + + if (NULL == auto_segment) { + /* + * Must be checked after PHP_WRAPPER_CALL to ensure txn didn't end during + * the call. + */ + goto end; + } + /* + *The retval should be an AMQPMessage. nr_php_is_zval_* ops do NULL checks + * as well. + */ + if (nr_php_is_zval_valid_object(*retval_ptr)) { + /* + * Get the exchange and routing key from the AMQPMessage + */ + amqp_exchange = nr_php_get_zval_object_property(*retval_ptr, "exchange"); + if (nr_php_is_zval_non_empty_string(amqp_exchange)) { + /* Used with consumer only; his is exchange name. Exchange name is + * Default in case of empty string. */ + message_params.messaging_destination_publish_name + = Z_STRVAL_P(amqp_exchange); + } else { + message_params.messaging_destination_publish_name = "Default"; + } + + amqp_routing_key + = nr_php_get_zval_object_property(*retval_ptr, "routingKey"); + if (nr_php_is_zval_non_empty_string(amqp_routing_key)) { + message_params.messaging_destination_routing_key + = Z_STRVAL_P(amqp_routing_key); + } + } + + /* Now create and end the instrumented segment as a message segment. */ + /* + * By this point, it's been determined that this call will be instrumented so + * only create the message segment now, grab the parent segment start time, + * add our message segment attributes/metrics then close the newly created + * message segment. + */ + message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); + + if (NULL == message_segment) { + goto end; + } + + /* re-use start time from auto_segment started in func_begin */ + message_segment->start_time = auto_segment->start_time; + + nr_segment_message_end(&message_segment, &message_params); + +end: +#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO + /* PHP 8+ */ + /* gnu compiler needed closure. */ + ; +#else + /* + * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, + * now we destroy them. There isn't a separate function to destroy all since + * some of the params are string literals and we don't want to strdup + * everything if we don't have to. RabbitMQ basic_get PHP 7.x will only strdup + * server_address and destination_name. + */ + nr_free(message_params.server_address); + nr_free(message_params.destination_name); +#endif +} +NR_PHP_WRAPPER_END + void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with @@ -239,9 +356,17 @@ void nr_php_amqplib_enable() { nr_php_wrap_user_function_before_after_clean( NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), NULL, nr_rabbitmq_basic_publish, nr_rabbitmq_basic_publish); + + nr_php_wrap_user_function_before_after_clean( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_get"), NULL, + nr_rabbitmq_basic_get, nr_rabbitmq_basic_get); #else nr_php_wrap_user_function( NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), nr_rabbitmq_basic_publish); + + nr_php_wrap_user_function( + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_get"), + nr_rabbitmq_basic_get); #endif } From 98061cd6d08436eaf7b119b6f36dc282c6a104ed Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 29 Jan 2025 09:43:06 -0700 Subject: [PATCH 42/71] fix(tests): Update tests to understand the precedence of publish_name --- axiom/nr_segment_message.c | 8 +++++--- axiom/tests/test_segment_message.c | 33 +++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/axiom/nr_segment_message.c b/axiom/nr_segment_message.c index e2c56b966..d07a3217e 100644 --- a/axiom/nr_segment_message.c +++ b/axiom/nr_segment_message.c @@ -12,6 +12,7 @@ #include "nr_segment_private.h" #include "util_strings.h" #include "util_url.h" +#include "util_logging.h" /* * Purpose : Set all the typed message attributes on the segment. @@ -171,9 +172,10 @@ static char* nr_segment_message_create_metrics( * messaging_destination_publish_name is only used if it exists; In all other * cases, we use the value from destination_string. */ - final_destination_string = message_params->messaging_destination_publish_name - ? message_params->messaging_destination_publish_name - : destination_string; + final_destination_string + = nr_strempty(message_params->messaging_destination_publish_name) + ? destination_string + : message_params->messaging_destination_publish_name; /* * Create the scoped metric diff --git a/axiom/tests/test_segment_message.c b/axiom/tests/test_segment_message.c index 9c1d3a293..bdb944f39 100644 --- a/axiom/tests/test_segment_message.c +++ b/axiom/tests/test_segment_message.c @@ -89,6 +89,17 @@ static void test_message_segment(nr_segment_message_params_t* params, tlib_pass_if_str_equal(expecteds.test_name, seg->typed_attributes->message.server_address, expecteds.server_address); + tlib_pass_if_str_equal( + expecteds.test_name, + seg->typed_attributes->message.messaging_destination_publish_name, + expecteds.messaging_destination_publish_name); + tlib_pass_if_str_equal( + expecteds.test_name, + seg->typed_attributes->message.messaging_destination_routing_key, + expecteds.messaging_destination_routing_key); + tlib_pass_if_int_equal(expecteds.test_name, + seg->typed_attributes->message.server_port, + expecteds.server_port); nr_txn_destroy(&txn); } @@ -1006,8 +1017,9 @@ static void test_segment_messaging_destination_publishing_name(void) { .destination_name = "my_destination"}, &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, (segment_message_expecteds_t){ - .test_name = "messaging_destination_publish_name is NULL, attribute " - "should be NULL", + .test_name + = "messaging_destination_publish_name is NULL, attribute " + "should be NULL, destination_name is used for metric/txn", .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", @@ -1025,8 +1037,9 @@ static void test_segment_messaging_destination_publishing_name(void) { .destination_name = "my_destination"}, &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, (segment_message_expecteds_t){ - .test_name = "messaging_destination_publish_name is empty string, " - "attribute should be NULL", + .test_name + = "messaging_destination_publish_name is empty string, " + "attribute should be NULL, destination_name is used for metric/txn", .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", @@ -1045,8 +1058,9 @@ static void test_segment_messaging_destination_publishing_name(void) { &(nr_segment_cloud_attrs_t){0}, true /* enable attributes */, (segment_message_expecteds_t){ .test_name = "Test valid messaging_destination_publish_name is " - "non-empty string, attribute should be the string", - .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + "non-empty string, attribute should be the string, " + "should be used for metric/txn", + .name = "MessageBroker/SQS/Topic/Produce/Named/publish_name", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", .num_metrics = 1, @@ -1127,8 +1141,8 @@ static void test_segment_message_parameters_enabled(void) { test_message_segment( &(nr_segment_message_params_t){ .messaging_destination_routing_key = "key to the kingdom", + .messaging_destination_publish_name = "publish_name", .server_port = 1234, - .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", @@ -1142,7 +1156,7 @@ static void test_segment_message_parameters_enabled(void) { true /* enable attributes */, (segment_message_expecteds_t){ .test_name = "Test true message_parameters_enabled", - .name = "MessageBroker/SQS/Topic/Produce/Named/my_destination", + .name = "MessageBroker/SQS/Topic/Produce/Named/publish_name", .txn_rollup_metric = "MessageBroker/all", .library_metric = "MessageBroker/SQS/all", .num_metrics = 1, @@ -1154,7 +1168,7 @@ static void test_segment_message_parameters_enabled(void) { .server_address = "localhost", .messaging_destination_routing_key = "key to the kingdom", .server_port = 1234, - .messaging_destination_publish_name = "publish name", + .messaging_destination_publish_name = "publish_name", .aws_operation = "sendMessage"}); /* @@ -1165,7 +1179,6 @@ static void test_segment_message_parameters_enabled(void) { &(nr_segment_message_params_t){ .messaging_destination_routing_key = "key to the kingdom", .server_port = 1234, - .messaging_destination_publish_name = "publish name", .server_address = "localhost", .messaging_system = "my_system", .library = "SQS", From de707379da23ba2a85df0b5a42437fd759730c88 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 30 Jan 2025 12:20:54 -0700 Subject: [PATCH 43/71] fix(agent): Handle vagaries of different Connection classes * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for * the agent to wrap the instrumented functions, there are AWS MQ_BROKER * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class * file does not explicitly load or does not load in time, and the instrumented * functions are NEVER wrapped regardless of how many times they are called in * one txn. Specifically, this centered around the very slight but impactful * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which * causes an explicit load of the AMQPChannel class/file and * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped. --- agent/lib_php_amqplib.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 4f663c851..7458083ae 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -37,6 +37,34 @@ #define ENSURE_PERSISTENCE(x) nr_strdup(x) #endif +/* + * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup + * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for + * the agent to wrap the instrumented functions, there are AWS MQ_BROKER + * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class + * file does not explicitly load or does not load in time, and the instrumented + * functions are NEVER wrapped regardless of how many times they are called in + * one txn. Specifically, this centered around the very slight but impactful + * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which + * causes an explicit load of the AMQPChannel class/file and + * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load + * of the AMQPChannelclass/file. The following method is thus the only way to + * ensure the class is loaded in time for the functions to be wrapped. + */ +static void nr_php_amqplib_ensure_class() { + zval retval; + int result = FAILURE; + + result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", + &retval, "Get nr_php_amqplib_class_exists"); + /* + * We don't need to check anything else at this point. If this fails, there's + * nothing else we can do anyway. + */ + + zval_dtor(&retval); +} + /* * Version detection will be called directly from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't @@ -351,6 +379,7 @@ void nr_php_amqplib_enable() { /* Extract the version for aws-sdk 3+ */ nr_php_amqplib_handle_version(); + nr_php_amqplib_ensure_class(); #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* less than PHP8.0 */ nr_php_wrap_user_function_before_after_clean( From 49f93b3745a3b8450d8a94149ee84571d4edc2c6 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 30 Jan 2025 17:43:01 -0700 Subject: [PATCH 44/71] fix(agent): check long not scalar --- agent/lib_php_amqplib.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 7458083ae..153b436f4 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -38,6 +38,21 @@ #endif /* + * Managing Amazon MQ for RabbitMQ engine versions +https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/rabbitmq-version-management.html +RabbitMQ version End of support on Amazon MQ +3.13 (recommended) +3.12 March 17, 2025 +3.11 February 17, 2025 +3.10 October 15, 2024 +3.9 September 16, 2024 + +4.0.5 +The latest release of RabbitMQ is 4.0.5. See change log for release notes. See +RabbitMQ support timeline to find out what release series are supported. + +Installing RabbitMQ + * * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for * the agent to wrap the instrumented functions, there are AWS MQ_BROKER @@ -45,11 +60,13 @@ * file does not explicitly load or does not load in time, and the instrumented * functions are NEVER wrapped regardless of how many times they are called in * one txn. Specifically, this centered around the very slight but impactful - * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which + * differences when using managing the AWS MQ Broker connect vs * causes an explicit load of the AMQPChannel class/file and * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped. + * + */ static void nr_php_amqplib_ensure_class() { zval retval; @@ -132,7 +149,7 @@ static inline void nr_php_amqplib_get_host_and_port( amqp_port = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (nr_php_is_zval_valid_scalar(amqp_port)) { + if (IS_LONG != Z_TYPE_P((amqp_port))) { message_params->server_port = Z_LVAL_P(amqp_port); } } From c937d7437ab86862e0a919629136c9ffc6f6cb15 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 30 Jan 2025 17:57:07 -0700 Subject: [PATCH 45/71] fix(agent): add an unperist macro --- agent/lib_php_amqplib.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 153b436f4..cf008d541 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -33,8 +33,10 @@ */ #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* PHP8.0+ */ #define ENSURE_PERSISTENCE(x) x +#define UNDO_PERSISTENCE(x) #else #define ENSURE_PERSISTENCE(x) nr_strdup(x) +#define UNDO_PERSISTENCE(x) nr_free(x); #endif /* @@ -247,11 +249,6 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { } end: -#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO - /* PHP 8+ */ - /* gnu compiler needed closure. */ - ; -#else /* * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, * now we destroy them. There isn't a separate function to destroy all since @@ -260,10 +257,9 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { * strdup server_address, destination_name, and * messaging_destination_routing_key. */ - nr_free(message_params.server_address); - nr_free(message_params.destination_name); - nr_free(message_params.messaging_destination_routing_key); -#endif + UNDO_PERSISTENCE(message_params.server_address); + UNDO_PERSISTENCE(message_params.destination_name); + UNDO_PERSISTENCE(message_params.messaging_destination_routing_key); } NR_PHP_WRAPPER_END @@ -366,11 +362,6 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { nr_segment_message_end(&message_segment, &message_params); end: -#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO - /* PHP 8+ */ - /* gnu compiler needed closure. */ - ; -#else /* * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, * now we destroy them. There isn't a separate function to destroy all since @@ -378,9 +369,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { * everything if we don't have to. RabbitMQ basic_get PHP 7.x will only strdup * server_address and destination_name. */ - nr_free(message_params.server_address); - nr_free(message_params.destination_name); -#endif + UNDO_PERSISTENCE(message_params.server_address); + UNDO_PERSISTENCE(message_params.destination_name); } NR_PHP_WRAPPER_END From 08bf3ff5dd8e62cf17d74013f2cf3e728679bfb2 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 31 Jan 2025 13:17:39 -0700 Subject: [PATCH 46/71] fix(agent): Only set to 'default' in case of valid but empty string --- agent/lib_php_amqplib.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index cf008d541..a8e968c2d 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -197,9 +197,13 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { message_params.destination_name = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); } else { - /* For producer, this is exchange name. Exchange name is Default in case of - * empty string. */ - message_params.destination_name = ENSURE_PERSISTENCE("Default"); + /* + * For producer, this is exchange name. Exchange name is Default in case of + * empty string. + */ + if (nr_php_is_zval_valid_string(amqp_exchange)) { + message_params.destination_name = ENSURE_PERSISTENCE("Default"); + } } amqp_routing_key = nr_php_get_user_func_arg(3, NR_EXECUTE_ORIG_ARGS); @@ -327,12 +331,18 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { */ amqp_exchange = nr_php_get_zval_object_property(*retval_ptr, "exchange"); if (nr_php_is_zval_non_empty_string(amqp_exchange)) { - /* Used with consumer only; his is exchange name. Exchange name is + /* Used with consumer only; this is exchange name. Exchange name is * Default in case of empty string. */ message_params.messaging_destination_publish_name = Z_STRVAL_P(amqp_exchange); } else { - message_params.messaging_destination_publish_name = "Default"; + /* + * For consumer, this is exchange name. Exchange name is Default in case + * of empty string. + */ + if (nr_php_is_zval_valid_string(amqp_exchange)) { + message_params.messaging_destination_publish_name = "Default"; + } } amqp_routing_key @@ -373,7 +383,6 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { UNDO_PERSISTENCE(message_params.destination_name); } NR_PHP_WRAPPER_END - void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with From a9b7d62890a61275337cb0378067a318502f0a1e Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 08:44:47 -0700 Subject: [PATCH 47/71] feat(agent): Insert/retrieve DT headers on rabbitmq --- agent/lib_php_amqplib.c | 469 ++++++++++++++++++++++++++++++++++------ agent/lib_php_amqplib.h | 15 ++ 2 files changed, 421 insertions(+), 63 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a8e968c2d..372ae427d 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -8,6 +8,7 @@ * https://github.com/php-amqplib/php-amqplib */ #include "php_agent.h" +#include "php_api_distributed_trace.h" #include "php_call.h" #include "php_hash.h" #include "php_wrapper.h" @@ -16,6 +17,7 @@ #include "util_logging.h" #include "lib_php_amqplib.h" #include "nr_segment_message.h" +#include "nr_header.h" #define PHP_PACKAGE_NAME "php-amqplib/php-amqplib" @@ -40,20 +42,17 @@ #endif /* - * Managing Amazon MQ for RabbitMQ engine versions -https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/rabbitmq-version-management.html -RabbitMQ version End of support on Amazon MQ -3.13 (recommended) -3.12 March 17, 2025 -3.11 February 17, 2025 -3.10 October 15, 2024 -3.9 September 16, 2024 - -4.0.5 -The latest release of RabbitMQ is 4.0.5. See change log for release notes. See -RabbitMQ support timeline to find out what release series are supported. - -Installing RabbitMQ + * See here for supported Amazon MQ for RabbitMQ engine versions + * https://docs.aws.amazon.com/amazon-mq/latest/developer-guide/rabbitmq-version-management.html + * For instance: + * As of Feb 2025, 3.13 (recommended) + * + * See here for latest RabbitMQ Server https://www.rabbitmq.com/docs/download + * For instance: + * As of Feb 2025, the latest release of RabbitMQ Server is 4.0.5. + * + * https://www.rabbitmq.com/tutorials/tutorial-one-php + * Installing RabbitMQ * * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for @@ -68,20 +67,31 @@ Installing RabbitMQ * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped. * + */ +/* + * Purpose : Retrieves host and port from an AMQP Connection and sets the + * host/port values in the message_params. + * + * Params : 1. PhpAmqpLib\Connection family of connections that inherit from + * AbstractConnection + * 2. nr_segment_message_params_t* message_params that will be + * modified with port and host info, if available + * + * Returns : None */ static void nr_php_amqplib_ensure_class() { - zval retval; + zval retval_dtor; int result = FAILURE; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - &retval, "Get nr_php_amqplib_class_exists"); + &retval_dtor, "Get nr_php_amqplib_class_exists"); /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. */ - zval_dtor(&retval); + zval_dtor(&retval_dtor); } /* @@ -94,7 +104,7 @@ static void nr_php_amqplib_ensure_class() { */ void nr_php_amqplib_handle_version() { char* version = NULL; - zval retval; + zval retval_dtor; int result = FAILURE; result = zend_eval_string( @@ -106,12 +116,12 @@ void nr_php_amqplib_handle_version() { " }" " return $nr_php_amqplib_version;" "})();", - &retval, "Get nr_php_amqplib_version"); + &retval_dtor, "Get nr_php_amqplib_version"); /* See if we got a non-empty/non-null string for version. */ if (SUCCESS == result) { - if (nr_php_is_zval_non_empty_string(&retval)) { - version = Z_STRVAL(retval); + if (nr_php_is_zval_non_empty_string(&retval_dtor)) { + version = Z_STRVAL(retval_dtor); } } @@ -123,9 +133,24 @@ void nr_php_amqplib_handle_version() { nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, version); - zval_dtor(&retval); + zval_dtor(&retval_dtor); } +/* + * Purpose : Retrieves host and port from an AMQP Connection and sets the + * host/port values in the message_params. + * + * Params : 1. PhpAmqpLib\Connection family of connections that inherit from + * AbstractConnection + * 2. nr_segment_message_params_t* message_params that will be + * modified with port and host info, if available + * + * Returns : None + * + * See here for more information about the AbstractConnection class that all + * Connection classes inherit from: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Connection/AbstractConnection.php + */ static inline void nr_php_amqplib_get_host_and_port( zval* amqp_connection, nr_segment_message_params_t* message_params) { @@ -137,28 +162,332 @@ static inline void nr_php_amqplib_get_host_and_port( return; } - if (nr_php_is_zval_valid_object(amqp_connection)) { - connect_constructor_params - = nr_php_get_zval_object_property(amqp_connection, "construct_params"); - if (nr_php_is_zval_valid_array(connect_constructor_params)) { - amqp_server - = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), - AMQP_CONSTRUCT_PARAMS_SERVER_INDEX); - if (nr_php_is_zval_non_empty_string(amqp_server)) { - message_params->server_address - = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_server)); - } - amqp_port - = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), - AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (IS_LONG != Z_TYPE_P((amqp_port))) { - message_params->server_port = Z_LVAL_P(amqp_port); + /* construct params are always saved to use for cloning purposes. */ + + if (!nr_php_is_zval_valid_object(amqp_connection)) { + return; + } + + connect_constructor_params + = nr_php_get_zval_object_property(amqp_connection, "construct_params"); + if (nr_php_is_zval_valid_array(connect_constructor_params)) { + return; + } + + amqp_server + = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), + AMQP_CONSTRUCT_PARAMS_SERVER_INDEX); + if (nr_php_is_zval_non_empty_string(amqp_server)) { + message_params->server_address + = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_server)); + } + + amqp_port = nr_php_zend_hash_index_find( + Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); + if (IS_LONG != Z_TYPE_P((amqp_port))) { + message_params->server_port = Z_LVAL_P(amqp_port); + } +} + +/* + * Purpose : Applies DT headers to an inbound AMQPMessage if + * newrelic.distributed_tracing_exclude_newrelic_header INI setting is false and + * if the headers don't already exist on the AMQPMessage. + * + * Params : PhpAmqpLib\Message\AMQPMessage + * + * Returns : None + * + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ + +static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { + zval* amqp_properties_array = NULL; + zval* dt_headers_zvf = NULL; + zval* amqp_headers_table = NULL; + zval* retval_set_property_zvf = NULL; + zval* retval_set_table_zvf = NULL; + zval application_headers_dtor; + zval key_zval_dtor; + zval amqp_table_retval_dtor; + zval* key_exists = NULL; + zval* amqp_table_data = NULL; + zend_ulong key_num = 0; + nr_php_string_hash_key_t* key_str = NULL; + zval* val = NULL; + int retval = FAILURE; + + /* + * Note, this functionality can be disabled by toggling the + * newrelic.distributed_tracing_exclude_newrelic_header INI setting. + */ + + /* + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ + if (!nr_php_is_zval_valid_object(amqp_msg)) { + return; + } + + amqp_properties_array + = nr_php_get_zval_object_property(amqp_msg, "properties"); + if (!nr_php_is_zval_valid_array(amqp_properties_array)) { + nrl_verbosedebug( + NRL_INSTRUMENT, + "AMQPMessage properties are invalid. AMQPMessage always sets " + "this to empty arrray by default so something is seriously wrong with " + "the message object. Exit."); + return; + } + + /* + * newrelic_get_request_metadata is an internal API that will only return DT + * headers if newrelic.distributed_tracing_exclude_newrelic_header is false. + */ + dt_headers_zvf = nr_php_call(NULL, "newrelic_get_request_metadata"); + if (!nr_php_is_zval_valid_array(dt_headers_zvf)) { + nr_php_zval_free(&dt_headers_zvf); + return; + } + + /* + * Get application+_headers string in zval form for use with nr_php_call + */ + ZVAL_STRING(&application_headers_dtor, "application_headers"); + + /* + * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable + * object + */ + amqp_headers_table = nr_php_zend_hash_find(Z_ARRVAL_P(amqp_properties_array), + "application_headers"); + + /* + * If the application_headers AMQPTable object doesn't exist, we'll have to + * create it with an empty array. + */ + if (!nr_php_is_zval_valid_object(amqp_headers_table)) { + retval = zend_eval_string( + "(function() {" + " try {" + " return new PhpAmqpLib\\Wire\\AMQPTable(array());" + " } catch (Throwable $e) {" + " return null;" + " }" + "})();", + &amqp_table_retval_dtor, "newrelic.amqplib.add_empty_headers"); + + if (FAILURE == retval + || !nr_php_is_zval_valid_object(&amqp_table_retval_dtor)) { + nrl_verbosedebug(NRL_INSTRUMENT, + "No application headers in AMQPTable, but couldn't " + "create one. Exit."); + goto end; + } + + /* + * Set the valid AMQPTable on the AMQPMessage. + */ + retval_set_property_zvf = nr_php_call( + amqp_msg, "set", &application_headers_dtor, &amqp_table_retval_dtor); + if (NULL == retval_set_property_zvf) { + nrl_verbosedebug(NRL_INSTRUMENT, + "AMQPMessage had no application_headers AMQPTable, but " + "set failed for the AMQPTable wthat was just created " + "for the application headers. Unable to proceed, exit."); + goto end; + } + /* Should have valid AMQPTable objec on the AMQPMessage at this point. */ + amqp_headers_table = nr_php_zend_hash_find( + Z_ARRVAL_P(amqp_properties_array), "application_headers"); + if (!nr_php_is_zval_valid_object(amqp_headers_table)) { + nrl_info( + NRL_INSTRUMENT, + "AMQPMessage had no application_headers AMQPTable, but unable to " + "retrieve even after creating and setting. Unable to proceed, exit."); + goto end; + } + } + + /* + * This contains the application_headers data. It is an array of + * key/encoded_array_val pairs. + */ + amqp_table_data = nr_php_get_zval_object_property(amqp_headers_table, "data"); + + /* + * First check if it's a reference to another zval, and if so, get point to + * the actual zval. + */ + + if (IS_REFERENCE == Z_TYPE_P(amqp_table_data)) { + amqp_table_data = Z_REFVAL_P(amqp_table_data); + } + if (!nr_php_is_zval_valid_array(amqp_table_data)) { + /* + * This is a basic part of the AMQPTable, if this doesn't exist, something + * is seriously wrong. Cannot proceed, exit. + */ + goto end; + } + + /* + * Loop through the DT Header array and set the headers in the + * application_header AMQPTable if they do not already exist. + */ + + ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(dt_headers_zvf), key_num, key_str, val) { + (void)key_num; + + if (NULL != key_str && nr_php_is_zval_valid_string(val)) { + key_exists + = nr_php_zend_hash_find(HASH_OF(amqp_table_data), ZSTR_VAL(key_str)); + if (NULL == key_exists) { + /* key_str is a zend_string. It needs to be a zval to pass to + * nr_php_call. */ + ZVAL_STR_COPY(&key_zval_dtor, key_str); + /* Key doesn't exist, so set the value in the AMQPTable. */ + retval_set_table_zvf + = nr_php_call(amqp_headers_table, "set", &key_zval_dtor, val); + + if (NULL == retval_set_table_zvf) { + nrl_verbosedebug(NRL_INSTRUMENT, + "%s didn't exist in the AMQPTable, but couldn't " + "set the key/val to the table.", + NRSAFESTR(Z_STRVAL(key_zval_dtor))); + } + nr_php_zval_free(&retval_set_table_zvf); } } } + ZEND_HASH_FOREACH_END(); + +end: + nr_php_zval_free(&dt_headers_zvf); + nr_php_zval_free(&retval_set_property_zvf); + zval_ptr_dtor(&application_headers_dtor); + zval_ptr_dtor(&amqp_table_retval_dtor); + zval_ptr_dtor(&key_zval_dtor); +} + +/* + * Purpose : Retrieve any DT headers from an inbound AMQPMessage if + * newrelic.distributed_tracing_exclude_newrelic_header INI setting is false + * and apply to txn. + * + * Params : PhpAmqpLib\Message\AMQPMessage + * + * Returns : None + * + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ +static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { + zval* amqp_headers_native_data_zvf = NULL; + zval* amqp_properties_array = NULL; + zval* amqp_headers_table = NULL; + zval* amqp_table_data = NULL; + zval* dt_payload = NULL; + zval* traceparent = NULL; + zval* tracestate = NULL; + char* dt_payload_string = NULL; + char* traceparent_string = NULL; + char* tracestate_string = NULL; + zend_ulong key_num = 0; + nr_php_string_hash_key_t* key_str = NULL; + zval* val = NULL; + int retval = FAILURE; + + /* + * Refer here for AMQPMessage: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php + * Refer here for AMQPTable: + * https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Wire/AMQPTable.php + */ + if (!nr_php_is_zval_valid_object(amqp_msg)) { + return; + } + + amqp_properties_array + = nr_php_get_zval_object_property(amqp_msg, "properties"); + if (!nr_php_is_zval_valid_array(amqp_properties_array)) { + nrl_verbosedebug( + NRL_INSTRUMENT, + "AMQPMessage properties not valid. AMQPMessage always sets " + "this to empty arrray by default. something seriously wrong with " + "the message object. Unable to proceed, Exit"); + return; + } + + /* PhpAmqpLib\Wire\AMQPTable object*/ + amqp_headers_table = nr_php_zend_hash_find(Z_ARRVAL_P(amqp_properties_array), + "application_headers"); + if (!nr_php_is_zval_valid_object(amqp_headers_table)) { + /* No headers here, exit. */ + return; + } + + /* + * We can't use amqp table "data" property here because while it has the + * correct keys, the vals are encoded arrays. We need to use getNativeData + * so it will decode the values for us since it formats the AMQPTable as an + * array of unencoded key/val pairs. */ + amqp_headers_native_data_zvf + = nr_php_call(amqp_headers_table, "getNativeData"); + + if (!nr_php_is_zval_valid_array(amqp_headers_native_data_zvf)) { + nr_php_zval_free(&amqp_headers_native_data_zvf); + return; + } + + dt_payload + = nr_php_zend_hash_find(HASH_OF(amqp_headers_native_data_zvf), NEWRELIC); + dt_payload_string + = nr_php_is_zval_valid_string(dt_payload) ? Z_STRVAL_P(dt_payload) : NULL; + + traceparent = nr_php_zend_hash_find(HASH_OF(amqp_headers_native_data_zvf), + W3C_TRACEPARENT); + traceparent_string = nr_php_is_zval_valid_string(traceparent) + ? Z_STRVAL_P(traceparent) + : NULL; + + tracestate = nr_php_zend_hash_find(HASH_OF(amqp_headers_native_data_zvf), + W3C_TRACESTATE); + tracestate_string + = nr_php_is_zval_valid_string(tracestate) ? Z_STRVAL_P(tracestate) : NULL; + + if (NULL != dt_payload || NULL != traceparent) { + nr_hashmap_t* header_map = nr_header_create_distributed_trace_map( + dt_payload_string, traceparent_string, tracestate_string); + + /* + * nr_php_api_accept_distributed_trace_payload_httpsafe will add the headers + * to the txn if there have been no other inbound/outbound headers added + * already. + */ + nr_php_api_accept_distributed_trace_payload_httpsafe(NRPRG(txn), header_map, + "Queue"); + nr_hashmap_destroy(&header_map); + } + nr_php_zval_free(&amqp_headers_native_data_zvf); + + return; } /* + * Purpose : A wrapper to instrument the php-amqplib basic_publish. This + * retrieves values to populate a message segment. If + * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also + * insert the DT headers. + * * PhpAmqpLib\Channel\AMQPChannel::basic_publish * Publishes a message * @@ -174,6 +503,7 @@ static inline void nr_php_amqplib_get_host_and_port( * */ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { + zval* amqp_msg = NULL; zval* amqp_exchange = NULL; zval* amqp_routing_key = NULL; zval* amqp_connection = NULL; @@ -188,6 +518,11 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { (void)wraprec; + amqp_msg = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); + /* + * nr_php_amqplib_insert_dt_headers will check the validity of the object. + */ + nr_php_amqplib_insert_dt_headers(amqp_msg); amqp_exchange = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); if (nr_php_is_zval_non_empty_string(amqp_exchange)) { /* @@ -198,8 +533,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); } else { /* - * For producer, this is exchange name. Exchange name is Default in case of - * empty string. + * For producer, this is exchange name. Exchange name is Default in case + * of empty string. */ if (nr_php_is_zval_valid_string(amqp_exchange)) { message_params.destination_name = ENSURE_PERSISTENCE("Default"); @@ -210,7 +545,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { if (nr_php_is_zval_non_empty_string(amqp_routing_key)) { /* * In PHP 7.x, the following will create a strdup in - * message_params.messaging_destination_routing_key that needs to be freed. + * message_params.messaging_destination_routing_key that needs to be + * freed. */ message_params.messaging_destination_routing_key = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_routing_key)); @@ -230,10 +566,10 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { /* * Now create and end the instrumented segment as a message segment. * - * By this point, it's been determined that this call will be instrumented so - * only create the message segment now, grab the parent segment start time, - * add our message segment attributes/metrics then close the newly created - * message segment. + * By this point, it's been determined that this call will be instrumented + * so only create the message segment now, grab the parent segment start + * time, add our message segment attributes/metrics then close the newly + * created message segment. */ if (NULL == auto_segment) { @@ -243,22 +579,21 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { */ goto end; } - message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); + message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); if (NULL != message_segment) { /* re-use start time from auto_segment started in func_begin */ message_segment->start_time = auto_segment->start_time; - nr_segment_message_end(&message_segment, &message_params); } end: /* - * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, - * now we destroy them. There isn't a separate function to destroy all since - * some of the params are string literals and we don't want to strdup - * everything if we don't have to. RabbitMQ basic_publish PHP 7.x will only - * strdup server_address, destination_name, and + * Because we had to strdup values to persist them beyond + * NR_PHP_WRAPPER_CALL, now we destroy them. There isn't a separate function + * to destroy all since some of the params are string literals and we don't + * want to strdup everything if we don't have to. RabbitMQ basic_publish + * PHP 7.x will only strdup server_address, destination_name, and * messaging_destination_routing_key. */ UNDO_PERSISTENCE(message_params.server_address); @@ -268,6 +603,11 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { NR_PHP_WRAPPER_END /* + * Purpose : A wrapper to instrument the php-amqplib basic_get. This + * retrieves values to populate a message segment. If + * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also + * retrieve the DT headers and, if applicable, apply to the txn. + * * PhpAmqpLib\Channel\AMQPChannel::basic_get * Direct access to a queue if no message was available in the queue, return * null @@ -337,8 +677,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { = Z_STRVAL_P(amqp_exchange); } else { /* - * For consumer, this is exchange name. Exchange name is Default in case - * of empty string. + * For consumer, this is exchange name. Exchange name is Default in + * case of empty string. */ if (nr_php_is_zval_valid_string(amqp_exchange)) { message_params.messaging_destination_publish_name = "Default"; @@ -353,12 +693,14 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { } } + nr_php_amqplib_retrieve_dt_headers(*retval_ptr); + /* Now create and end the instrumented segment as a message segment. */ /* - * By this point, it's been determined that this call will be instrumented so - * only create the message segment now, grab the parent segment start time, - * add our message segment attributes/metrics then close the newly created - * message segment. + * By this point, it's been determined that this call will be instrumented + * so only create the message segment now, grab the parent segment start + * time, add our message segment attributes/metrics then close the newly + * created message segment. */ message_segment = nr_segment_start(NRPRG(txn), NULL, NULL); @@ -373,16 +715,17 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { end: /* - * Because we had to strdup values to persist them beyond NR_PHP_WRAPPER_CALL, - * now we destroy them. There isn't a separate function to destroy all since - * some of the params are string literals and we don't want to strdup - * everything if we don't have to. RabbitMQ basic_get PHP 7.x will only strdup - * server_address and destination_name. + * Because we had to strdup values to persist them beyond + * NR_PHP_WRAPPER_CALL, now we destroy them. There isn't a separate function + * to destroy all since some of the params are string literals and we don't + * want to strdup everything if we don't have to. RabbitMQ basic_get PHP 7.x + * will only strdup server_address and destination_name. */ UNDO_PERSISTENCE(message_params.server_address); UNDO_PERSISTENCE(message_params.destination_name); } NR_PHP_WRAPPER_END + void nr_php_amqplib_enable() { /* * Set the UNKNOWN package first, so it doesn't overwrite what we find with diff --git a/agent/lib_php_amqplib.h b/agent/lib_php_amqplib.h index 4ca9e05e7..b4e565b40 100644 --- a/agent/lib_php_amqplib.h +++ b/agent/lib_php_amqplib.h @@ -13,7 +13,22 @@ #define AMQP_CONSTRUCT_PARAMS_SERVER_INDEX 0 #define AMQP_CONSTRUCT_PARAMS_PORT_INDEX 1 +/* + * Purpose : Enable the library after detection. + * + * Params : None + * + * Returns : None + */ extern void nr_aws_php_amqplib_enable(); + +/* + * Purpose : Detect the version and create package and metrics. + * + * Params : None + * + * Returns : None + */ extern void nr_php_amqplib_handle_version(); #endif /* LIB_PHP_AMQPLIB */ From 8a3086190bfe1daa7a6bcd99fe12a0fdfbaa7ae3 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 10:43:08 -0700 Subject: [PATCH 48/71] fix(agent): Update docstring --- agent/lib_php_amqplib.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 372ae427d..e8a7e62cf 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -70,13 +70,9 @@ */ /* - * Purpose : Retrieves host and port from an AMQP Connection and sets the - * host/port values in the message_params. + * Purpose : Ensures the php-amqplib instrumentation gets wrapped. * - * Params : 1. PhpAmqpLib\Connection family of connections that inherit from - * AbstractConnection - * 2. nr_segment_message_params_t* message_params that will be - * modified with port and host info, if available + * Params : None * * Returns : None */ From fc2e8803447a8572b2fcb88e34d97b429b0bbb1b Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 15:14:27 -0700 Subject: [PATCH 49/71] style(agent): rename variable --- agent/lib_php_amqplib.c | 44 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index e8a7e62cf..0e552d90b 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -77,17 +77,19 @@ * Returns : None */ static void nr_php_amqplib_ensure_class() { - zval retval_dtor; + zval retval_zpd; int result = FAILURE; +//amber +return; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - &retval_dtor, "Get nr_php_amqplib_class_exists"); + &retval_zpd, "Get nr_php_amqplib_class_exists"); /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. */ - zval_dtor(&retval_dtor); + zval_dtor(&retval_zpd); } /* @@ -100,7 +102,7 @@ static void nr_php_amqplib_ensure_class() { */ void nr_php_amqplib_handle_version() { char* version = NULL; - zval retval_dtor; + zval retval_zpd; int result = FAILURE; result = zend_eval_string( @@ -112,12 +114,12 @@ void nr_php_amqplib_handle_version() { " }" " return $nr_php_amqplib_version;" "})();", - &retval_dtor, "Get nr_php_amqplib_version"); + &retval_zpd, "Get nr_php_amqplib_version"); /* See if we got a non-empty/non-null string for version. */ if (SUCCESS == result) { - if (nr_php_is_zval_non_empty_string(&retval_dtor)) { - version = Z_STRVAL(retval_dtor); + if (nr_php_is_zval_non_empty_string(&retval_zpd)) { + version = Z_STRVAL(retval_zpd); } } @@ -129,7 +131,7 @@ void nr_php_amqplib_handle_version() { nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, version); - zval_dtor(&retval_dtor); + zval_dtor(&retval_zpd); } /* @@ -206,9 +208,9 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { zval* amqp_headers_table = NULL; zval* retval_set_property_zvf = NULL; zval* retval_set_table_zvf = NULL; - zval application_headers_dtor; - zval key_zval_dtor; - zval amqp_table_retval_dtor; + zval application_headers_zpd; + zval key_zval_zpd; + zval amqp_table_retval_zpd; zval* key_exists = NULL; zval* amqp_table_data = NULL; zend_ulong key_num = 0; @@ -255,7 +257,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { /* * Get application+_headers string in zval form for use with nr_php_call */ - ZVAL_STRING(&application_headers_dtor, "application_headers"); + ZVAL_STRING(&application_headers_zpd, "application_headers"); /* * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable @@ -277,10 +279,10 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { " return null;" " }" "})();", - &amqp_table_retval_dtor, "newrelic.amqplib.add_empty_headers"); + &amqp_table_retval_zpd, "newrelic.amqplib.add_empty_headers"); if (FAILURE == retval - || !nr_php_is_zval_valid_object(&amqp_table_retval_dtor)) { + || !nr_php_is_zval_valid_object(&amqp_table_retval_zpd)) { nrl_verbosedebug(NRL_INSTRUMENT, "No application headers in AMQPTable, but couldn't " "create one. Exit."); @@ -291,7 +293,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { * Set the valid AMQPTable on the AMQPMessage. */ retval_set_property_zvf = nr_php_call( - amqp_msg, "set", &application_headers_dtor, &amqp_table_retval_dtor); + amqp_msg, "set", &application_headers_zpd, &amqp_table_retval_zpd); if (NULL == retval_set_property_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "AMQPMessage had no application_headers AMQPTable, but " @@ -347,16 +349,16 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { if (NULL == key_exists) { /* key_str is a zend_string. It needs to be a zval to pass to * nr_php_call. */ - ZVAL_STR_COPY(&key_zval_dtor, key_str); + ZVAL_STR_COPY(&key_zval_zpd, key_str); /* Key doesn't exist, so set the value in the AMQPTable. */ retval_set_table_zvf - = nr_php_call(amqp_headers_table, "set", &key_zval_dtor, val); + = nr_php_call(amqp_headers_table, "set", &key_zval_zpd, val); if (NULL == retval_set_table_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "%s didn't exist in the AMQPTable, but couldn't " "set the key/val to the table.", - NRSAFESTR(Z_STRVAL(key_zval_dtor))); + NRSAFESTR(Z_STRVAL(key_zval_zpd))); } nr_php_zval_free(&retval_set_table_zvf); } @@ -367,9 +369,9 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { end: nr_php_zval_free(&dt_headers_zvf); nr_php_zval_free(&retval_set_property_zvf); - zval_ptr_dtor(&application_headers_dtor); - zval_ptr_dtor(&amqp_table_retval_dtor); - zval_ptr_dtor(&key_zval_dtor); + zval_ptr_dtor(&application_headers_zpd); + zval_ptr_dtor(&amqp_table_retval_zpd); + zval_ptr_dtor(&key_zval_zpd); } /* From 24d9617d6432c4fb9a069cef1e6702ba2521f840 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 17:27:07 -0700 Subject: [PATCH 50/71] style(agent): Comment clarification --- agent/lib_php_amqplib.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 0e552d90b..213b08db3 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -79,8 +79,6 @@ static void nr_php_amqplib_ensure_class() { zval retval_zpd; int result = FAILURE; -//amber -return; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", &retval_zpd, "Get nr_php_amqplib_class_exists"); @@ -93,7 +91,7 @@ return; } /* - * Version detection will be called directly from PhpAmqpLib\\Package::VERSION + * Version detection will be called pulled from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not * impossible fatal error if the file/class isn't loaded yet, we need to wrap From 2bd6f97379c06eb8d66cd4999205e3f8ad3e8690 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 4 Feb 2025 17:34:27 -0700 Subject: [PATCH 51/71] style(agent): Comment update --- agent/lib_php_amqplib.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 213b08db3..788b3524c 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -94,9 +94,11 @@ static void nr_php_amqplib_ensure_class() { * Version detection will be called pulled from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not - * impossible fatal error if the file/class isn't loaded yet, we need to wrap - * the call in a try/catch block and make it a lambda so that we avoid fatal - * errors. + * impossible fatal error if the file/class doesn't exist, we need to wrap + * the call in a try/catch block and make it a lambda so that we avoid errors. + * This won't load the file if it doesn't exist, but by the time this is called, + * the existence of the php-amqplib is a known quantity so calling the following + * lambda will result in the PhpAmqpLib\\Package class being loaded. */ void nr_php_amqplib_handle_version() { char* version = NULL; From b024ce606bc0c6f3ada867e4edc26c41fda9ce0d Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 08:46:43 -0700 Subject: [PATCH 52/71] fix(agent): Update comment --- agent/lib_php_amqplib.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 788b3524c..95518b82d 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -56,17 +56,29 @@ * * While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for - * the agent to wrap the instrumented functions, there are AWS MQ_BROKER + * the agent to wrap the instrumented functions, with AWS MQ_BROKER * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class - * file does not explicitly load or does not load in time, and the instrumented + * file does not explicitly load and the instrumented * functions are NEVER wrapped regardless of how many times they are called in - * one txn. Specifically, this centered around the very slight but impactful - * differences when using managing the AWS MQ Broker connect vs - * causes an explicit load of the AMQPChannel class/file and - * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load - * of the AMQPChannelclass/file. The following method is thus the only way to - * ensure the class is loaded in time for the functions to be wrapped. + * one txn. + * Specifically, this centered around the very slight but impactful + * differences when using managing the AWS MQ_BROKER connect vs using the + * official RabbitMq Server, and this function is needed ONLY to support AWS's + * MQ_BROKER. * + * When connecting via SSL with rabbitmq's official server is explicitly loaded. + * Hoever, when connecting via SSL with an MQ_BROKER that uses RabbitMQ(using + * the exact same php file and with only changes in the server name for the + * connection), the AMQPChannel file (and therefore class), the AMQPChannel file + * (and therefore class) is NOT explicitly loaded. + * + * Because the very key `PhpAmqpLib/Channel/AMQPChannel.php` file never gets + * explicitly loaded when interacting with the AWS MQ_BROKER, the class is not + * automatically loaded even though it is available and can be resolved if + * called from within PHP. Because of this, the instrumented functions NEVER + * get wrapped when connecting to the MQ_BROKER and therefore the + * instrumentation is never triggered. The explicit loading of the class is + * needed to work with MQ_BROKER. */ /* From e246369a74b65bf085e8a3535790358ef70f0107 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 10:21:47 -0700 Subject: [PATCH 53/71] fix(agent): Comment about support. --- agent/php_execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/php_execute.c b/agent/php_execute.c index f8d75628b..41c430d11 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -491,7 +491,7 @@ static nr_library_table_t libraries[] = { {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, - /* php-amqplib RabbitMQ >= 3.7 */ + /* php-amqplib RabbitMQ; PHP Agent supports php-amqplib >= 3.7 */ {"php-amqplib", NR_PSTR("phpamqplib/connection/abstractconnection.php"), nr_php_amqplib_enable}, From a965d83986b754b67869629616782fab9afc9108 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 13:13:38 -0700 Subject: [PATCH 54/71] Update agent/lib_php_amqplib.c Co-authored-by: Michal Nowacki --- agent/lib_php_amqplib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 95518b82d..c485ccf0f 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -675,7 +675,7 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { *The retval should be an AMQPMessage. nr_php_is_zval_* ops do NULL checks * as well. */ - if (nr_php_is_zval_valid_object(*retval_ptr)) { + if (NULL != retval_ptr && nr_php_is_zval_valid_object(*retval_ptr)) { /* * Get the exchange and routing key from the AMQPMessage */ From e2c8e05c6690cf1899464679a68ab7913c34d6a7 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 14:17:29 -0700 Subject: [PATCH 55/71] fix(agent): belongs inside the block that checked for valid retval --- agent/lib_php_amqplib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index c485ccf0f..91415b0b6 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -701,9 +701,9 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { message_params.messaging_destination_routing_key = Z_STRVAL_P(amqp_routing_key); } - } - nr_php_amqplib_retrieve_dt_headers(*retval_ptr); + nr_php_amqplib_retrieve_dt_headers(*retval_ptr); + } /* Now create and end the instrumented segment as a message segment. */ /* From b814dc488d839555624530866524f8a5dd4918ca Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 15:46:22 -0700 Subject: [PATCH 56/71] fix(agent): Expectations needed to be flipped after a previous refactor --- agent/lib_php_amqplib.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 91415b0b6..9365c1dd9 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -172,15 +172,14 @@ static inline void nr_php_amqplib_get_host_and_port( return; } - /* construct params are always saved to use for cloning purposes. */ - if (!nr_php_is_zval_valid_object(amqp_connection)) { return; } + /* construct_params are always saved to use for cloning purposes. */ connect_constructor_params = nr_php_get_zval_object_property(amqp_connection, "construct_params"); - if (nr_php_is_zval_valid_array(connect_constructor_params)) { + if (!nr_php_is_zval_valid_array(connect_constructor_params)) { return; } @@ -194,7 +193,7 @@ static inline void nr_php_amqplib_get_host_and_port( amqp_port = nr_php_zend_hash_index_find( Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (IS_LONG != Z_TYPE_P((amqp_port))) { + if (IS_LONG == Z_TYPE_P((amqp_port))) { message_params->server_port = Z_LVAL_P(amqp_port); } } From 7a4c0b962330996ca2d005e79c8a194355bc1de4 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 16:20:31 -0700 Subject: [PATCH 57/71] fix(agent): unneeded var --- agent/lib_php_amqplib.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 9365c1dd9..a4d5971a2 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -89,17 +89,14 @@ * Returns : None */ static void nr_php_amqplib_ensure_class() { - zval retval_zpd; int result = FAILURE; result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - &retval_zpd, "Get nr_php_amqplib_class_exists"); + NULL, "Get nr_php_amqplib_class_exists"); /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. */ - - zval_dtor(&retval_zpd); } /* From fc6769d8e7a9f19a7a6ae8cdf2dc84af26eb32fa Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 17:50:24 -0700 Subject: [PATCH 58/71] fix(agent): Update comments regarding DT vs w3c header behavior --- agent/lib_php_amqplib.c | 51 +++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a4d5971a2..a30e1e7b1 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -196,9 +196,14 @@ static inline void nr_php_amqplib_get_host_and_port( } /* - * Purpose : Applies DT headers to an inbound AMQPMessage if - * newrelic.distributed_tracing_exclude_newrelic_header INI setting is false and - * if the headers don't already exist on the AMQPMessage. + * Purpose : Applies DT headers to an inbound AMQPMessage. + * Note: + * The DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be added if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. * * Params : PhpAmqpLib\Message\AMQPMessage * @@ -227,8 +232,13 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { int retval = FAILURE; /* - * Note, this functionality can be disabled by toggling the - * newrelic.distributed_tracing_exclude_newrelic_header INI setting. + * Note: + * The DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be added if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. */ /* @@ -253,8 +263,13 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { } /* - * newrelic_get_request_metadata is an internal API that will only return DT - * headers if newrelic.distributed_tracing_exclude_newrelic_header is false. + * newrelic_get_request_metadata is an internal API that will only return the + * DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be returned if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. */ dt_headers_zvf = nr_php_call(NULL, "newrelic_get_request_metadata"); if (!nr_php_is_zval_valid_array(dt_headers_zvf)) { @@ -490,9 +505,15 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { /* * Purpose : A wrapper to instrument the php-amqplib basic_publish. This - * retrieves values to populate a message segment. If - * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also - * insert the DT headers. + * retrieves values to populate a message segment and insert the DT headers, if + * applicable. + * + * Note: The DT header 'newrelic' will only be added if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be added if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. * * PhpAmqpLib\Channel\AMQPChannel::basic_publish * Publishes a message @@ -610,8 +631,14 @@ NR_PHP_WRAPPER_END /* * Purpose : A wrapper to instrument the php-amqplib basic_get. This - * retrieves values to populate a message segment. If - * newrelic.distributed_tracing_exclude_newrelic_header is false, it will also + * retrieves values to populate a message segment. + * Note: + * The DT header 'newrelic' will only be considered if both + * newrelic.distributed_tracing_enabled is enabled and + * newrelic.distributed_tracing_exclude_newrelic_header is set to false in the + * INI settings. The W3C headers 'traceparent' and 'tracestate' will will only + * be considered if newrelic.distributed_tracing_enabled is enabled in the + * newrelic.ini settings. If settings are correct, it will * retrieve the DT headers and, if applicable, apply to the txn. * * PhpAmqpLib\Channel\AMQPChannel::basic_get From 011a6f7436811e93592669b3d1cf980ad385e05a Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 20:42:06 -0700 Subject: [PATCH 59/71] fix(agent): just bail early from headers if DT isn't on --- agent/lib_php_amqplib.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a30e1e7b1..80d3e85da 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -251,6 +251,10 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { return; } + if (!NRPRG(txn)->options.distributed_tracing_enabled) { + return; + } + amqp_properties_array = nr_php_get_zval_object_property(amqp_msg, "properties"); if (!nr_php_is_zval_valid_array(amqp_properties_array)) { @@ -286,9 +290,9 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable * object */ + amqp_headers_table = nr_php_zend_hash_find(Z_ARRVAL_P(amqp_properties_array), "application_headers"); - /* * If the application_headers AMQPTable object doesn't exist, we'll have to * create it with an empty array. @@ -311,7 +315,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { "create one. Exit."); goto end; } - /* * Set the valid AMQPTable on the AMQPMessage. */ @@ -384,6 +387,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { NRSAFESTR(Z_STRVAL(key_zval_zpd))); } nr_php_zval_free(&retval_set_table_zvf); + zval_ptr_dtor(&key_zval_zpd); } } } @@ -394,7 +398,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { nr_php_zval_free(&retval_set_property_zvf); zval_ptr_dtor(&application_headers_zpd); zval_ptr_dtor(&amqp_table_retval_zpd); - zval_ptr_dtor(&key_zval_zpd); } /* @@ -437,6 +440,10 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { return; } + if (!NRPRG(txn)->options.distributed_tracing_enabled) { + return; + } + amqp_properties_array = nr_php_get_zval_object_property(amqp_msg, "properties"); if (!nr_php_is_zval_valid_array(amqp_properties_array)) { @@ -769,7 +776,7 @@ void nr_php_amqplib_enable() { PHP_PACKAGE_VERSION_UNKNOWN); } - /* Extract the version for aws-sdk 3+ */ + /* Extract the version */ nr_php_amqplib_handle_version(); nr_php_amqplib_ensure_class(); From d4fc082be8da14cee3c0a5e0e2623c79adac7638 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 5 Feb 2025 21:28:51 -0700 Subject: [PATCH 60/71] fix(agent): use zval is integer function --- agent/lib_php_amqplib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 80d3e85da..92407fde1 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -190,7 +190,7 @@ static inline void nr_php_amqplib_get_host_and_port( amqp_port = nr_php_zend_hash_index_find( Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); - if (IS_LONG == Z_TYPE_P((amqp_port))) { + if (nr_php_is_zval_valid_integer(amqp_port)) { message_params->server_port = Z_LVAL_P(amqp_port); } } From 60fd62872a1def18978b57eae96531c677e25976 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 09:11:16 -0700 Subject: [PATCH 61/71] fix(agent): Move dtors closer to where the values are initialized --- agent/lib_php_amqplib.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 92407fde1..bfc49fdf9 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -281,11 +281,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { return; } - /* - * Get application+_headers string in zval form for use with nr_php_call - */ - ZVAL_STRING(&application_headers_zpd, "application_headers"); - /* * The application_headers are stored in an encoded PhpAmqpLib\Wire\AMQPTable * object @@ -308,18 +303,32 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { "})();", &amqp_table_retval_zpd, "newrelic.amqplib.add_empty_headers"); - if (FAILURE == retval - || !nr_php_is_zval_valid_object(&amqp_table_retval_zpd)) { + if (FAILURE == retval) { + nrl_verbosedebug(NRL_INSTRUMENT, + "No application headers in AMQPTable, but couldn't " + "create one. Exit."); + goto end; + } + if (!nr_php_is_zval_valid_object(&amqp_table_retval_zpd)) { nrl_verbosedebug(NRL_INSTRUMENT, "No application headers in AMQPTable, but couldn't " "create one. Exit."); + zval_ptr_dtor(&amqp_table_retval_zpd); goto end; } + /* + * Get application+_headers string in zval form for use with nr_php_call + */ + ZVAL_STRING(&application_headers_zpd, "application_headers"); /* * Set the valid AMQPTable on the AMQPMessage. */ retval_set_property_zvf = nr_php_call( amqp_msg, "set", &application_headers_zpd, &amqp_table_retval_zpd); + + zval_ptr_dtor(&application_headers_zpd); + zval_ptr_dtor(&amqp_table_retval_zpd); + if (NULL == retval_set_property_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "AMQPMessage had no application_headers AMQPTable, but " @@ -396,8 +405,6 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { end: nr_php_zval_free(&dt_headers_zvf); nr_php_zval_free(&retval_set_property_zvf); - zval_ptr_dtor(&application_headers_zpd); - zval_ptr_dtor(&amqp_table_retval_zpd); } /* From e623e9994397efd7e33b10c222ae7818fb672d5c Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 09:31:56 -0700 Subject: [PATCH 62/71] fix(agent): add dtor closer to init --- agent/lib_php_amqplib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index bfc49fdf9..bcd01a608 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -382,21 +382,21 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { key_exists = nr_php_zend_hash_find(HASH_OF(amqp_table_data), ZSTR_VAL(key_str)); if (NULL == key_exists) { + /* Key doesn't exist, so set the value in the AMQPTable. */ + /* key_str is a zend_string. It needs to be a zval to pass to * nr_php_call. */ ZVAL_STR_COPY(&key_zval_zpd, key_str); - /* Key doesn't exist, so set the value in the AMQPTable. */ retval_set_table_zvf = nr_php_call(amqp_headers_table, "set", &key_zval_zpd, val); - + zval_ptr_dtor(&key_zval_zpd); if (NULL == retval_set_table_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "%s didn't exist in the AMQPTable, but couldn't " "set the key/val to the table.", - NRSAFESTR(Z_STRVAL(key_zval_zpd))); + NRSAFESTR(ZSTR_VAL(key_str))); } nr_php_zval_free(&retval_set_table_zvf); - zval_ptr_dtor(&key_zval_zpd); } } } From 839b098229bd2c53549a6d30fa70f39b5df48a79 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 20:02:25 -0700 Subject: [PATCH 63/71] fix(agent): Calling order --- agent/lib_php_amqplib.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index bcd01a608..9b15ab3c3 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -389,13 +389,13 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { ZVAL_STR_COPY(&key_zval_zpd, key_str); retval_set_table_zvf = nr_php_call(amqp_headers_table, "set", &key_zval_zpd, val); - zval_ptr_dtor(&key_zval_zpd); if (NULL == retval_set_table_zvf) { nrl_verbosedebug(NRL_INSTRUMENT, "%s didn't exist in the AMQPTable, but couldn't " "set the key/val to the table.", NRSAFESTR(ZSTR_VAL(key_str))); } + zval_ptr_dtor(&key_zval_zpd); nr_php_zval_free(&retval_set_table_zvf); } } @@ -510,6 +510,7 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { */ nr_php_api_accept_distributed_trace_payload_httpsafe(NRPRG(txn), header_map, "Queue"); + nr_hashmap_destroy(&header_map); } nr_php_zval_free(&amqp_headers_native_data_zvf); @@ -543,8 +544,20 @@ static inline void nr_php_amqplib_retrieve_dt_headers(zval* amqp_msg) { * @throws AMQPConnectionBlockedException * */ -NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { + +NR_PHP_WRAPPER(nr_rabbitmq_basic_publish_before) { zval* amqp_msg = NULL; + (void)wraprec; + + amqp_msg = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); + /* + * nr_php_amqplib_insert_dt_headers will check the validity of the object. + */ + nr_php_amqplib_insert_dt_headers(amqp_msg); +} +NR_PHP_WRAPPER_END + +NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { zval* amqp_exchange = NULL; zval* amqp_routing_key = NULL; zval* amqp_connection = NULL; @@ -559,11 +572,15 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_publish) { (void)wraprec; +#if ZEND_MODULE_API_NO < ZEND_8_0_X_API_NO /* PHP8.0+ */ + zval* amqp_msg = NULL; amqp_msg = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS); /* * nr_php_amqplib_insert_dt_headers will check the validity of the object. */ nr_php_amqplib_insert_dt_headers(amqp_msg); +#endif + amqp_exchange = nr_php_get_user_func_arg(2, NR_EXECUTE_ORIG_ARGS); if (nr_php_is_zval_non_empty_string(amqp_exchange)) { /* @@ -789,8 +806,9 @@ void nr_php_amqplib_enable() { #if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO /* less than PHP8.0 */ nr_php_wrap_user_function_before_after_clean( - NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), NULL, - nr_rabbitmq_basic_publish, nr_rabbitmq_basic_publish); + NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_publish"), + nr_rabbitmq_basic_publish_before, nr_rabbitmq_basic_publish, + nr_rabbitmq_basic_publish); nr_php_wrap_user_function_before_after_clean( NR_PSTR("PhpAmqpLib\\Channel\\AMQPChannel::basic_get"), NULL, From ba45e6c624f8575435f798c92b85808924e32bfb Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 6 Feb 2025 20:48:44 -0700 Subject: [PATCH 64/71] Update agent/lib_php_amqplib.c Co-authored-by: Michal Nowacki --- agent/lib_php_amqplib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 9b15ab3c3..7eb69a6dd 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -196,7 +196,7 @@ static inline void nr_php_amqplib_get_host_and_port( } /* - * Purpose : Applies DT headers to an inbound AMQPMessage. + * Purpose : Applies DT headers to an outbound AMQPMessage. * Note: * The DT header 'newrelic' will only be added if both * newrelic.distributed_tracing_enabled is enabled and From 87f641ecb0b768d06a513090ecb76a3bf3b57cc3 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 7 Feb 2025 10:08:35 -0700 Subject: [PATCH 65/71] fix(agent): return null in version check and handle that outcome --- agent/lib_php_amqplib.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 9b15ab3c3..e93994e4a 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -116,7 +116,7 @@ void nr_php_amqplib_handle_version() { result = zend_eval_string( "(function() {" - " $nr_php_amqplib_version = '';" + " $nr_php_amqplib_version = null;" " try {" " $nr_php_amqplib_version = PhpAmqpLib\\Package::VERSION;" " } catch (Throwable $e) {" @@ -126,19 +126,23 @@ void nr_php_amqplib_handle_version() { &retval_zpd, "Get nr_php_amqplib_version"); /* See if we got a non-empty/non-null string for version. */ - if (SUCCESS == result) { - if (nr_php_is_zval_non_empty_string(&retval_zpd)) { - version = Z_STRVAL(retval_zpd); - } + if (FAILURE == result) { + return; } - if (NRINI(vulnerability_management_package_detection_enabled)) { - /* Add php package to transaction */ - nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); + if (nr_php_is_zval_valid_string(&retval_zpd)) { + version = Z_STRVAL(retval_zpd); } - nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, - version); + if (NULL != version) { + if (NRINI(vulnerability_management_package_detection_enabled)) { + /* Add php package to transaction */ + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); + } + + nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, + version); + } zval_dtor(&retval_zpd); } From 3fa60ecd1d7abbb2687aba0b7ebc37c645389ecc Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 7 Feb 2025 16:21:17 -0700 Subject: [PATCH 66/71] fix(agent): revert rebase error --- agent/lib_php_amqplib.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 8b013edf9..d138d3bd2 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -200,11 +200,7 @@ static inline void nr_php_amqplib_get_host_and_port( } /* -<<<<<<< HEAD - * Purpose : Applies DT headers to an inbound AMQPMessage. -======= * Purpose : Applies DT headers to an outbound AMQPMessage. ->>>>>>> ba45e6c624f8575435f798c92b85808924e32bfb * Note: * The DT header 'newrelic' will only be added if both * newrelic.distributed_tracing_enabled is enabled and From 268c28bc3839e1d1c6cd6c849455d1aee3834b8f Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Fri, 7 Feb 2025 21:43:23 -0700 Subject: [PATCH 67/71] fix(agent): It's actually fine if version is null reverting a portion of previous PR feedback. It's okay if version remains null, the package function will then set it to library known, version unknown. --- agent/lib_php_amqplib.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index d138d3bd2..98cf17bb7 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -126,23 +126,19 @@ void nr_php_amqplib_handle_version() { &retval_zpd, "Get nr_php_amqplib_version"); /* See if we got a non-empty/non-null string for version. */ - if (FAILURE == result) { - return; + if (SUCCESS == result) { + if (nr_php_is_zval_valid_string(&retval_zpd)) { + version = Z_STRVAL(retval_zpd); + } } - if (nr_php_is_zval_valid_string(&retval_zpd)) { - version = Z_STRVAL(retval_zpd); + if (NRINI(vulnerability_management_package_detection_enabled)) { + /* Add php package to transaction */ + nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); } - if (NULL != version) { - if (NRINI(vulnerability_management_package_detection_enabled)) { - /* Add php package to transaction */ - nr_txn_add_php_package(NRPRG(txn), PHP_PACKAGE_NAME, version); - } - - nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, - version); - } + nr_txn_suggest_package_supportability_metric(NRPRG(txn), PHP_PACKAGE_NAME, + version); zval_dtor(&retval_zpd); } @@ -789,6 +785,8 @@ NR_PHP_WRAPPER(nr_rabbitmq_basic_get) { * want to strdup everything if we don't have to. RabbitMQ basic_get PHP 7.x * will only strdup server_address and destination_name. */ + // amber make these peristent for all since retval of null clears the values + // from the cxn UNDO_PERSISTENCE(message_params.server_address); UNDO_PERSISTENCE(message_params.destination_name); } From 73b47b2b29c14f239c855b25148549dbee0b7511 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Mon, 10 Feb 2025 08:53:14 -0700 Subject: [PATCH 68/71] fix(agent): check if Channel class exists before trying to manually load it --- agent/lib_php_amqplib.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 98cf17bb7..a611877df 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -91,8 +91,12 @@ static void nr_php_amqplib_ensure_class() { int result = FAILURE; - result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - NULL, "Get nr_php_amqplib_class_exists"); + class_entry = nr_php_find_class("phpamqplib\\channel\\amqpchannel"); + if (NULL == class_entry) { + result + = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", + NULL, "Get nr_php_amqplib_class_exists"); + } /* * We don't need to check anything else at this point. If this fails, there's * nothing else we can do anyway. From 54f720d58c4b248a52d54156945f752b07a002a3 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Mon, 10 Feb 2025 09:00:51 -0700 Subject: [PATCH 69/71] style(agent): rename eval_string last param --- agent/lib_php_amqplib.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index a611877df..28e85b2dc 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -90,12 +90,13 @@ */ static void nr_php_amqplib_ensure_class() { int result = FAILURE; + zend_class_entry* class_entry = NULL; class_entry = nr_php_find_class("phpamqplib\\channel\\amqpchannel"); if (NULL == class_entry) { - result - = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", - NULL, "Get nr_php_amqplib_class_exists"); + result = zend_eval_string( + "class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", NULL, + "nr_php_amqplib_class_exists_channel_amqpchannel"); } /* * We don't need to check anything else at this point. If this fails, there's @@ -127,7 +128,7 @@ void nr_php_amqplib_handle_version() { " }" " return $nr_php_amqplib_version;" "})();", - &retval_zpd, "Get nr_php_amqplib_version"); + &retval_zpd, "nr_php_amqplib_get_phpamqplib_package_version"); /* See if we got a non-empty/non-null string for version. */ if (SUCCESS == result) { @@ -305,7 +306,7 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { " return null;" " }" "})();", - &amqp_table_retval_zpd, "newrelic.amqplib.add_empty_headers"); + &amqp_table_retval_zpd, "nr_php_amqplib_create_empty_amqptable"); if (FAILURE == retval) { nrl_verbosedebug(NRL_INSTRUMENT, From 55383f0a2136c5ac33d3ca68a006654961d3732a Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Mon, 10 Feb 2025 09:05:02 -0700 Subject: [PATCH 70/71] fix(agent): use zend_eval_stringl --- agent/lib_php_amqplib.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index 28e85b2dc..ec246ecc7 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -94,8 +94,8 @@ static void nr_php_amqplib_ensure_class() { class_entry = nr_php_find_class("phpamqplib\\channel\\amqpchannel"); if (NULL == class_entry) { - result = zend_eval_string( - "class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", NULL, + result = zend_eval_stringl( + NR_PSTR("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');"), NULL, "nr_php_amqplib_class_exists_channel_amqpchannel"); } /* @@ -119,15 +119,16 @@ void nr_php_amqplib_handle_version() { zval retval_zpd; int result = FAILURE; - result = zend_eval_string( - "(function() {" - " $nr_php_amqplib_version = null;" - " try {" - " $nr_php_amqplib_version = PhpAmqpLib\\Package::VERSION;" - " } catch (Throwable $e) {" - " }" - " return $nr_php_amqplib_version;" - "})();", + result = zend_eval_stringl( + NR_PSTR( + "(function() {" + " $nr_php_amqplib_version = null;" + " try {" + " $nr_php_amqplib_version = PhpAmqpLib\\Package::VERSION;" + " } catch (Throwable $e) {" + " }" + " return $nr_php_amqplib_version;" + "})();"), &retval_zpd, "nr_php_amqplib_get_phpamqplib_package_version"); /* See if we got a non-empty/non-null string for version. */ @@ -298,14 +299,14 @@ static inline void nr_php_amqplib_insert_dt_headers(zval* amqp_msg) { * create it with an empty array. */ if (!nr_php_is_zval_valid_object(amqp_headers_table)) { - retval = zend_eval_string( - "(function() {" - " try {" - " return new PhpAmqpLib\\Wire\\AMQPTable(array());" - " } catch (Throwable $e) {" - " return null;" - " }" - "})();", + retval = zend_eval_stringl( + NR_PSTR("(function() {" + " try {" + " return new PhpAmqpLib\\Wire\\AMQPTable(array());" + " } catch (Throwable $e) {" + " return null;" + " }" + "})();"), &amqp_table_retval_zpd, "nr_php_amqplib_create_empty_amqptable"); if (FAILURE == retval) { From 4461be70372109b92a6ccd64e17f3369ed6d5533 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 12 Feb 2025 14:07:14 -0700 Subject: [PATCH 71/71] Update agent/lib_php_amqplib.c Co-authored-by: Michal Nowacki --- agent/lib_php_amqplib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/lib_php_amqplib.c b/agent/lib_php_amqplib.c index ec246ecc7..94e15fe96 100644 --- a/agent/lib_php_amqplib.c +++ b/agent/lib_php_amqplib.c @@ -105,7 +105,7 @@ static void nr_php_amqplib_ensure_class() { } /* - * Version detection will be called pulled from PhpAmqpLib\\Package::VERSION + * Version information will be pulled from PhpAmqpLib\\Package::VERSION * nr_php_amqplib_handle_version will automatically load the class if it isn't * loaded yet and then evaluate the string. To avoid the VERY unlikely but not * impossible fatal error if the file/class doesn't exist, we need to wrap