-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(tracing): Make shouldAttachHeaders
not fall back to default values
#6238
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
Conversation
I'm waiting for this fix! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly I'm the one you rubber-ducked the approach with, so I think it's a good one, but I think this looks solid overall. Good for you for realizing the bug!
size-limit report 📦
|
This PR fixes a bug in our
shouldAttachHeaders
function that determines internally if outgoing requests should have headers attached or not.Previously, we assigned the default values to
tracingOrigins
as well astracePropagationTargets
and overwrote these defaults, if users provided one (or (unlikely) both) options. InshouldAttachHeaders
, we then first testedtracingOrigins
for a match and in case there was no match, we'd testtracePropagationTargets
. This, however, also meant that if users specifiedtracingOrigins
and an URL didn't match them, there was still a chance that headers would be attached - in case the url matched thetracePropagationTargets
' default values.With this fix, we don't assign default values to either option but check for definedness of them and only if they're both undefined, well fall back to defaults.
Example:
url:
/api/test/123
tracingOrigins:
[]
tracePropTargets:
['localhost', /^\//]
// default valuesshouldAttachHeaders
returnedtrue
but should have returnedfalse
. This PR fixes this behaviour.In case, both options are defined, we prefer
tracePropagationTargets
overtracingOrigins
, as defined in the dev specification.Furthermore, this PR extracts the
shouldAttachHeaders
function by wrapping it in a factory function so that it's easier to test this behaviour. Also, the PR adds some tests to verify correct behaviour.We can probably get rid of some code around this in v8 when we finally remove
tracingOrigins
.possibly fixes the reopened issue #6077
Side-note: Because we're exporting
defaultRequestInstrumentationOptions
as public API, I decided to leave the optional value assignment in place but to simply not take the default values from it anymore ininstrumentOutgoingRequests
(see aed40a6).