Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
feat(agent): add new function that checks for guzzle version
  • Loading branch information
hahuja2 committed Jul 29, 2022
commit 4c98d5a65b87767a610da4cb947aab714c5050ea
64 changes: 54 additions & 10 deletions agent/lib_guzzle_common.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,22 @@
#include "php_user_instrument.h"
#include "php_hash.h"
#include "php_wrapper.h"
#include "php_execute.h"
#include "php_globals.h"
#include "lib_guzzle_common.h"
#include "lib_guzzle4.h"
#include "lib_guzzle6.h"
#include "fw_laravel.h"
#include "fw_laravel_queue.h"
#include "fw_support.h"
#include "php_error.h"
#include "nr_header.h"
#include "util_logging.h"
#include "util_memory.h"
#include "util_strings.h"

int php_version_compare(char*, char*);

char* nr_guzzle_create_async_context_name(const char* prefix, const zval* obj) {
if (!nr_php_is_zval_valid_object(obj)) {
return NULL;
Expand Down Expand Up @@ -74,9 +82,44 @@ int nr_guzzle_in_call_stack(TSRMLS_D) {
return in_guzzle;
}

int nr_guzzle_does_zval_implement_has_emitter(zval* obj TSRMLS_DC) {
return nr_php_object_instanceof_class(
obj, "GuzzleHttp\\Event\\HasEmitterInterface" TSRMLS_CC);
extern char* nr_guzzle_version(zval* obj TSRMLS_DC) {
char* retval = NULL;
zval* version = NULL;
zend_class_entry* ce = NULL;

if (0 == nr_php_is_zval_valid_object(obj)) {
nrl_verbosedebug(NRL_FRAMEWORK, "%s: Application object is invalid",
__func__);
return NULL;
}

ce = Z_OBJCE_P(obj);
if (NULL == ce) {
nrl_verbosedebug(NRL_FRAMEWORK, "%s: Application has NULL class entry",
__func__);
return NULL;
}

if (nr_php_get_class_constant(ce, "VERSION") == NULL){
version = nr_php_get_class_constant(ce, "MAJOR_VERSION");
} else {
version = nr_php_get_class_constant(ce, "VERSION");
}
if (NULL == version) {
nrl_verbosedebug(NRL_FRAMEWORK, "%s: Application does not have VERSION",
__func__);
return NULL;
}

if (nr_php_is_zval_valid_string(version)) {
retval = nr_strndup(Z_STRVAL_P(version), Z_STRLEN_P(version));
} else {
nrl_verbosedebug(NRL_FRAMEWORK,
"%s: expected VERSION be a valid string, got type %d",
__func__, Z_TYPE_P(version));
}
nr_php_zval_free(&version);
return retval;
}

nr_segment_t* nr_guzzle_obj_add(const zval* obj,
Expand Down Expand Up @@ -272,19 +315,20 @@ char* nr_guzzle_response_get_header(const char* header,
}

NR_PHP_WRAPPER_START(nr_guzzle_client_construct) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

int is_guzzle_45 = 0;
zval* this_var = nr_php_scope_get(NR_EXECUTE_ORIG_ARGS TSRMLS_CC);
char *version = NULL;
version = nr_guzzle_version(this_var TSRMLS_CC);

(void)wraprec;
NR_UNUSED_SPECIALFN;

is_guzzle_45 = nr_guzzle_does_zval_implement_has_emitter(this_var TSRMLS_CC);
nr_php_scope_release(&this_var);

if (is_guzzle_45) {
NR_PHP_WRAPPER_DELEGATE(nr_guzzle4_client_construct);
} else {
if (php_version_compare(version, "7") >= 0){
NR_PHP_WRAPPER_DELEGATE(nr_guzzle7_client_construct);
} else if (php_version_compare(version, "6") >= 0) {
NR_PHP_WRAPPER_DELEGATE(nr_guzzle6_client_construct);
} else{
NR_PHP_WRAPPER_DELEGATE(nr_guzzle4_client_construct);
}
}
NR_PHP_WRAPPER_END
11 changes: 4 additions & 7 deletions agent/lib_guzzle_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,13 @@ extern char* nr_guzzle_create_async_context_name(const char* prefix,
extern int nr_guzzle_in_call_stack(TSRMLS_D);

/*
* Purpose : Checks if the given object implements
* GuzzleHttp\Event\HasEmitterInterface. For a Client object, this
* indicates that the object is from Guzzle 4 or 5.
* Purpose: This function checks which guzzle version is being used by the object
*
* Params : 1. The object to check.
*
* Returns : Non-zero if the object does implement the interface; zero
* otherwise.
*
* Returns : A string indicating the guzzle version being used
Copy link
Member

Choose a reason for hiding this comment

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

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

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, 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

*/
extern int nr_guzzle_does_zval_implement_has_emitter(zval* obj TSRMLS_DC);
extern char* nr_guzzle_version(zval* obj TSRMLS_DC);

/*
* Purpose : Adds a Guzzle Request object to the hashmap containing all active
Expand Down