Skip to content

fix(agent): fix Drupal error and exception handling #995

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

Merged
merged 5 commits into from
Dec 20, 2024
Merged

fix(agent): fix Drupal error and exception handling #995

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
955697b
fix(agent): fix Drupal error and exception handling
bduranleau-nr Dec 11, 2024
dfd1392
Update agent/fw_drupal8.c
bduranleau-nr Dec 16, 2024
47a930d
Update agent/fw_drupal8.c
bduranleau-nr Dec 16, 2024
7ab64fb
fix: remove HttpExceptionSubscriber wrapper
bduranleau-nr Dec 17, 2024
82ec0d7
fix: defensively free exception if getThrowable fails
bduranleau-nr Dec 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
81 changes: 81 additions & 0 deletions agent/fw_drupal8.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "php_user_instrument.h"
#include "php_execute.h"
#include "php_wrapper.h"
#include "php_error.h"
#include "fw_drupal_common.h"
#include "fw_hooks.h"
#include "fw_support.h"
Expand All @@ -20,6 +21,60 @@

#define PHP_PACKAGE_NAME "drupal/core"

NR_PHP_WRAPPER(nr_drupal_exception) {
int priority = nr_php_error_get_priority(E_ERROR);
zval* event = NULL;
zval* exception = NULL;

/* Warning avoidance */
(void)wraprec;

NR_PHP_WRAPPER_REQUIRE_FRAMEWORK(NR_FW_DRUPAL8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use a variation of the NR_PHP_WRAPPER_REQUIRE_FRAMEWORK macro (which doesnt yet exist) to make sure a compatible version of Drupal is present? This works with 9+ correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes exceptions for 8 and exceptions and errors for 9+. For Drupal 8 errors, this code has no impact (positive or negative) as the problem with error reporting originates from this block unique to pre-oapi instrumentation, and D8 is only compatible with PHP up to 7.4.


if (NR_SUCCESS != nr_txn_record_error_worthy(NRPRG(txn), priority)) {
NR_PHP_WRAPPER_CALL;
goto end;
}

/* Get the event that was given. */
event = nr_php_arg_get(1, NR_EXECUTE_ORIG_ARGS TSRMLS_CC);

/* Call the original function. */
NR_PHP_WRAPPER_CALL;

if (0 == nr_php_is_zval_valid_object(event)) {
nrl_verbosedebug(NRL_TXN,
"Drupal: ExceptionSubscriber::onException() does not "
"have an `event` parameter");
goto end;
}

/*
* Get the exception from the event.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This code seems to rely on nr_php_call() gracefully handling if these methods did not exist. Have you verified this is how it works?
I am also curious if these calls could return a valid zval that is not an exception object and perhaps it would be best to verify it is what we expect?
Or does nr_php_error_record_exception() handle this inspection and verification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code seems to rely on nr_php_call() gracefully handling if these methods did not exist. Have you verified this is how it works?

I believe this is outside the scope of this PR. We use nr_php_call in this same manner throughout the agent, but for what it's worth, I do believe it handles the case that it does not exist gracefully.

I am also curious if these calls could return a valid zval that is not an exception object and perhaps it would be best to verify it is what we expect?

The very first call in nr_php_error_record_exception is to nr_php_error_zval_is_exception.

exception = nr_php_call(event, "getThrowable");
if (!nr_php_is_zval_valid_object(exception)) {
exception = nr_php_call(event, "getException");
}

if (!nr_php_is_zval_valid_object(exception)) {
nrl_verbosedebug(NRL_TXN, "Drupal: getException() returned a non-object");
goto end;
}

if (NR_SUCCESS
!= nr_php_error_record_exception(NRPRG(txn), exception, priority, true,
NULL,
&NRPRG(exception_filters) TSRMLS_CC)) {
nrl_verbosedebug(NRL_TXN, "Drupal: unable to record exception");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be higher priority than verbosedebug?

}

end:
nr_php_arg_release(&event);
nr_php_zval_free(&exception);
}
NR_PHP_WRAPPER_END

/*
* Purpose : Convenience function to handle adding a callback to a method,
* given a class entry and a method name. This will check the
Expand Down Expand Up @@ -730,6 +785,32 @@ void nr_drupal8_enable(TSRMLS_D) {
"er::getControllerFromDefinition"),
nr_drupal8_name_the_wt TSRMLS_CC);

/*
* ExceptionSubscribers handle Drupal errors and exceptions before
* the agent has the opportunity to capture them. Instrument several
* of these ExceptionSubscriber function `onException` methods in order
* to capture Exceptions and Errors in Drupal 9.x+
*/
// clang-format off
/*
* Utility base class for exception subscribers.
*/
nr_php_wrap_user_function(NR_PSTR("Drupal\\Core\\EventSubscriber\\HttpExceptionSubscriberBase::onException"),
nr_drupal_exception);

/*
* Log exceptions without further handling.
*/
nr_php_wrap_user_function(NR_PSTR("Drupal\\Core\\EventSubscriber\\ExceptionLoggingSubscriber::onException"),
nr_drupal_exception);

/*
* Last-chance handler for exceptions: the final exception subscriber.
*/
nr_php_wrap_user_function(NR_PSTR("Drupal\\Core\\EventSubscriber\\FinalExceptionSubscriber::onException"),
nr_drupal_exception);
// clang-format on

/*
* The drupal_modules config setting controls instrumentation of modules,
* hooks, and views.
Expand Down
Loading