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 7 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
6 changes: 0 additions & 6 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
191 changes: 158 additions & 33 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,15 @@
*/
#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;
zval* config;
zend_class_entry* guzzle_client_ce;
zval* handler_stack;
zval* middleware = NULL;
zval* retval;
Copy link
Member

Choose a reason for hiding this comment

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

Why do these variables need to be global?

Copy link
Contributor Author

@hahuja2 hahuja2 Aug 31, 2022

Choose a reason for hiding this comment

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

These variables were initially global because nr_guzzle6_client_construct and nr_guzzle7_client_construct both used these variables. However, I have added a helper function called nr_guzzle_client_construct_helper, which reduces code duplication and does not require these variables to be global anymore. Fixed! 6222a51


/*
* Arginfo for the RequestHandler methods.
Expand Down Expand Up @@ -107,7 +110,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 +143,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 +209,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 +260,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,40 +343,33 @@ const zend_function_entry nr_guzzle6_requesthandler_functions[]
nr_guzzle6_requesthandler_onrejected_arginfo,
ZEND_ACC_PUBLIC) PHP_FE_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_guzzle6_client_construct) {
zval* config;
zend_class_entry* guzzle_client_ce;
zval* handler_stack;
zval* middleware = NULL;
zval* retval;
NR_PHP_WRAPPER_START(nr_guzzle7_client_construct){
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");
nr_php_zval_str(middleware, "newrelic\\Guzzle7\\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);
"Supportability/library/Guzzle 7/MiddlewareNotCallable", 0);

goto end;
}
Expand Down Expand Up @@ -408,8 +404,8 @@ NR_PHP_WRAPPER_START(nr_guzzle6_client_construct) {
}
NR_PHP_WRAPPER_END

void nr_guzzle6_enable(TSRMLS_D) {
int retval;
void nr_guzzle7_enable(TSRMLS_D) {
Copy link
Member

Choose a reason for hiding this comment

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

This function differs from nr_guzzle6_enable only by a guzzle version number. I would strongly suggest extracting the common code into a helper function that would accept this difference as an input. Something in the form of:

static void nr_guzzleX_enable(const int guzzle_version) {
// paste the contents of `nr_guzzle7_enable` and use guzzle_version when constructing strings that utilize the version

}

void nr_guzzle6_enable() {
  nr_guzzleX_enable(6);
}

void nr_guzzle7_enable() {
  nr_guzzleX_enable(7);
}

Alternatively only leave original nr_guzzle6_enable but update all strings that reference guzzle version number to include two versions. Look at how it is done in lib_guzzle4.c for Guzzle 4 and Guzzle 5.

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! I have added a helper function called nr_guzzle_enable. I also have added a helper function called nr_guzzle_client_construct_helper, which helps reduce code duplication! 6222a51

int _retval;

if (0 == NRINI(guzzle_enabled)) {
return;
Expand All @@ -429,20 +425,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;"
_retval = zend_eval_string(
"namespace newrelic\\Guzzle7;"

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

"if (!function_exists('newrelic\\Guzzle6\\middleware')) {"
"if (!function_exists('newrelic\\Guzzle7\\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 7') as $k => $v) {"
" $request = $request->withHeader($k, $v);"
" }"

Expand All @@ -455,13 +451,142 @@ void nr_guzzle6_enable(TSRMLS_D) {
" $promise = $handler($request, $options);"
" $promise->then([$rh, 'onFulfilled'], [$rh, 'onRejected']);"

" return $promise;"
" };"
" }"
"}",
NULL, "newrelic/Guzzle7" TSRMLS_CC);

if (SUCCESS == _retval) {
nr_php_wrap_user_function(NR_PSTR("GuzzleHttp\\Client::__construct"),
nr_guzzle_client_construct TSRMLS_CC);
} else {
nrl_warning(NRL_FRAMEWORK,
"%s: error evaluating PHP code; not installing handler",
__func__);
}
}

void nr_guzzle7_minit(TSRMLS_D) {
Copy link
Member

Choose a reason for hiding this comment

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

This function differs from nr_guzzle6_minit by a single character. I would strongly suggest extracting the common code into a helper function that would accept this difference as an input. Something in the form of:

static void nr_guzzleX_minit(const int guzzle_version) {
// paste the contents of `nr_guzzle6_minit` and use guzzle_version when constructing class entry
}

void nr_guzzle6_minit() {
  nr_guzzleX_minit(6);
}

void nr_guzzle7_minit() {
  nr_guzzleX_minit(7);
}

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, I have added a helper function called nr_guzzle_minit, which reduces code duplication. 566b26e

zend_class_entry ce;

if (0 == NRINI(guzzle_enabled)) {
return;
}

INIT_CLASS_ENTRY(ce, "newrelic\\Guzzle7\\RequestHandler",
nr_guzzle6_requesthandler_functions);
nr_guzzle6_requesthandler_ce
= nr_php_zend_register_internal_class_ex(&ce, NULL TSRMLS_CC);

zend_declare_property_null(nr_guzzle6_requesthandler_ce, NR_PSTR("request"),
ZEND_ACC_PRIVATE TSRMLS_CC);
}


#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) {
zval* this_var = nr_php_scope_get(NR_EXECUTE_ORIG_ARGS TSRMLS_CC);

(void)wraprec;
NR_UNUSED_SPECIALFN;

NR_PHP_WRAPPER_CALL;
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;
}

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


void nr_guzzle6_enable(TSRMLS_D) {
int _retval;

if (0 == NRINI(guzzle_enabled)) {
return;
}

_retval = zend_eval_string(
"namespace newrelic\\Guzzle6;"

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

"if (!function_exists('newrelic\\Guzzle6\\middleware')) {"
" function middleware(callable $handler) {"
" return function (RequestInterface $request, array $options) use "
"($handler) {"

" foreach (newrelic_get_request_metadata('Guzzle 6') as $k => $v) {"
" $request = $request->withHeader($k, $v);"
" }"


" $rh = new RequestHandler($request);"
" $promise = $handler($request, $options);"
" $promise->then([$rh, 'onFulfilled'], [$rh, 'onRejected']);"

" return $promise;"
" };"
" }"
"}",
NULL, "newrelic/Guzzle6" TSRMLS_CC);

if (SUCCESS == retval) {
if (SUCCESS == _retval) {
nr_php_wrap_user_function(NR_PSTR("GuzzleHttp\\Client::__construct"),
nr_guzzle_client_construct TSRMLS_CC);
} else {
Expand Down Expand Up @@ -504,4 +629,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 */

10 changes: 6 additions & 4 deletions agent/lib_guzzle6.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,22 @@
/*
* This file contains exported functions for Guzzle 6.
*/
#ifndef LIB_GUZZLE6_HDR
#define LIB_GUZZLE6_HDR
#ifndef LIB_GUZZLE7_HDR
#define LIB_GUZZLE7_HDR
lavarou marked this conversation as resolved.
Show resolved Hide resolved

#include "php_includes.h"

/*
* Purpose : Performs tasks that we need performed on MINIT in the Guzzle 6
* Purpose : Performs tasks that we need performed on MINIT in the Guzzle 6 and 7
* instrumentation.
Comment on lines +15 to 16
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be clang formatted

Suggested change
* Purpose : Performs tasks that we need performed on MINIT in the Guzzle 6 and 7
* instrumentation.
* Purpose : Performs tasks that we need performed on MINIT in the Guzzle 6 and
* 7 instrumentation.

*/
extern void nr_guzzle6_minit(TSRMLS_D);
extern void nr_guzzle7_minit(TSRMLS_D);

/*
* Purpose : Client::__construct() wrapper for Guzzle 6.
* Purpose : Client::__construct() wrapper for Guzzle 6 and 7.
*/
extern NR_PHP_WRAPPER_PROTOTYPE(nr_guzzle6_client_construct);
extern NR_PHP_WRAPPER_PROTOTYPE(nr_guzzle7_client_construct);

#endif /* LIB_GUZZLE4_HDR */
Loading