Skip to content

Conversation

zsistla
Copy link
Contributor

@zsistla zsistla commented Feb 13, 2025

No description provided.

lavarou and others added 9 commits January 14, 2025 11:38
Address issues spotted by `go vet`:
- fix syntax of `kubernetes.KubernetesServiceHost` struct tags
- ensure `context.cancel()` is always called to avoid context leak

Add go vet check for pull requests.
1) Added message segment.
Agent will now handle generic, external, datastore, and message
segments.

2) Added unit tests for the changes

3) New functionality involves adding message specific attributes to
segments/spans, generating message specific metrics, and honoring the
message_tracer_parameters_enabled INI setting to disable/enable the
attributes.

4)Things to keep in mind while reviewing:
* the commits describe a lot of what is going on
* Functionality-wise, message segment is a mix of external and datastore
* nr_segment_message is the new file

---------

Co-authored-by: ZNeumann <[email protected]>
Co-authored-by: Michael Fulbright <[email protected]>
Uses message segments to create SQS instrumentation.

Added unit tests and multiverse tests.

---------

Co-authored-by: ZNeumann <[email protected]>
Co-authored-by: Michael Fulbright <[email protected]>
Co-authored-by: Michal Nowacki <[email protected]>
Instead of a blanket segment_start in the "before" function that needs
to be discarded in the majority of the cases, just create the segment
when it is needed, add the attributes/metrics, then close the segment.

DONE:Multiverse SQS exception tests need to be updated.
DONE:Needs additional testing against backend.

---------

Co-authored-by: Michal Nowacki <[email protected]>
Free strings that were strdup-ed but not being freed.
…hashmap is destroyed (#1017)

This function created a new hashmap, but didn's pass the string dtor in
so any strdupped values were not being freed when the hashmap was
destroyed.

valgrind output from a multiverse run showed:

==220==    by 0x6AA4680: nr_strdup (util_memory.c:156)
==220== by 0x6A83BD3: nr_header_create_distributed_trace_map
(nr_header.c:60)
==220== by 0x6A45D28: nr_php_amqplib_retrieve_dt_headers
(lib_php_amqplib.c:503)
==220==    by 0x6A45D28: nr_rabbitmq_basic_get (lib_php_amqplib.c:742)
==220== by 0x6A74993: nr_zend_call_orig_execute_special
(php_user_instrument.c:105)
==220==    by 0x6A52EAA: nr_php_instrument_func_end (php_execute.c:2086)
==220==    by 0x6A54D5B: nr_php_observer_fcall_end (php_execute.c:2188)
==220==    by 0x71D7ED: zend_observer_fcall_end (in /usr/local/bin/php)
==220==    by 0x6E79FF: execute_ex (in /usr/local/bin/php)
==220==    by 0x6F0B42: zend_execute (in /usr/local/bin/php)
==220==    by 0x67C06F: zend_execute_scripts (in /usr/local/bin/php)
==220==    by 0x60FA3D: php_execute_script (in /usr/local/bin/php)

after applying the fix, valgrind had no issues.
…ry (#1009)

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:
* Detect library via magic file
* Detect package and version information.
* Basic unit tests

Subsequent commits:
* Add attributes needed for rabbitMQ to message segment
* Instrument basic_publish and basic_get
* add unit tests, multiverse tests
* added support for PHP 7.x
* DT header insertion/retrieval
* Added more multiverse tests, especially around the DT functionality
@newrelic-php-agent-bot
Copy link

newrelic-php-agent-bot commented Feb 13, 2025

Test Suite Status Result
Multiverse 8/8 passing
SOAK 79/79 passing

@zsistla zsistla merged commit 9409457 into main Feb 13, 2025
129 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants