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.

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

Prevent build up of duplicate Distributed Tracing headers #120

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

joshuabenuck
Copy link
Contributor

@joshuabenuck joshuabenuck commented Mar 10, 2021

This PR prevents a build up of Distributed Tracing headers from being added to a request when using file_get_contents and reusing the stream context parameter.

It does this by:

  • Modifying the test trace endpoint to detect if there is more than one value for a distributed tracing header
  • Adding a scenario that reuses the stream context when calling file_get_contents, this triggers the modified detection logic in the trace endpoint
  • Updating nr_php_file_get_contents_add_headers_internal to detect if one of the DT headers are present in both the existing headers string and the headers that are about to be added. This change assumes that all of the DT headers are added as a group and that detecting one of them is sufficient.

The flow goes roughly like this:

  • nr_php_file_get_contents_create_outbound_headers calls nr_header_create_outbound_request_create. The resulting headers are converted to a string.
  • The headers string is passed by nr_php_file_get_contents_add_headers to nr_php_file_get_contents_add_headers_internal which concats the new headers to the existing headers.

In this flow, the existing headers are already a string. This changes attempts to reparse the existing headers to reduce overhead. The simplest approach seems to be just looking for the presence of a key phrase when determining if the DT headers are already present and are about to be added.

W3C_TRACESTATE was somewhat arbitrarily chosen. NEWRELIC can be optionally excluded via configuration so it wasn't an option. W3C_TRACEPARENT was the other option.

A colon is included in the searched for string to avoid matching any content in the value portion of any of the headers.

Copy link
Contributor

@zsistla zsistla left a comment

Choose a reason for hiding this comment

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

Great find, fix, and tests!

@joshuabenuck joshuabenuck merged commit aee50bf into newrelic:dev Mar 11, 2021
@joshuabenuck joshuabenuck deleted the dt-duplicate-headers branch March 11, 2021 15:02
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.

None yet

3 participants