Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

refactor(agent): cleanup guzzle instrumentation #498

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions agent/fw_hooks.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ extern void nr_doctrine2_enable(TSRMLS_D);
extern void nr_guzzle3_enable(TSRMLS_D);
extern void nr_guzzle4_enable(TSRMLS_D);
extern void nr_guzzle6_enable(TSRMLS_D);
extern void nr_guzzle7_enable(TSRMLS_D);
extern void nr_laminas_http_enable(TSRMLS_D);
extern void nr_mongodb_enable(TSRMLS_D);
extern void nr_phpunit_enable(TSRMLS_D);
Expand Down
8 changes: 1 addition & 7 deletions agent/lib_guzzle4.c
Original file line number Diff line number Diff line change
Expand Up @@ -445,12 +445,6 @@ NR_PHP_WRAPPER_START(nr_guzzle4_client_construct) {
(void)wraprec;
NR_UNUSED_SPECIALFN;

/* This is how we distinguish Guzzle 4/5 from other versions. */
if (0 == nr_guzzle_does_zval_implement_has_emitter(this_var TSRMLS_CC)) {
NR_PHP_WRAPPER_CALL;
goto end;
}

lavarou marked this conversation as resolved.
Show resolved Hide resolved
NR_PHP_WRAPPER_CALL;

/*
Expand Down Expand Up @@ -517,7 +511,7 @@ void nr_guzzle4_enable(TSRMLS_D) {
* for all requests created on that client.
*/
nr_php_wrap_user_function(NR_PSTR("GuzzleHttp\\Client::__construct"),
nr_guzzle_client_construct TSRMLS_CC);
nr_guzzle4_client_construct TSRMLS_CC);
}

void nr_guzzle4_minit(TSRMLS_D) {
Expand Down
251 changes: 155 additions & 96 deletions agent/lib_guzzle6.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
* It is a required component in Drupal 8, and strongly recommended by other
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to be clang formatted

* frameworks, including Symfony 2 and 3.
*
* Our approach for Guzzle 6 is to register middleware on every client that
* Our approach for Guzzle 6-7 is to register middleware on every client that
* adds our headers to the request object, handles responses, and creates
* metrics and trace nodes using the internal RequestHandler class declared
* below.
*
* There is one issue with this approach, which is that the middleware is
* called when the request is created, rather than when the request is sent. As
* Guzzle 6 removed the event system that allowed us to know exactly when the
* Guzzle 6-7 removed the event system that allowed us to know exactly when the
* request was sent, we are unable to get the time of the request being sent
* without instrumenting much more deeply into Guzzle's handlers. We consider
* this to be an obscure enough edge case that we are not doing this work at
Expand Down Expand Up @@ -65,12 +65,11 @@
*/
#if ZEND_MODULE_API_NO >= ZEND_5_5_X_API_NO

/* {{{ newrelic\Guzzle6\RequestHandler class definition and methods */

/*
* True global for the RequestHandler class entry.
*/
zend_class_entry* nr_guzzle6_requesthandler_ce;
int php_version_compare(char*, char*);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessary. Instead, #include "ext/standard/php_versioning.h" should be added to the #include statements

Suggested change
int php_version_compare(char*, char*);


/*
* Arginfo for the RequestHandler methods.
Expand Down Expand Up @@ -107,7 +106,7 @@ static void nr_guzzle6_requesthandler_handle_response(zval* handler,
zval* response
TSRMLS_DC) {
nr_segment_t* segment = NULL;
nr_segment_external_params_t external_params = {.library = "Guzzle 6"};
nr_segment_external_params_t external_params = {.library = "Guzzle 6-7"};
zval* request;
zval* method;
zval* status;
Expand Down Expand Up @@ -140,7 +139,7 @@ static void nr_guzzle6_requesthandler_handle_response(zval* handler,

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

Expand Down Expand Up @@ -206,14 +205,14 @@ static PHP_NAMED_FUNCTION(nr_guzzle6_requesthandler_construct) {
zend_update_property(Z_OBJCE_P(this_obj), ZVAL_OR_ZEND_OBJECT(this_obj),
NR_PSTR("request"), request TSRMLS_CC);

nr_guzzle_obj_add(this_obj, "Guzzle 6" TSRMLS_CC);
nr_guzzle_obj_add(this_obj, "Guzzle 6-7" TSRMLS_CC);
}

/*
* Proto : void RequestHandler::onFulfilled(Psr\Http\Message\ResponseInterface
* $response)
*
* Purpose : Called when a Guzzle 6 request promise is fulfilled.
* Purpose : Called when a Guzzle 6-7 request promise is fulfilled.
*
* Params : 1. The response object.
*/
Expand Down Expand Up @@ -257,7 +256,7 @@ static PHP_NAMED_FUNCTION(nr_guzzle6_requesthandler_onfulfilled) {
* Proto : void
* RequestHandler::onRejected(GuzzleHttp\Exception\TransferException $e)
*
* Purpose : Called when a Guzzle 6 request promise failed.
* Purpose : Called when a Guzzle 6-7 request promise failed.
*
* Params : 1. The exception object.
*/
Expand Down Expand Up @@ -340,76 +339,27 @@ const zend_function_entry nr_guzzle6_requesthandler_functions[]
nr_guzzle6_requesthandler_onrejected_arginfo,
ZEND_ACC_PUBLIC) PHP_FE_END};

/* }}} */

NR_PHP_WRAPPER_START(nr_guzzle6_client_construct) {
zval* config;
zend_class_entry* guzzle_client_ce;
zval* handler_stack;
zval* middleware = NULL;
zval* retval;
zval* this_var = nr_php_scope_get(NR_EXECUTE_ORIG_ARGS TSRMLS_CC);

(void)wraprec;
NR_UNUSED_SPECIALFN;

/* This is how we distinguish Guzzle 4/5. */
if (nr_guzzle_does_zval_implement_has_emitter(this_var TSRMLS_CC)) {
NR_PHP_WRAPPER_CALL;
goto end;
}

NR_PHP_WRAPPER_CALL;

/*
* Get our middleware callable (which is just a string), and make sure it's
* actually callable before we invoke push(). (See also PHP-1184.)
*/
middleware = nr_php_zval_alloc();
nr_php_zval_str(middleware, "newrelic\\Guzzle6\\middleware");
if (!nr_php_is_zval_valid_callable(middleware TSRMLS_CC)) {
nrl_verbosedebug(NRL_FRAMEWORK,
"%s: middleware string is not considered callable",
__func__);

nrm_force_add(NRTXN(unscoped_metrics),
"Supportability/library/Guzzle 6/MiddlewareNotCallable", 0);

goto end;
}

guzzle_client_ce = nr_php_find_class("guzzlehttp\\client" TSRMLS_CC);
if (NULL == guzzle_client_ce) {
nrl_verbosedebug(NRL_FRAMEWORK,
"%s: unable to get class entry for GuzzleHttp\\Client",
__func__);
goto end;
}

config = nr_php_get_zval_object_property_with_class(
this_var, guzzle_client_ce, "config" TSRMLS_CC);
if (!nr_php_is_zval_valid_array(config)) {
goto end;
static void nr_guzzle_minit(const int guzzle_version){
zend_class_entry ce;
char guzzle_path[] = "newrelic\\Guzzle6\\RequestHandler";
if (guzzle_version == 7){
nr_strcpy(guzzle_path, "newrelic\\Guzzle7\\RequestHandler");
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem the right approach. Luckily declaration (and initialization) on line 348 creates char array of exactly the size enough to fit this strcpy. Maybe consider turning guzzle_path into a function that will return const char * based on guzzle_version? Something like this:

static const char *
make_request_handler_class_name(const int guzzle_version) {
    if (6 == guzzle_version) {
        return "newrelic\\Guzzle6\\RequestHandler";
    }
    if (7 == guzzle_version) {
        return "newrelic\\Guzzle7\\RequestHandler";
    }
    return NULL;
}
...
static void nr_guzzle_minit(const int guzzle_version){
...
    const char *nr_guzzle_request_handler_class_name = NULL;
...
    if (0 == NRINI(guzzle_enabled)) {
        return;
    }
    nr_guzzle_request_handler_class_name = make_request_handler_class_name(guzzle_version);
    if (NULL == nr_guzzle_request_handler_class_name) {
        return;
    }
    INIT_CLASS_ENTRY(ce, nr_guzzle_request_handler_class_name, nr_guzzle6_requesthandler_functions);
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also agree that using nr_strcpy is not the most efficient way. I have fixed this by removing the guzzle version from the namespace! 40ab8cf

}

handler_stack = nr_php_zend_hash_find(Z_ARRVAL_P(config), "handler");
if (!nr_php_object_instanceof_class(handler_stack,
"GuzzleHttp\\HandlerStack" TSRMLS_CC)) {
goto end;
if (0 == NRINI(guzzle_enabled)) {
return;
}

retval = nr_php_call(handler_stack, "push", middleware);

nr_php_zval_free(&retval);
INIT_CLASS_ENTRY(ce, guzzle_path,
nr_guzzle6_requesthandler_functions);
nr_guzzle6_requesthandler_ce
= nr_php_zend_register_internal_class_ex(&ce, NULL TSRMLS_CC);

end:
nr_php_zval_free(&middleware);
nr_php_scope_release(&this_var);
zend_declare_property_null(nr_guzzle6_requesthandler_ce, NR_PSTR("request"),
ZEND_ACC_PRIVATE TSRMLS_CC);
}
NR_PHP_WRAPPER_END

void nr_guzzle6_enable(TSRMLS_D) {
int retval;
static void nr_guzzle_enable(const int guzzle_version){
int _retval;

if (0 == NRINI(guzzle_enabled)) {
return;
Expand All @@ -429,20 +379,20 @@ void nr_guzzle6_enable(TSRMLS_D) {
* as a standalone file, so we can use a normal namespace declaration to
* avoid possible clashes.
*/
retval = zend_eval_string(
"namespace newrelic\\Guzzle6;"
char *eval = nr_formatf(
Copy link
Member

Choose a reason for hiding this comment

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

I don't see eval being freed...

Copy link
Contributor Author

@hahuja2 hahuja2 Sep 21, 2022

Choose a reason for hiding this comment

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

Fixed! 40ab8cf

"namespace newrelic\\Guzzle%d;"

"use Psr\\Http\\Message\\RequestInterface;"

"if (!function_exists('newrelic\\Guzzle6\\middleware')) {"
/*
* Start by adding the outbound CAT/DT/Synthetics headers to the request.
*/
"if (!function_exists('newrelic\\Guzzle%d\\middleware')) {"
" function middleware(callable $handler) {"
" return function (RequestInterface $request, array $options) use "
"($handler) {"

/*
* Start by adding the outbound CAT/DT/Synthetics headers to the request.
*/
" foreach (newrelic_get_request_metadata('Guzzle 6') as $k => $v) {"
" foreach (newrelic_get_request_metadata('Guzzle %d') as $k => $v) {"
" $request = $request->withHeader($k, $v);"
" }"

Expand All @@ -458,33 +408,142 @@ void nr_guzzle6_enable(TSRMLS_D) {
" return $promise;"
" };"
" }"
"}",
NULL, "newrelic/Guzzle6" TSRMLS_CC);
"}", guzzle_version, guzzle_version, guzzle_version);

char *guzzle_ver = nr_formatf("newrelic/Guzzle%d", guzzle_version);
Copy link
Member

Choose a reason for hiding this comment

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

I don't see guzzle_ver being freed...

Copy link
Contributor Author

@hahuja2 hahuja2 Sep 21, 2022

Choose a reason for hiding this comment

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

Fixed 40ab8cf

_retval = zend_eval_string(eval, NULL, guzzle_ver TSRMLS_CC);

if (SUCCESS == retval) {
if (SUCCESS == _retval && guzzle_version == 6) {
nr_php_wrap_user_function(NR_PSTR("GuzzleHttp\\Client::__construct"),
nr_guzzle_client_construct TSRMLS_CC);
} else {
nr_guzzle6_client_construct TSRMLS_CC);
}else if (SUCCESS == _retval && guzzle_version == 7){
nr_php_wrap_user_function(NR_PSTR("GuzzleHttp\\Client::__construct"),
nr_guzzle7_client_construct TSRMLS_CC);
}else {
Copy link
Member

Choose a reason for hiding this comment

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

I think this code can be simplified further because nr_guzzle6_client_construct and nr_guzzle7_client_construct are actually nr_guzzle_client_construct_helper. Also I don't think the guzzle_version needs to be used in any of the New Relic instrumenting namespaces or class names. This would simplify the code and reduce memory allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is a great point! I have simplified the code 40ab8cf

nrl_warning(NRL_FRAMEWORK,
"%s: error evaluating PHP code; not installing handler",
__func__);
}
}

void nr_guzzle6_minit(TSRMLS_D) {
zend_class_entry ce;
NR_PHP_WRAPPER_START(nr_guzzle_client_construct_helper){
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a brief doc comment describing what this function does? I.e. it adds newrelic\Guzzle\middleware to the GuzzleHttp\Client handler stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I have added a brief description that describes what this function is doing f3ac653

zval* this_var = nr_php_scope_get(NR_EXECUTE_ORIG_ARGS TSRMLS_CC);
zval* retval;
int guzzle_version = 6;
char *version = nr_guzzle_version(this_var);
Copy link
Member

Choose a reason for hiding this comment

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

Does version need to be freed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, version also needs to be freed because the nr_guzzle_version function allocates memory for a new string and does not free it, which means the caller is responsible for freeing the result. 29f4551

if (php_version_compare(version, "7") >= 0){
guzzle_version = 7;
}

if (0 == NRINI(guzzle_enabled)) {
return;
(void)wraprec;
NR_UNUSED_SPECIALFN;
NR_PHP_WRAPPER_CALL;
/*
* Get our middleware callable (which is just a string), and make sure it's
* actually callable before we invoke push(). (See also PHP-1184.)
*/
char *str_middleware = nr_formatf("newrelic\\Guzzle%d\\middleware",
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this being freed.

Copy link
Contributor Author

@hahuja2 hahuja2 Sep 21, 2022

Choose a reason for hiding this comment

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

Thank you for pointing that out! Since guzzle version is not needed in the namespace, I will not have to use nr_formatf() anymore. 40ab8cf

guzzle_version);
zval* middleware = nr_php_zval_alloc();
nr_php_zval_str(middleware, str_middleware);
if (!nr_php_is_zval_valid_callable(middleware TSRMLS_CC)) {
nrl_verbosedebug(NRL_FRAMEWORK,
"%s: middleware string is not considered callable",
__func__);

char* error_message = nr_formatf(
Copy link
Member

Choose a reason for hiding this comment

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

I don't see error_message being freed.

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! 29f4551

"Supportability/library/Guzzle %d/MiddlewareNotCallable", guzzle_version);
nrm_force_add(NRTXN(unscoped_metrics), error_message, 0);
goto end;
}

INIT_CLASS_ENTRY(ce, "newrelic\\Guzzle6\\RequestHandler",
nr_guzzle6_requesthandler_functions);
nr_guzzle6_requesthandler_ce
= nr_php_zend_register_internal_class_ex(&ce, NULL TSRMLS_CC);
zend_class_entry* guzzle_client_ce = nr_php_find_class("guzzlehttp\\client"
TSRMLS_CC);
if (NULL == guzzle_client_ce) {
nrl_verbosedebug(NRL_FRAMEWORK,
"%s: unable to get class entry for GuzzleHttp\\Client",
__func__);
goto end;
}

zend_declare_property_null(nr_guzzle6_requesthandler_ce, NR_PSTR("request"),
ZEND_ACC_PRIVATE TSRMLS_CC);
zval* config = nr_php_get_zval_object_property_with_class(
this_var, guzzle_client_ce, "config" TSRMLS_CC);
if (!nr_php_is_zval_valid_array(config)) {
goto end;
}

zval* handler_stack = nr_php_zend_hash_find(Z_ARRVAL_P(config), "handler");
if (!nr_php_object_instanceof_class(handler_stack,
"GuzzleHttp\\HandlerStack" TSRMLS_CC)) {
goto end;
}

retval = nr_php_call(handler_stack, "push", middleware);

nr_php_zval_free(&retval);

end:
nr_php_zval_free(&middleware);
nr_php_scope_release(&this_var);
}
NR_PHP_WRAPPER_END

/*
* Guzzle 7 requires PHP 7.2.0 or later, which is why we will not build Guzzle 7
* support on older versions and will instead provide simple stubs for the two
* exported functions to avoid linking errors.
*/
#if ZEND_MODULE_API_NO >= ZEND_7_2_X_API_NO

NR_PHP_WRAPPER_START(nr_guzzle7_client_construct){
NR_PHP_WRAPPER_DELEGATE(nr_guzzle_client_construct_helper);
(void)wraprec;
NR_UNUSED_SPECIALFN;
NR_PHP_WRAPPER_CALL;
}
NR_PHP_WRAPPER_END

void nr_guzzle7_enable(TSRMLS_D) {
nr_guzzle_enable(7);
}

void nr_guzzle7_minit(TSRMLS_D) {
nr_guzzle_minit(7);
}

#else /* PHP < 7.2 */

NR_PHP_WRAPPER_START(nr_guzzle7_client_construct) {
(void)wraprec;
NR_UNUSED_SPECIALFN;
NR_UNUSED_TSRMLS;
}
NR_PHP_WRAPPER_END

void nr_guzzle7_enable(TSRMLS_D) {
NR_UNUSED_TSRMLS
}

void nr_guzzle7_minit(TSRMLS_D) {
NR_UNUSED_TSRMLS;
}

#endif /* 7.2.x */

NR_PHP_WRAPPER_START(nr_guzzle6_client_construct) {
NR_PHP_WRAPPER_DELEGATE(nr_guzzle_client_construct_helper);
(void)wraprec;
NR_UNUSED_SPECIALFN;
NR_PHP_WRAPPER_CALL;
}
NR_PHP_WRAPPER_END

void nr_guzzle6_enable(TSRMLS_D) {
nr_guzzle_enable(6);
}

void nr_guzzle6_minit(TSRMLS_D) {
nr_guzzle_minit(6);
}

#else /* PHP < 5.5 */
Expand All @@ -504,4 +563,4 @@ void nr_guzzle6_minit(TSRMLS_D) {
NR_UNUSED_TSRMLS;
}

#endif /* 5.5.x */
#endif /* 5.5.x */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif /* 5.5.x */
#endif /* 5.5.x */

Loading