refactor(agent): cleanup guzzle instrumentation #498

hahuja2 · 2022-08-01T02:48:35Z

This PR implements the following changes:

Combine Guzzle 6 instrumentation with Guzzle 7
Add guzzle version check function, which returns the correct guzzle version
Remove emitter function, which checks for emitter file in guzzle 4-5 package
Add Guzzle 7 test cases
Install Guzzle 7 dependencies and create new tar file with all guzzle packages included (Guzzle 4-7)
Guzzle 7 requires PHP 7.2 or above

hahuja2 · 2022-08-16T00:13:30Z

ok jenkins

zsistla · 2022-08-16T16:35:37Z

ok jenkins

zsistla · 2022-08-16T21:08:59Z

ok jenkins

agent/lib_guzzle_common.c

tests/integration/external/guzzle6/test_dt.php

ZNeumann · 2022-08-18T16:05:48Z

tests/integration/external/guzzle7/test_cat.php

+    [{"name":"OtherTransactionTotalTime"},                [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime/php__FILE__"},    [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/library/Guzzle 4-5/detected"},
+                                                          [2,    0,    0,    0,    0,    0]],


Why is this a 2?

Thanks for catching that! I need to look further into this and figure out why the program outputs a 2 for guzzle 4-5 detection. Guzzle 7 is being correctly detected, which is good since this is a guzzle 7 test case.

Hi Zach, I figured out that it was outputting a 2 for Guzzle 4-5 because the file path "clientinterface.php" was being detected twice inside of the guzzle 7 package. This is why I have updated the file path for each guzzle version to ensure that each file path is unique, so that we only detect the correct guzzle version based on which test is being run. 48f8e9e!

ZNeumann · 2022-08-18T16:07:08Z

tests/integration/external/guzzle6/test_cat.php

@@ -64,6 +64,7 @@
    [{"name":"Supportability/library/Guzzle 4-5/detected"},
                                                          [1,    0,    0,    0,    0,    0]],
    [{"name":"Supportability/library/Guzzle 6/detected"}, [1,    0,    0,    0,    0,    0]],
+    [{"name":"Supportability/library/Guzzle 7/detected"}, [1,    0,    0,    0,    0,    0]],


For the Guzzle 6 cases, is there some limitation such that we need to have Guzzle 7 detected?

In unpack_guzzle.php, this file extracts the different guzzle packages in the tar file (4-7) and imports the correct guzzle version. Then in php_execute.c, we define the nr_guzzle6_enable and nr_guzzle7_enable functions, which is the reason why guzzle 6 and 7 are both detected in the Guzzle 6 test cases. I will further look into this and see if I can prevent guzzle 7 from being detected

Fixed 48f8e9e! I have updated the file paths, which makes sure only one guzzle version is detected based on which guzzle test is being run

lavarou

@hahuja2 Thank you for your work and contribution to make New Relic PHP Agent support the most recent version of Guzzle! 👍 I have a few suggestions that will improve future maintenance of New Relic PHP Agent by eliminating some of the code duplication and reducing complexity. Please have a look at my comments below.

tests/integration/external/guzzle7/test_dt.php

agent/lib_guzzle6.h

lavarou · 2022-08-27T01:23:38Z

agent/lib_guzzle6.c

+  }
+}
+
+void nr_guzzle7_minit(TSRMLS_D) {


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

Fixed, I have added a helper function called nr_guzzle_minit, which reduces code duplication. 566b26e

lavarou · 2022-08-27T01:35:11Z

agent/lib_guzzle6.c

@@ -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) {


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.

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

agent/lib_guzzle4.c

lavarou · 2022-08-27T02:00:20Z

agent/lib_guzzle_common.c

@@ -272,19 +315,19 @@ char* nr_guzzle_response_get_header(const char* header,
 }

 NR_PHP_WRAPPER_START(nr_guzzle_client_construct) {


Is this function still needed? If Guzzle library version is determined by a file name in nr_execute_handle_library, and correct nr_guzzleX_enable is used to enable instrumentation, then why version specific nr_guzzleX_enable calls the dispatch instrumentation (nr_guzzle_client_construct) instead of version specific instrumentation nr_guzzleX_client_construct?
I know that this function existed before this PR but maybe this PR could improve our codebase and eliminate unnecessary code complexity.

Great point Michal! This is performing an unnecessary extra step because we have already identified the correct version by determining the correct file name in nr_execute_handle_library and can instead call the specific guzzle instrumentation in nr_guzzleX_enable

I have removed this function and now directly call the correct guzzle instrumentation inside of nr_guzzle_enable! 6a879ff

lavarou · 2022-08-27T02:03:38Z

agent/lib_guzzle6.c

+zval* config;
+zend_class_entry* guzzle_client_ce;
+zval* handler_stack;
+zval* middleware = NULL;
+zval* retval;


Why do these variables need to be global?

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

hahuja2 · 2022-08-29T15:43:08Z

@misiekn Thank you Michal for the detailed feedback!

…ient

RuthAdele · 2022-09-15T08:38:28Z

What are the chances of getting this merged in and available soon (ish)?

lavarou

@hahuja2 Thank you for refactoring the code to reduce code duplication! 👍 After looking at the code again I come to the conclusion that including Guzzle version in the namespace (newrelic\Guzzle6 or newrelic\Guzzle7) adds unnecessary code complexity and introduces a few memory leaks. I do not think the Guzzle version is necessary in the namespace name. Please have a look at my comments below which have more details.

lavarou · 2022-09-20T02:46:57Z

agent/lib_guzzle6.c

+  zend_class_entry ce;
+  char guzzle_path[] = "newrelic\\Guzzle6\\RequestHandler";
+  if (guzzle_version == 7){
+    nr_strcpy(guzzle_path, "newrelic\\Guzzle7\\RequestHandler");


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); ...

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

lavarou · 2022-09-20T03:01:15Z

agent/lib_guzzle6.c

+   * 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", 


I don't see this being freed.

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

lavarou · 2022-09-20T03:02:55Z

agent/lib_guzzle6.c

@@ -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(


I don't see eval being freed...

Fixed! 40ab8cf

lavarou · 2022-09-20T03:04:19Z

agent/lib_guzzle6.c

-      NULL, "newrelic/Guzzle6" TSRMLS_CC);
+      "}", guzzle_version, guzzle_version, guzzle_version);
+
+  char *guzzle_ver = nr_formatf("newrelic/Guzzle%d", guzzle_version);


I don't see guzzle_ver being freed...

Fixed 40ab8cf

lavarou · 2022-09-20T03:06:54Z

agent/lib_guzzle6.c

+  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);


Does version need to be freed?

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

lavarou · 2022-09-20T03:08:09Z

agent/lib_guzzle_common.h

- * Returns : Non-zero if the object does implement the interface; zero
- *           otherwise.
+ * 
+ * Returns : A string indicating the guzzle version being used


Does this function allocate a new string? Is the caller responsible for freeing the result?

Yes, it allocates a new string and the caller is responsible for freeing the returned result. I have updated the description of the function to include that the returned string must be freed. 8065805

lavarou · 2022-09-20T03:15:11Z

agent/lib_guzzle6.c

+  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 {


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.

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

lavarou · 2022-09-20T03:21:29Z

agent/lib_guzzle6.c

    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){


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

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

lavarou · 2022-09-20T03:24:25Z

agent/lib_guzzle6.c

+                     "%s: middleware string is not considered callable",
+                     __func__);
+
+    char* error_message = nr_formatf(


I don't see error_message being freed.

Fixed! 29f4551

bduranleau-nr · 2022-09-28T16:21:30Z

agent/lib_guzzle6.c

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


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*);

bduranleau-nr · 2022-09-28T16:40:24Z

agent/lib_guzzle6.c

@@ -504,4 +522,4 @@ void nr_guzzle6_minit(TSRMLS_D) {
  NR_UNUSED_TSRMLS;
 }

-#endif /* 5.5.x */
+#endif /* 5.5.x */


Suggested change

#endif /* 5.5.x */

#endif /* 5.5.x */

bduranleau-nr · 2022-09-28T16:42:34Z

agent/lib_guzzle6.h

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


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.

bduranleau-nr · 2022-09-28T16:43:48Z

agent/lib_guzzle6.c

@@ -9,14 +9,14 @@
 * It is a required component in Drupal 8, and strongly recommended by other


This file needs to be clang formatted

bduranleau-nr · 2022-09-28T17:58:54Z

There's a tar.bz2 file that has been committed... tests/include/guzzle.tar.bz2
Was this intentional?
There are no other tar files in the repository, and I don't think we should depart from that format here. I would suggest removing guzzle.tar.bz2 from the repository

ZNeumann · 2022-09-28T18:16:15Z

There's a tar.bz2 file that has been committed... tests/include/guzzle.tar.bz2 Was this intentional? There are no other tar files in the repository, and I don't think we should depart from that format here. I would suggest removing guzzle.tar.bz2 from the repository

There are other tar files in that directory as well. It's where we put the libraries to run for tests, so we have guzzle and monolog in there. It needs to be changed because this PR is adding guzzle 7 to it.

bduranleau-nr

Great job!
Made a few suggestions for changes, nothing major, other than that looks good to go 👍

bduranleau-nr · 2022-09-28T18:30:20Z

There are other tar files in that directory as well. It's where we put the libraries to run for tests, so we have guzzle and monolog in there. It needs to be changed because this PR is adding guzzle 7 to it.

Is there something unique to guzzle in regards to testing that makes it so it requires this tar when the other frameworks don't?

bduranleau-nr · 2022-09-28T18:31:56Z

There are other tar files in that directory as well.

I did a search of the entire codebase for *.tar* and this tar file was the only one that came up.

ZNeumann · 2022-09-28T19:13:33Z

There are other tar files in that directory as well. It's where we put the libraries to run for tests, so we have guzzle and monolog in there. It needs to be changed because this PR is adding guzzle 7 to it.

Is there something unique to guzzle in regards to testing that makes it so it requires this tar when the other frameworks don't?

Added here b5c11b9 for monolog, and long before that for guzzle.

I don't know why it was decided to do it this way, but it seems like libraries (as opposed to frameworks) are added via tars in this directory.

bduranleau-nr · 2022-09-28T19:36:15Z

Added here b5c11b9 for monolog, and long before that for guzzle.

I see- I was searching the checkout of this PR, so it was missing (a lot) of the more recent changes, including the monolog2/3 tars

I don't know why it was decided to do it this way, but it seems like libraries (as opposed to frameworks) are added via tars in this directory.

Maybe some kind of a README in the tests/ or tests/include/ directory with an explanation of what the tar file is and why it is implemented this way would be a good addition?

lavarou · 2023-08-16T18:42:04Z

Guzzle integration tests have been enhanced in #713. That enhancement contains integration tests from this PR. No other changes from this PR have been moved over.

joshua-bn · 2023-08-30T14:14:12Z

This PR has been open for 13 months at this point. Is there something missing that needs to be added?

lavarou · 2023-08-30T16:34:05Z

This PR has been open for 13 months at this point. Is there something missing that needs to be added?

@joshua-bn @RuthAdele Thank you for your interest in New Relic PHP Agent. While this PR refactors the code used to instrument Guzzle, New Relic PHP Agent supports Guzzle 7 - #713 added tests to verify Guzzle 7 instrumentation. New Relic PHP Agent documentation has also been recently updated to reflect that Guzzle 7 is supported - see here.

scottbuscemi · 2024-04-06T04:23:37Z

Hi, what's the status of this PR? It's affecting site performance for our platform.

hahuja2 added 5 commits July 29, 2022 16:39

chore(agent): add Guzzle 7 dependencies to tar file

d04bf87

feat(agent): add new function that checks for guzzle version

4c98d5a

refactor(agent): remove emitter function that is not needed

ca5c948

test(agent): update Guzzle 6 test cases and add Guzzle 7 test cases

a20c93d

feat(agent): combine Guzzle 6 instrumentation with Guzzle 7

4b45445

hahuja2 force-pushed the guzzle7 branch from 4b45445 to 8ce07e7 Compare August 1, 2022 03:28

hahuja2 force-pushed the guzzle7 branch from 8ce07e7 to 137f620 Compare August 15, 2022 18:22

hahuja2 marked this pull request as ready for review August 15, 2022 19:05

hahuja2 requested review from lavarou, zsistla, bduranleau-nr, mfulb and ZNeumann August 15, 2022 19:06

newrelic deleted a comment from nr-opensource-bot Aug 16, 2022

ZNeumann reviewed Aug 18, 2022

View reviewed changes

agent/lib_guzzle_common.c Outdated Show resolved Hide resolved

ZNeumann reviewed Aug 18, 2022

View reviewed changes

tests/integration/external/guzzle6/test_dt.php Outdated Show resolved Hide resolved

ZNeumann reviewed Aug 18, 2022

View reviewed changes

refactor(agent/tests): remove commented out debug lines

734784e

hahuja2 force-pushed the guzzle7 branch from 137f620 to 734784e Compare August 18, 2022 18:13

fix(agent/tests): update file paths for each Guzzle version

48f8e9e

hahuja2 added the ready for review label Aug 24, 2022

lavarou requested changes Aug 28, 2022

View reviewed changes

hahuja2 added 3 commits August 29, 2022 12:21

refactor(agent/tests): add skipif.inc file to guzzle 6 and 7 test cases

6ac5c13

fix(agent): restore guards to match filename

d4dea40

refactor(agent): create helper function for guzzle minit

566b26e

hahuja2 added 2 commits August 30, 2022 16:51

refactor(agent): remove guzzle client construct function

6a879ff

refactor(agent): add helper functions for guzzle enable and guzzle cl…

6222a51

…ient

lavarou requested changes Sep 20, 2022

View reviewed changes

hahuja2 added 2 commits September 21, 2022 08:51

fix(agent): free char variables after they are used

29f4551

refactor(agent): add description to free string after using

8065805

mfulb added this to the Upcoming Release milestone Sep 21, 2022

hahuja2 added 2 commits September 22, 2022 13:34

fix(agent): remove guzzle version from namespace

40ab8cf

docs(agent): add description for guzzle client construct function

f3ac653

bduranleau-nr reviewed Sep 28, 2022

View reviewed changes

bduranleau-nr requested changes Sep 28, 2022

View reviewed changes

lavarou removed this from the Upcoming Release milestone Oct 26, 2022

lavarou mentioned this pull request Aug 17, 2023

refactor(agent): improve guzzle detection #714

Merged

lavarou changed the title ~~feat(agent): Add Guzzle 7 Library Support~~ refactor(agent): cleanup guzzle instrumentation Aug 30, 2023

lavarou added on hold This issue or pull request is necessary, but better suited for the future and removed ready for review labels Dec 6, 2023

		@@ -272,19 +315,19 @@ char* nr_guzzle_response_get_header(const char* header,
		}

		NR_PHP_WRAPPER_START(nr_guzzle_client_construct) {

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

		@@ -9,14 +9,14 @@
		* It is a required component in Drupal 8, and strongly recommended by other

refactor(agent): cleanup guzzle instrumentation #498

Are you sure you want to change the base?

refactor(agent): cleanup guzzle instrumentation #498

Conversation

hahuja2 commented Aug 1, 2022

hahuja2 commented Aug 16, 2022

zsistla commented Aug 16, 2022

zsistla commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavarou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahuja2 Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

hahuja2 commented Aug 29, 2022

RuthAdele commented Sep 15, 2022

lavarou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahuja2 Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahuja2 Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahuja2 Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bduranleau-nr commented Sep 28, 2022

ZNeumann commented Sep 28, 2022

bduranleau-nr left a comment

Choose a reason for hiding this comment

bduranleau-nr commented Sep 28, 2022

bduranleau-nr commented Sep 28, 2022

ZNeumann commented Sep 28, 2022

bduranleau-nr commented Sep 28, 2022

lavarou commented Aug 16, 2023

joshua-bn commented Aug 30, 2023

lavarou commented Aug 30, 2023 • edited Loading

scottbuscemi commented Apr 6, 2024

hahuja2 Aug 31, 2022 •

edited

Loading

hahuja2 Sep 21, 2022 •

edited

Loading

hahuja2 Sep 21, 2022 •

edited

Loading

hahuja2 Sep 21, 2022 •

edited

Loading

lavarou commented Aug 30, 2023 •

edited

Loading