Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(agent): remove oapi 'clean' callback #866

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

refactor(agent): remove oapi 'clean' callback #866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2197a45
refactor(agent): remove oapi 'clean' callback
ZNeumann Mar 20, 2024
5e4bbff
fix: accidentally included file
ZNeumann Mar 20, 2024
7998fed
fix drupal dangling segment
ZNeumann Apr 17, 2024
d03e5b2
fix tests
ZNeumann Apr 17, 2024
8b2ec4b
fix tests
ZNeumann Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions agent/fw_cakephp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,8 @@ void nr_cakephp_enable_2(TSRMLS_D) {
nr_cakephp_name_the_wt_2 TSRMLS_CC);
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
NR_PSTR("CakeException::__construct"), nr_cakephp_problem_2, NULL, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("CakeException::__construct"), nr_cakephp_problem_2, NULL);
#else
nr_php_wrap_user_function(NR_PSTR("CakeException::__construct"),
nr_cakephp_problem_2 TSRMLS_CC);
Expand Down
66 changes: 29 additions & 37 deletions agent/fw_drupal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ NR_PHP_WRAPPER(nr_drupal_http_request_after) {

NR_PHP_WRAPPER_REQUIRE_FRAMEWORK(NR_FW_DRUPAL);

nr_segment_external_params_t external_params
= {.library = "Drupal",
.uri = NULL};

/*
* Grab the URL for the external metric, which is the first parameter in all
* versions of Drupal.
Expand All @@ -290,57 +294,54 @@ NR_PHP_WRAPPER(nr_drupal_http_request_after) {
goto end;
}

if (NULL == NR_GET_RETURN_VALUE_PTR) {
goto end;
}

external_params.uri = nr_strndup(Z_STRVAL_P(arg1), Z_STRLEN_P(arg1));

/*
* We only want to create a metric here if this isn't a recursive call to
* drupal_http_request() caused by the original call returning a redirect.
* We can check how many drupal_http_request() calls are on the stack by
* checking a counter.
*/
if (1 == NRPRG(drupal_http_request_depth)) {
nr_segment_external_params_t external_params
= {.library = "Drupal",
.uri = nr_strndup(Z_STRVAL_P(arg1), Z_STRLEN_P(arg1))};

external_params.procedure
= nr_drupal_http_request_get_method(NR_EXECUTE_ORIG_ARGS);

external_params.encoded_response_header
= nr_drupal_http_request_get_response_header(&func_return_value);
= nr_drupal_http_request_get_response_header(NR_GET_RETURN_VALUE_PTR);

external_params.status
= nr_drupal_http_request_get_response_code(&func_return_value);
= nr_drupal_http_request_get_response_code(NR_GET_RETURN_VALUE_PTR);
if (NRPRG(txn) && NRTXN(special_flags.debug_cat)) {
nrl_verbosedebug(
NRL_CAT, "CAT: outbound response: transport='Drupal 6-7' %s=" NRP_FMT,
X_NEWRELIC_APP_DATA,
NRP_CAT(external_params.encoded_response_header));
}
}

nr_segment_external_end(&NRPRG(drupal_http_request_segment),
&external_params);
end:
if (1 == NRPRG(drupal_http_request_depth)) {
if (external_params.uri == NULL) {
nr_segment_discard(&NRPRG(drupal_http_request_segment));
} else {
nr_segment_external_end(&NRPRG(drupal_http_request_segment),
&external_params);
}
NRPRG(drupal_http_request_segment) = NULL;

nr_free(external_params.encoded_response_header);
nr_free(external_params.procedure);
nr_free(external_params.uri);
}

end:
nr_php_arg_release(&arg1);
NRPRG(drupal_http_request_depth) -= 1;
}
NR_PHP_WRAPPER_END

NR_PHP_WRAPPER(nr_drupal_http_request_clean) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this removal, it is failing some drupal integration tests, because the tests were changed for PHP 8.0+ to not create an external segment after an exception. A simple if can be added to the "after" instrumentation if this behavior is still desired. Or do we want to go back to what the agent was seemingly previously doing?

(Also, this old "clean" function was leaving a dangling segment on the transaction. Not a memory leak because the transaction handles its own segment memory, but scary nonetheless)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although the tests do raise an interesting point. There are some tests that test for "bad" parameters; these cause an exception. But because our "after" code isn't perfectly checking arguments, it crashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9081863

NR_UNUSED_SPECIALFN;
NR_UNUSED_FUNC_RETURN_VALUE;
(void)wraprec;

NR_PHP_WRAPPER_REQUIRE_FRAMEWORK(NR_FW_DRUPAL);

NRPRG(drupal_http_request_depth) -= 1;
}
NR_PHP_WRAPPER_END
#else

/*
Expand Down Expand Up @@ -778,14 +779,6 @@ NR_PHP_WRAPPER(nr_drupal_wrap_module_invoke_all_after) {
}
NR_PHP_WRAPPER_END

NR_PHP_WRAPPER(nr_drupal_wrap_module_invoke_all_clean) {
NR_UNUSED_SPECIALFN;
NR_UNUSED_FUNC_RETURN_VALUE;
(void)wraprec;
nr_drupal_invoke_all_hook_stacks_pop();
}
NR_PHP_WRAPPER_END

#else
NR_PHP_WRAPPER(nr_drupal_wrap_module_invoke_all) {
zval* hook = NULL;
Expand Down Expand Up @@ -834,14 +827,14 @@ void nr_drupal_enable(TSRMLS_D) {
nr_drupal_cron_run TSRMLS_CC);
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
NR_PSTR("QFormBase::Run"), nr_drupal_qdrupal_name_the_wt, NULL, NULL);
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("QFormBase::Run"), nr_drupal_qdrupal_name_the_wt, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("drupal_page_cache_header"), nr_drupal_name_wt_as_cached_page,
NULL, NULL);
nr_php_wrap_user_function_before_after_clean(
NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("drupal_http_request"), nr_drupal_http_request_before,
nr_drupal_http_request_after, nr_drupal_http_request_clean);
nr_drupal_http_request_after);
#else
nr_php_wrap_user_function(NR_PSTR("QFormBase::Run"),
nr_drupal_qdrupal_name_the_wt TSRMLS_CC);
Expand All @@ -860,10 +853,9 @@ void nr_drupal_enable(TSRMLS_D) {
nr_drupal_wrap_module_invoke TSRMLS_CC);
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("module_invoke_all"), nr_drupal_wrap_module_invoke_all_before,
nr_drupal_wrap_module_invoke_all_after,
nr_drupal_wrap_module_invoke_all_clean);
nr_drupal_wrap_module_invoke_all_after);
#else
nr_php_wrap_user_function(NR_PSTR("module_invoke_all"),
nr_drupal_wrap_module_invoke_all TSRMLS_CC);
Expand Down
24 changes: 8 additions & 16 deletions agent/fw_drupal8.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,12 @@ static void nr_drupal8_add_method_callback(const zend_class_entry* ce,

#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
static void nr_drupal8_add_method_callback_before_after_clean(
static void nr_drupal8_add_method_callback_before_after(
const zend_class_entry* ce,
const char* method,
size_t method_len,
nrspecialfn_t before_callback,
nrspecialfn_t after_callback,
nrspecialfn_t clean_callback) {
nrspecialfn_t after_callback) {
zend_function* function = NULL;

if (NULL == ce) {
Expand All @@ -106,9 +105,9 @@ static void nr_drupal8_add_method_callback_before_after_clean(
"%.*s::%.*s", NRSAFELEN(nr_php_class_entry_name_length(ce)),
nr_php_class_entry_name(ce), NRSAFELEN(method_len), method);

nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
class_method, nr_strlen(class_method),
before_callback, after_callback, clean_callback);
before_callback, after_callback);

nr_free(class_method);
}
Expand Down Expand Up @@ -542,12 +541,6 @@ NR_PHP_WRAPPER(nr_drupal94_invoke_all_with_after) {
nr_drupal_invoke_all_hook_stacks_pop();
}
NR_PHP_WRAPPER_END

NR_PHP_WRAPPER(nr_drupal94_invoke_all_with_clean) {
(void)wraprec;
nr_drupal_invoke_all_hook_stacks_pop();
}
NR_PHP_WRAPPER_END
#endif // OAPI

/*
Expand Down Expand Up @@ -584,11 +577,10 @@ NR_PHP_WRAPPER(nr_drupal8_module_handler) {
/* Drupal 9.4 introduced a replacement method for getImplentations */
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_drupal8_add_method_callback_before_after_clean(
nr_drupal8_add_method_callback_before_after(
ce, NR_PSTR("invokeallwith"),
nr_drupal94_invoke_all_with,
nr_drupal94_invoke_all_with_after,
nr_drupal94_invoke_all_with_clean);
nr_drupal94_invoke_all_with_after);
#else
nr_drupal8_add_method_callback(ce, NR_PSTR("invokeallwith"),
nr_drupal94_invoke_all_with TSRMLS_CC);
Expand Down Expand Up @@ -709,10 +701,10 @@ void nr_drupal8_enable(TSRMLS_D) {
*/
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Symfony\\Component\\HttpKernel\\EventListe"
"ner\\RouterListener::onKernelRequest"),
nr_drupal8_name_the_wt_via_symfony, NULL, NULL);
nr_drupal8_name_the_wt_via_symfony, NULL);
#else
nr_php_wrap_user_function(NR_PSTR("Symfony\\Component\\HttpKernel\\EventListe"
"ner\\RouterListener::onKernelRequest"),
Expand Down
12 changes: 6 additions & 6 deletions agent/fw_laravel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,8 @@ static void nr_laravel5_wrap_middleware(zval* app TSRMLS_DC) {
Z_STRVAL_P(classname));
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
name, nr_strlen(name), nr_laravel5_middleware_handle, NULL, NULL);
nr_php_wrap_user_function_before_after(
name, nr_strlen(name), nr_laravel5_middleware_handle, NULL);
#else
nr_php_wrap_user_function(name, nr_strlen(name),
nr_laravel5_middleware_handle TSRMLS_CC);
Expand Down Expand Up @@ -833,8 +833,8 @@ static void nr_laravel_add_callback_method(const zend_class_entry* ce,

#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
class_method, nr_strlen(class_method), callback, NULL, NULL);
nr_php_wrap_user_function_before_after(
class_method, nr_strlen(class_method), callback, NULL);
#else
nr_php_wrap_user_function(class_method, nr_strlen(class_method),
callback TSRMLS_CC);
Expand Down Expand Up @@ -1231,9 +1231,9 @@ void nr_laravel_enable(TSRMLS_D) {
*/
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Illuminate\\Console\\Application::doRun"),
nr_laravel_console_application_dorun, NULL, NULL);
nr_laravel_console_application_dorun, NULL);
#else
nr_php_wrap_user_function(NR_PSTR("Illuminate\\Console\\Application::doRun"),
nr_laravel_console_application_dorun TSRMLS_CC);
Expand Down
16 changes: 8 additions & 8 deletions agent/fw_laravel_queue.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,18 +864,18 @@ void nr_laravel_queue_enable(TSRMLS_D) {
* took.
*/

nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Illuminate\\Queue\\Worker::raiseBeforeJobEvent"), NULL,
nr_laravel_queue_worker_raiseBeforeJobEvent_after, NULL);
nr_php_wrap_user_function_before_after_clean(
nr_laravel_queue_worker_raiseBeforeJobEvent_after);
nr_php_wrap_user_function_before_after(
NR_PSTR("Illuminate\\Queue\\Worker::raiseAfterJobEvent"),
nr_laravel_queue_worker_raiseAfterJobEvent_before, NULL, NULL);
nr_php_wrap_user_function_before_after_clean(
nr_laravel_queue_worker_raiseAfterJobEvent_before, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("Illuminate\\Queue\\SyncQueue::raiseBeforeJobEvent"),
nr_laravel_queue_syncqueue_raiseBeforeJobEvent_before, NULL, NULL);
nr_php_wrap_user_function_before_after_clean(
nr_laravel_queue_syncqueue_raiseBeforeJobEvent_before, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("Illuminate\\Queue\\SyncQueue::raiseAfterJobEvent"),
nr_laravel_queue_worker_raiseAfterJobEvent_before, NULL, NULL);
nr_laravel_queue_worker_raiseAfterJobEvent_before, NULL);

#else

Expand Down
4 changes: 2 additions & 2 deletions agent/fw_lumen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,9 @@ void nr_lumen_enable(TSRMLS_D) {
nr_lumen_handle_found_route TSRMLS_CC);
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Laravel\\Lumen\\Application::sendExceptionToHandler"),
nr_lumen_exception, NULL, NULL);
nr_lumen_exception, NULL);
#else
nr_php_wrap_user_function(
NR_PSTR("Laravel\\Lumen\\Application::sendExceptionToHandler"),
Expand Down
16 changes: 8 additions & 8 deletions agent/fw_magento2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,9 +440,9 @@ void nr_magento2_enable(TSRMLS_D) {
*/
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Magento\\Framework\\App\\Action\\Action::dispatch"),
nr_magento2_action_dispatch, NULL, NULL);
nr_magento2_action_dispatch, NULL);
#else
nr_php_wrap_user_function(
NR_PSTR("Magento\\Framework\\App\\Action\\Action::dispatch"),
Expand Down Expand Up @@ -472,10 +472,10 @@ void nr_magento2_enable(TSRMLS_D) {
*/
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR(
"Magento\\Webapi\\Controller\\Rest\\InputParamsResolver::resolve"),
nr_magento2_inputparamsresolver_resolve, NULL, NULL);
nr_magento2_inputparamsresolver_resolve, NULL);
#else
nr_php_wrap_user_function(
NR_PSTR(
Expand All @@ -497,14 +497,14 @@ void nr_magento2_enable(TSRMLS_D) {
nr_magento2_soap_iswsdllistrequest TSRMLS_CC);
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Magento\\Webapi\\Controller\\Soap\\Request\\Handler::_"
"prepareRequestData"),
nr_magento2_soap_handler_preparerequestdata, NULL, NULL);
nr_php_wrap_user_function_before_after_clean(
nr_magento2_soap_handler_preparerequestdata, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("Magento\\Webapi\\Controller\\Soap\\Request\\Handler::"
"prepareOperationInput"),
nr_magento2_soap_handler_prepareoperationinput, NULL, NULL);
nr_magento2_soap_handler_prepareoperationinput, NULL);

#else
nr_php_wrap_user_function(
Expand Down
12 changes: 6 additions & 6 deletions agent/fw_slim.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,17 +181,17 @@ void nr_slim_enable(TSRMLS_D) {
&& !defined OVERWRITE_ZEND_EXECUTE_DATA

/* Slim 3 */
nr_php_wrap_user_function_before_after_clean(
NR_PSTR("Slim\\Route::run"), nr_slim3_4_route_run, NULL, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("Slim\\Route::run"), nr_slim3_4_route_run, NULL);

/* Slim 4 */
nr_php_wrap_user_function_before_after_clean(
NR_PSTR("Slim\\Routing\\Route::run"), nr_slim3_4_route_run, NULL, NULL);
nr_php_wrap_user_function_before_after(
NR_PSTR("Slim\\Routing\\Route::run"), nr_slim3_4_route_run, NULL);

/* Slim 4 */
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Slim\\Routing\\Dispatcher::dispatch"), nr_slim4_route_dispatch,
NULL, NULL);
NULL);
#else
/* Slim 4*/
nr_php_wrap_user_function(NR_PSTR("Slim\\Routing\\Route::run"),
Expand Down
4 changes: 2 additions & 2 deletions agent/fw_symfony4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,9 @@ void nr_symfony4_enable(TSRMLS_D) {
*/
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_php_wrap_user_function_before_after_clean(
nr_php_wrap_user_function_before_after(
NR_PSTR("Symfony\\Component\\Console\\Command\\Command::run"),
nr_symfony4_console_application_run, NULL, NULL);
nr_symfony4_console_application_run, NULL);
#else
nr_php_wrap_user_function(
NR_PSTR("Symfony\\Component\\Console\\Command\\Command::run"),
Expand Down
Loading