fix(node): Deduplicate sentry-trace and baggage headers on outgoing requests#19960
fix(node): Deduplicate sentry-trace and baggage headers on outgoing requests#19960
sentry-trace and baggage headers on outgoing requests#19960Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Other
Bug Fixes 🐛Node
Other
Documentation 📚
Internal Changes 🔧Core
Deps
Deps Dev
Other
🤖 This preview updates automatically when you update the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Lms24
force-pushed
the
lms/fix-double-baggage-propagation
branch
from
c4c5fbb to
a11ca57
Compare
Lms24
force-pushed
the
lms/fix-double-baggage-propagation
branch
from
35051e1 to
8d53f05
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Lms24
force-pushed
the
lms/fix-double-baggage-propagation
branch
from
fa73db1 to
27a400b
Compare
There was a problem hiding this comment.
the browser SDK already guarded against multiple sentry headers, I just strengthened the test assertions a bit.
| 'sentry-environment=myEnv', | ||
| 'sentry-release=2.1.0', | ||
| expect.stringMatching(/sentry-sample_rand=\d+/), | ||
| 'sentry-sample_rate=0.54', |
There was a problem hiding this comment.
this was a wrong assertion in the test
Lms24
requested review from
a team,
chargome and
logaretm
and removed request for
a team
Lms24
changed the title
sentry-trace and baggage headers on fetch requestssentry-trace and baggage headers on fetch requests
Lms24
changed the title
sentry-trace and baggage headers on fetch requestssentry-trace and baggage headers on outgoing requests
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Looks solid! The integration tests should harden for regressions here too.
| if (key) { | ||
| headers.push(key, value); | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
l: We drop malformed headers here, should we log it?
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| headers.push(key, value); | ||
| } | ||
| } catch { | ||
| debug.warn(`Failed to convert string request header to array header: ${header}`); |
There was a problem hiding this comment.
Missing DEBUG_BUILD guard on debug.warn call
Low Severity
The debug.warn() call is not guarded by DEBUG_BUILD &&, and DEBUG_BUILD is not imported in this file. Every other debug.log/warn/error call across packages/node-core/src/utils/ is consistently guarded with DEBUG_BUILD && (see outgoingHttpRequest.ts, errorhandling.ts). While debug.warn internally short-circuits via _maybeLog's own DEBUG_BUILD check, the external guard is what lets the calling package's bundler tree-shake the entire expression — including the template literal string allocation. Without it, the function call and string creation remain in the production bundle.
2 participants
This PR fixes a bunch of closely related issues with our node fetch and http integrations for outgoing request header propagation.
Summary:
sentry-baggage entries when merging two headers where both containsentry-entries. We only take one of the two and delete the other.Sentry Propagator
(The propagator is used by both, fetch and node:http outgoing request instrumentation)
baggage(and optionallytraceparent) headers to requests, even if asentry-traceheader was already set on the requestsentry-traceheaders. Only apply our headers, ifsentry-tracedoesn't exist yet.fetch instrumentation
mergeBaggageHeaderswould mix upsentry-entries from both headers creating completely inconsistent baggage entries.sentry-entries and add all new ones (merge non-sentry entries)addTracePropagationHeadersToFetchRequestwould correctly avoid setting asentry-traceheader if a previous one existed but happily still add newbaggageand optionallytraceparentheaders.sentry-traceand the other two values (e.g. distinct trace ids across the headers in one request).sentry-traceheaders. Only apply our headers, ifsentry-tracedoesn't exist yet.sentry-entries would cause multiplebaggageheaders to be present. This was due to insufficient deduplication in our own header adding logic. This surfaced because we didn't only have two competing headers in this case but three (user, OTel Undici, our instrumentation).sentry-traceandbaggageheader if >1 are already set. Only add new headers, if no headers were set yet.baggageorsentry-trace(e.g.my-vendor-baggage) headers. Generally, the string replacement logic was brittle.Http instrumentation
node:httprequests, we'd incorrectly setbaggage(andtraceparent) headers even if asentry-traceheader was already set.sentry-traceheaders. Only apply our headers, ifsentry-tracedoesn't exist yet.Minor fixes
Added tests for the various propagation mechanisms and integration tests asserting on traceid and spanid consistency across spans and all headers (as appropriate for various scenarios).
closes #19158