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.

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

refactor(agent): cleanup guzzle instrumentation #498

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open

refactor(agent): cleanup guzzle instrumentation #498

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d04bf87
chore(agent): add Guzzle 7 dependencies to tar file
hahuja2 Jul 29, 2022
4c98d5a
feat(agent): add new function that checks for guzzle version
hahuja2 Jul 29, 2022
ca5c948
refactor(agent): remove emitter function that is not needed
hahuja2 Jul 29, 2022
a20c93d
test(agent): update Guzzle 6 test cases and add Guzzle 7 test cases
hahuja2 Jul 29, 2022
4b45445
feat(agent): combine Guzzle 6 instrumentation with Guzzle 7
hahuja2 Aug 1, 2022
734784e
refactor(agent/tests): remove commented out debug lines
hahuja2 Aug 18, 2022
48f8e9e
fix(agent/tests): update file paths for each Guzzle version
hahuja2 Aug 24, 2022
6ac5c13
refactor(agent/tests): add skipif.inc file to guzzle 6 and 7 test cases
hahuja2 Aug 29, 2022
d4dea40
fix(agent): restore guards to match filename
hahuja2 Aug 29, 2022
566b26e
refactor(agent): create helper function for guzzle minit
hahuja2 Aug 29, 2022
6a879ff
refactor(agent): remove guzzle client construct function
hahuja2 Aug 30, 2022
6222a51
refactor(agent): add helper functions for guzzle enable and guzzle cl…
hahuja2 Aug 31, 2022
29f4551
fix(agent): free char variables after they are used
hahuja2 Sep 21, 2022
8065805
refactor(agent): add description to free string after using
hahuja2 Sep 21, 2022
40ab8cf
fix(agent): remove guzzle version from namespace
hahuja2 Sep 22, 2022
f3ac653
docs(agent): add description for guzzle client construct function
hahuja2 Sep 22, 2022
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
47 changes: 21 additions & 26 deletions agent/lib_guzzle6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,25 @@ 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");
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

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

/*
* 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
Expand Down Expand Up @@ -468,19 +487,7 @@ void nr_guzzle7_enable(TSRMLS_D) {
}

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


Expand Down Expand Up @@ -597,19 +604,7 @@ void nr_guzzle6_enable(TSRMLS_D) {
}

void nr_guzzle6_minit(TSRMLS_D) {
zend_class_entry ce;

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

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_declare_property_null(nr_guzzle6_requesthandler_ce, NR_PSTR("request"),
ZEND_ACC_PRIVATE TSRMLS_CC);
nr_guzzle_minit(6);
}

#else /* PHP < 5.5 */
Expand Down