feat(agent): Add RabbitMQ instrumentation using the php-amqplib library #1009
Conversation
Initial commit does the following: * Detect library via magic file * Detect package and version information. * Basic unit tests
|
Initial commit does the following: * Detect library via magic file * Detect package and version information. * Basic unit tests
zsistla
force-pushed
the
feat/rabbitmq_instrumentation
branch
from
2df782f to
0b184ca
Compare
…ewrelic-php-agent into feat/rabbitmq_instrumentation
Test version detection when class exists but version const doesn't. Fixed typos.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1009 +/- ##
==========================================
- Coverage 78.02% 77.44% -0.59%
==========================================
Files 197 198 +1
Lines 27418 27700 +282
==========================================
+ Hits 21392 21451 +59
- Misses 6026 6249 +223
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
* Creates message segment on basic_publish call.
* While the RabbitMQ tutorial for using with the dockerized RabbitMQ setup * correctly and loads the PhpAmqpLib\\Channel\\AMQPChannel class in time for * the agent to wrap the instrumented functions, there are AWS MQ_BROKER * specific but valid scenarios where the PhpAmqpLib\\Channel\\AMQPChannel class * file does not explicitly load or does not load in time, and the instrumented * functions are NEVER wrapped regardless of how many times they are called in * one txn. Specifically, this centered around the very slight but impactful * differences when using the PhpAmqpLib\Connection\AMQPStreamConnection which * causes an explicit load of the AMQPChannel class/file and * PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load * of the AMQPChannelclass/file. The following method is thus the only way to * ensure the class is loaded in time for the functions to be wrapped.
| result = zend_eval_string( | ||
| "(function() {" | ||
| " $nr_php_amqplib_version = '';" | ||
| " try {" |
There was a problem hiding this comment.
I believe PHP provides tools to make sure this constant exists so it would avoid triggering an exception in the first place.
There was a problem hiding this comment.
That would be doing an PHP extra call when this call already verifies and retrieves the value and is future proofed against any future exceptions. Additionally, it will automatically load the class if it's not already loaded.
There was a problem hiding this comment.
I am not confident in the ability to trap all types of errors using this pattern - we have seen something similar fail in the current Drupal PR. I would be curious what the performance overhead is for a call to defined() would incur, as it the most direct way to prevent a failure. If I understand this code correctly this extra function call would only occur once per request so it would hopefully have an undetectable impact.
There was a problem hiding this comment.
The Drupal case is a lot more complicated.
This case is only referencing a const in a class.
There are only a limited amount of errors that could occur:
- the class doesn't exist
2)the const doesn't exist.
There are unit tests that verify both of these cases are handled.
The defined() is unneeded in this case.
There was a problem hiding this comment.
Why not use nr_php_get_class_constant like is done for Drupal here?
There was a problem hiding this comment.
A couple of reasons.
- That would be two calls vs one that is occuring now.
- if the class is not loaded, the nr_php_find_class call will fail since the class isn't loaded. The eval_string forces it to load. This was the reason the aws versioning does what it does. It initially used the same method as drupal did, but that meant it HAD to be placed in a function that guaranteed it existed. That function went away, so instead of being reliant on functions that may or may go away to guarantee this file is loaded and waiting until it's called to determine package info; we decoupled the agent from that being reliant on any particular function call and are able to detect the version as soon as we detect the library.
- I fail to see why the other method is more advantageous.
There was a problem hiding this comment.
- That would be two calls vs one that is occuring now.
The two calls you point out are hashmap lookups: nr_php_find_class and nr_php_get_constant which is a wrapper around zend_get_constant. Call to zend_eval_string is single, however under the hood zend_eval_string does much more - it is a compilation and an execution of a PHP script which is more involved than hashmap lookup.
- if the class is not loaded, the nr_php_find_class call will fail since the class isn't loaded. The eval_string forces it to load
The zend_eval_string does not force the class to load. The reason why it works, and the only way it works, is if the project uses autoloader and autoloader's code has been loaded at the time the code passed to zend_eval_string executes.
- I fail to see why the other method is more advantageous.
nr_php_get_class_constant has much smaller overhead.
There was a problem hiding this comment.
Comparing the two methods like that is a bit simplistic because it fails to account for different overheads involved with Drupal's version detection.
Drupal only works because it wraps a file here: https://github.com/newrelic/newrelic-php-agent/blob/main/agent/php_execute.c#L549
To call into the detect version function.
Which means drupal version is only detected when vulnerability_management_package_detection_enabled is enabled and it also means it adds additional overhead of one more file lookup every time any file is loaded
https://github.com/newrelic/newrelic-php-agent/blob/main/agent/php_execute.c#L972
Whereas the current rabbitmq version detection only incurs overhead once on package detection and not every single time a file is loaded for ANY framework.
Furthermore, rabbitmq doesn't have a file that is loaded that it can hinge on.
These are the loaded files when connecting to a regular rabbitmq server:
2025-02-05 15:06:30.271 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/Constants091.php'}
2025-02-05 15:06:30.272 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AMQPStreamConnection.php'}
2025-02-05 15:06:30.274 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AbstractConnection.php'}
2025-02-05 15:06:30.276 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Channel/AMQPChannel.php'}
2025-02-05 15:06:30.277 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/Constants080.php'}
2025-02-05 15:06:30.278 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/IO/StreamIO.php'}
2025-02-05 15:06:30.279 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPWriter.php'}
2025-02-05 15:06:30.280 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPAbstractCollection.php'}
2025-02-05 15:06:30.284 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPBufferReader.php'}
2025-02-05 15:06:30.285 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPReader.php'}
2025-02-05 15:06:30.287 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPIOReader.php'}
2025-02-05 15:06:30.306 +0000 (4496 4496) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPTable.php'}
PhpAmqpLib\Package.php is never explicitly loaded as a file, so we couldn't even link to it if we wanted. We are forced to load the class as currently implemented.
agent/lib_php_amqplib.c
Outdated
| amqp_port | ||
| = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), | ||
| AMQP_CONSTRUCT_PARAMS_PORT_INDEX); | ||
| if (nr_php_is_zval_valid_scalar(amqp_port)) { |
There was a problem hiding this comment.
This checks if it is a bool, a valid string, long or double. Are these all valid values for the port?
There was a problem hiding this comment.
Good point, only allow long:
095f82d
agent/lib_php_amqplib.c
Outdated
| = nr_php_zend_hash_index_find(Z_ARRVAL_P(connect_constructor_params), | ||
| AMQP_CONSTRUCT_PARAMS_PORT_INDEX); | ||
| if (nr_php_is_zval_valid_scalar(amqp_port)) { | ||
| message_params->server_port = Z_LVAL_P(amqp_port); |
There was a problem hiding this comment.
The check above does not guarantee the value is a long.
There was a problem hiding this comment.
verify long: 095f82d
There was a problem hiding this comment.
Is it possible for this value to be a string? Does the AWS SDK define the allowable types?
There was a problem hiding this comment.
php-amqplib only allows integers > 0.
| */ | ||
| message_params.destination_name | ||
| = ENSURE_PERSISTENCE(Z_STRVAL_P(amqp_exchange)); | ||
| } else { |
There was a problem hiding this comment.
What if ampq_exchange is not a string at all?
There was a problem hiding this comment.
Only create default if it is an empty string:
ead28c2
agent/lib_php_amqplib.c
Outdated
| * strdup server_address, destination_name, and | ||
| * messaging_destination_routing_key. | ||
| */ | ||
| nr_free(message_params.server_address); |
There was a problem hiding this comment.
I think it could be cleaner to have a matching macro to ENSURE_PERSISTENCE that could be used to un-persist these values in the appropriate way for the PHP is use.
There was a problem hiding this comment.
Great idea!
71e5e02
| * Default in case of empty string. */ | ||
| message_params.messaging_destination_publish_name | ||
| = Z_STRVAL_P(amqp_exchange); | ||
| } else { |
There was a problem hiding this comment.
What if ampq_exchange is not a string? (This applies to this pattern in any wrappers in this PR).
There was a problem hiding this comment.
The function param list and/or class definition dictates strings or not.
nr_php_is_zval_non_empty_string verifies that the value is not null, is not empty, and is a zval string. Only after that do we do anything with it.
There was a problem hiding this comment.
Under the hood this is using zend_read_property() which can return many data types so my understanding is a property is not guaranteed to be a string. Is there some condition that allows us to be sure these properties are strings?
There was a problem hiding this comment.
The check nr_php_is_zval_non_empty_string verifies the property is a string before we do anything with it. If it's not a string, we ignore it.
There was a problem hiding this comment.
Only set default if it is a valid but empty string
ead28c2
|
|
||
| /* Extract the version for aws-sdk 3+ */ | ||
| nr_php_amqplib_handle_version(); | ||
| nr_php_amqplib_ensure_class(); |
There was a problem hiding this comment.
I am concerned on how this all works - if the file used to trigger this enable function is, for example, in a sequence of PHP calls that was in response to autoloading the class we need, and inside this sequence we start another sequence to load this class - is composer setup to handle this?
Is there not a file we can use in the situations this class is not triggered (the AWS context I guess?).
There was a problem hiding this comment.
No. None of the files (autoload files or files in general that get loaded through the course of the test case) can cause this class to be loaded in order to wrap the function properly during AWS test cases. This is different behavior from the rabbitmq server used with multiverse so the difference is either due to the slight variance in making an SSL connection or due to the older rabbitmq engine that is running in AWS. None of the files that loaded when running AWS tests worked to allow our instrumentation to be wrapped (different than the non-ssl, non-aws rabbitmq server which actually loaded the file as well as the class).
All this is doing is calling class_exists so pretty innocuous. This is a php function which other functions can call in the middle of their logic and have easily handled as well. It's analogous to function calls that make calls to other classes. If you turn the show_* on, classes get loaded inside classes all the time because unless the file is loaded previously, the class doesn't actually get loaded until it's used.
Yes composer can handle it. That change was enough to finally get the functions wrapped for AWS setups, and the segments were created and made it to the backend with appropriate attributes.
agent/lib_php_amqplib.c
Outdated
| zval retval_dtor; | ||
| int result = FAILURE; | ||
|
|
||
| result = zend_eval_string("class_exists('PhpAmqpLib\\Channel\\AMQPChannel');", | ||
| &retval_dtor, "Get nr_php_amqplib_class_exists"); | ||
| /* | ||
| * We don't need to check anything else at this point. If this fails, there's | ||
| * nothing else we can do anyway. | ||
| */ | ||
|
|
||
| zval_dtor(&retval_dtor); |
There was a problem hiding this comment.
If return value from class_exists('PhpAmqpLib\Channel\AMQPChannel'); is not verified, why is it passed to zend_eval_string? Also I really don't understand why neither result nor return value from class_exists('PhpAmqpLib\Channel\AMQPChannel'); are not verified. What is the purpose of nr_php_amqplib_ensure_class?
There was a problem hiding this comment.
It loads the class if it is loadable. The call itself is sufficient to load the class. We could check but there's nothing we would do differently either way.
This big block of text explains the purpose.
There was a problem hiding this comment.
[...] there are AWS MQ_BROKER specific but valid scenarios where the PhpAmqpLib\Channel\AMQPChannel class file does not explicitly load or does not load in time, and the instrumented functions are NEVER wrapped regardless of how many times they are called in one txn [...]
is very vague. Could you give an example?
This part is also not very clear to me:
[...] Specifically, this centered around the very slight but impactful differences when using managing the AWS MQ Broker connect vs causes an explicit load of the AMQPChannel class/file and PhpAmqpLib\Connection\AMQPSSLConnection which does NOT cause an explicit load of the AMQPChannelclass/file [...]
It feels like there some extra/missing verbs?
There was a problem hiding this comment.
Basically, the way AWS support rabbitmq in their MQ_BROKER differs slightly from official rabbitmq servers.
So this added function is needed only to support AWS's MQ_BROKER.
when connecting via SSL with rabbitmq's official server, these are the php-amqplib files that show up:
2025-02-05 15:28:29.672 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/Constants091.php'}
2025-02-05 15:28:29.673 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AMQPStreamConnection.php'}
2025-02-05 15:28:29.675 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AbstractConnection.php'}
2025-02-05 15:28:29.680 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Channel/AMQPChannel.php'}
2025-02-05 15:28:29.680 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/Constants080.php'}
2025-02-05 15:28:29.682 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/IO/StreamIO.php'}
2025-02-05 15:28:29.684 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPWriter.php'}
2025-02-05 15:28:29.686 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPAbstractCollection.php'}
2025-02-05 15:28:29.703 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPBufferReader.php'}
2025-02-05 15:28:29.704 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPReader.php'}
2025-02-05 15:28:29.706 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPIOReader.php'}
2025-02-05 15:28:29.731 +0000 (4543 4543) verbosedebug: execute: file={'/multiverse/tests/php-amqplib/3.7.x/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPTable.php'}
When using THE SAME file, with only changes in the server name for the connection, these are the files that show up:
2025-01-17 18:44:08.370 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/Constants091.php'}
2025-01-17 18:44:08.371 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AMQPStreamConnection.php'}
2025-01-17 18:44:08.371 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AbstractConnection.php'}
2025-01-17 18:44:08.373 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/IO/StreamIO.php'}
2025-01-17 18:44:08.373 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPWriter.php'}
2025-01-17 18:44:08.375 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPAbstractCollection.php'}
2025-01-17 18:44:08.546 +0000 (2411 2411) verbosedebug: execute: file={'/home/ubuntu/phptest/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Wire/AMQPBufferReader.php'}
202
You can see the missing, but very key PhpAmqpLib/Channel/AMQPChannel.php file never gets explicitly loaded in the AWS version of events. The class is not automatically loaded, but is available and can be resolved if called from within PHP. Because of this the functions we instrument NEVER get wrapped when connecting to the MQ_BROKER and therefore our instrumentation is never triggered. The explicit loading of the class is therefore needed to work with MQ_BROKER.
There was a problem hiding this comment.
Updated comment to clarify: 0e6ec90
There was a problem hiding this comment.
But also correct, the retval is not needed since we aren't using it.
23a8a34
agent/php_execute.c
Outdated
| @@ -491,6 +491,10 @@ static nr_library_table_t libraries[] = { | |||
|
|
|||
| {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, | |||
|
|
|||
| /* php-amqplib RabbitMQ >= 3.7 */ | |||
There was a problem hiding this comment.
I don't think this comment is accurate - phpamqplib/connection/abstractconnection.php file also exists in version 2.x (starting with version v2.0.2) of php-amqplib.
There was a problem hiding this comment.
That's a good point. The should say something like PHP Agent Supports RabbitMQ >= 3.7
They aren't maintaining 2.x and the highest version works with PHP < 8.
Preliminary testing shows it basically works ok and creates the message segments.
I updated the comment here: 127e62a.
| {"php-amqplib", NR_PSTR("phpamqplib/connection/abstractconnection.php"), | ||
| nr_php_amqplib_enable}, |
There was a problem hiding this comment.
Since phpamqplib/connection/abstractconnection.php file also exists in version 2.x of php-amqplib (starting with version v2.0.2), this code will enable agent's instrumentation of php-amqplib in projects that use version 2 of php-amqplib. Is this safe?
There was a problem hiding this comment.
Preliminary testing shows it basically works fine and creates the message segments, but we aren't fully testing it.
Would your vote be to allow it or to force a check and not instrument if it's a version below 3.7. This would involve adding some string version compare logic.
There was a problem hiding this comment.
Are the signatures of PhpAmqpLib\Channel\AMQPChannel::basic_publish and PhpAmqpLib\Channel\AMQPChannel::basic_get methods the same between 2.x and 3.x?
Co-authored-by: Michal Nowacki <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
agent/lib_php_amqplib.c
Outdated
|
|
||
| amqp_port = nr_php_zend_hash_index_find( | ||
| Z_ARRVAL_P(connect_constructor_params), AMQP_CONSTRUCT_PARAMS_PORT_INDEX); | ||
| if (IS_LONG != Z_TYPE_P((amqp_port))) { |
There was a problem hiding this comment.
nr_php_is_zval_valid_integer is what is needed here (it also checks for NULL):
| if (IS_LONG != Z_TYPE_P((amqp_port))) { | |
| if (nr_php_is_zval_valid_integer(amqp_port)) { |
| = Z_STRVAL_P(amqp_routing_key); | ||
| } | ||
|
|
||
| nr_php_amqplib_retrieve_dt_headers(*retval_ptr); |
There was a problem hiding this comment.
This function has a side effect - it sets DT headers in the current transaction (it eventually calls nr_txn_accept_distributed_trace_payload). This is not very obvious when reading the name of this function, so leaving this comment for future reviewers and maintainers.
Most of the PR is basic instrumentation, retrieval of values and setting attributes or creating metrics and is very similar to patterns we've established in other instrumentation.
The DT header insertion logic is the trickiest bit since we are modifying the headers in flight. (look to drupal and laravel for similar logic).
Note: the DT header insertion logic, while trickiest, is something that can be turned off anytime by the user by setting newrelic.distributed_tracing_exclude_newrelic_header to true.
Initial commit does the following:
Subsequent commits:
Remaining: