refactor(agent): cleanup guzzle instrumentation #498
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,14 +9,14 @@ | |||||||
| * It is a required component in Drupal 8, and strongly recommended by other | ||||||||
|
There was a problem hiding this comment. This file needs to be clang formatted |
||||||||
| * frameworks, including Symfony 2 and 3. | ||||||||
| * | ||||||||
| * 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-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 | ||||||||
|
|
@@ -65,12 +65,11 @@ | |||||||
| */ | ||||||||
| #if ZEND_MODULE_API_NO >= ZEND_5_5_X_API_NO | ||||||||
|
|
||||||||
| /* | ||||||||
| * True global for the RequestHandler class entry. | ||||||||
| */ | ||||||||
| zend_class_entry* nr_guzzle6_requesthandler_ce; | ||||||||
| int php_version_compare(char*, char*); | ||||||||
|
There was a problem hiding this comment. This seems unnecessary. Instead,
Suggested change
|
||||||||
|
|
||||||||
| /* | ||||||||
| * Arginfo for the RequestHandler methods. | ||||||||
|
|
@@ -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-7"}; | ||||||||
| zval* request; | ||||||||
| zval* method; | ||||||||
| zval* status; | ||||||||
|
|
@@ -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-7' %s=" NRP_FMT, | ||||||||
| X_NEWRELIC_APP_DATA, NRP_CAT(external_params.encoded_response_header)); | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -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-7" TSRMLS_CC); | ||||||||
| } | ||||||||
|
|
||||||||
| /* | ||||||||
| * Proto : void RequestHandler::onFulfilled(Psr\Http\Message\ResponseInterface | ||||||||
| * $response) | ||||||||
| * | ||||||||
| * Purpose : Called when a Guzzle 6-7 request promise is fulfilled. | ||||||||
| * | ||||||||
| * Params : 1. The response object. | ||||||||
| */ | ||||||||
|
|
@@ -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-7 request promise failed. | ||||||||
| * | ||||||||
| * Params : 1. The exception object. | ||||||||
| */ | ||||||||
|
|
@@ -340,76 +339,27 @@ const zend_function_entry nr_guzzle6_requesthandler_functions[] | |||||||
| nr_guzzle6_requesthandler_onrejected_arginfo, | ||||||||
| ZEND_ACC_PUBLIC) PHP_FE_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"); | ||||||||
|
There was a problem hiding this comment. 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: There was a problem hiding this comment. 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 |
||||||||
| } | ||||||||
| if (0 == NRINI(guzzle_enabled)) { | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| 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); | ||||||||
|
|
||||||||
| zend_declare_property_null(nr_guzzle6_requesthandler_ce, NR_PSTR("request"), | ||||||||
| ZEND_ACC_PRIVATE TSRMLS_CC); | ||||||||
| } | ||||||||
|
|
||||||||
| static void nr_guzzle_enable(const int guzzle_version){ | ||||||||
| int _retval; | ||||||||
|
|
||||||||
| if (0 == NRINI(guzzle_enabled)) { | ||||||||
| return; | ||||||||
|
|
@@ -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. | ||||||||
| */ | ||||||||
| char *eval = nr_formatf( | ||||||||
|
There was a problem hiding this comment. I don't see |
||||||||
| "namespace newrelic\\Guzzle%d;" | ||||||||
|
|
||||||||
| "use Psr\\Http\\Message\\RequestInterface;" | ||||||||
|
|
||||||||
| /* | ||||||||
| * 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) {" | ||||||||
|
|
||||||||
| " foreach (newrelic_get_request_metadata('Guzzle %d') as $k => $v) {" | ||||||||
| " $request = $request->withHeader($k, $v);" | ||||||||
| " }" | ||||||||
|
|
||||||||
|
|
@@ -458,33 +408,142 @@ void nr_guzzle6_enable(TSRMLS_D) { | |||||||
| " return $promise;" | ||||||||
| " };" | ||||||||
| " }" | ||||||||
| "}", guzzle_version, guzzle_version, guzzle_version); | ||||||||
|
|
||||||||
| char *guzzle_ver = nr_formatf("newrelic/Guzzle%d", guzzle_version); | ||||||||
|
There was a problem hiding this comment. I don't see |
||||||||
| _retval = zend_eval_string(eval, NULL, guzzle_ver TSRMLS_CC); | ||||||||
|
|
||||||||
| if (SUCCESS == _retval && guzzle_version == 6) { | ||||||||
| nr_php_wrap_user_function(NR_PSTR("GuzzleHttp\\Client::__construct"), | ||||||||
| 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 { | ||||||||
|
There was a problem hiding this comment. I think this code can be simplified further because There was a problem hiding this comment. 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__); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| NR_PHP_WRAPPER_START(nr_guzzle_client_construct_helper){ | ||||||||
|
There was a problem hiding this comment. Maybe add a brief doc comment describing what this function does? I.e. it adds There was a problem hiding this comment. 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); | ||||||||
|
There was a problem hiding this comment. 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; | ||||||||
| } | ||||||||
|
|
||||||||
| (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", | ||||||||
|
There was a problem hiding this comment. 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( | ||||||||
|
There was a problem hiding this comment. I don't see |
||||||||
| "Supportability/library/Guzzle %d/MiddlewareNotCallable", guzzle_version); | ||||||||
| nrm_force_add(NRTXN(unscoped_metrics), error_message, 0); | ||||||||
| goto end; | ||||||||
| } | ||||||||
|
|
||||||||
| 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; | ||||||||
| } | ||||||||
|
|
||||||||
| 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 */ | ||||||||
|
|
@@ -504,4 +563,4 @@ void nr_guzzle6_minit(TSRMLS_D) { | |||||||
| NR_UNUSED_TSRMLS; | ||||||||
| } | ||||||||
|
|
||||||||
| #endif /* 5.5.x */ | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.