fix(node): Fix baggage propagation #11363
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bundle ReportChanges will decrease total bundle size by 3.2MB ⬇️
|
Lms24
reviewed
Apr 2, 2024
| // TODO(v8): Fix this test | ||
| // eslint-disable-next-line jest/no-disabled-tests | ||
| test.skip('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => { | ||
| test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => { |
There was a problem hiding this comment.
maybe I'm missunderstanding the test but looking at the assertions below, shouldn't we also check for the presence of foo=bar,bar=baz?
There was a problem hiding this comment.
Oh I think this is fine; We should only care about not overwriting baggage entries that were already added to an outgoing request before we inject our entries. The foo and bar entries were not added further; when the outgoing request was made, right?
Comment on lines
370
to
379
| /** | ||
| * Will parse a baggage header into an array of tuples, | ||
| * where the first item is the baggage name, the second the baggage value. | ||
| */ | ||
| function parseBaggageHeaderString(baggageHeader: string): [string, string][] { | ||
| return baggageHeader.split(',').map(baggageEntry => { | ||
| const [key, value] = baggageEntry.split('='); | ||
| return [decodeURIComponent(key.trim()), decodeURIComponent(value.trim())]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
l: Is there a reason we need to parse this into an array of tuples? just wondering if it makes sense to unify things here with baggageHeaderToObject from @sentry/utils?
Feel free to disregard if N/A :)
| @@ -227,6 +225,8 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi | |||
| const { spanId, traceId } = parentSpan.spanContext(); | |||
| const sampled = getSamplingDecision(parentSpan.spanContext()); | |||
|
|
|||
| // In this case, when we are forcing a transaction, we want to treat this like continuing an incoming trace | |||
There was a problem hiding this comment.
m: just to understand this correctly: Here we're populating a DSC at span creation time - right? Do we still update the DSC's transaction property when the name changes?
There was a problem hiding this comment.
We are not allowed to update the transaction name in that case because it's a transaction boundary and would break dynamic sampling. Or do you mean, when the parent transaciton's name updates?
There was a problem hiding this comment.
in this case yes, we would not update this anymore - BUT it is very likely that by this time we already have updated the span correctly, so it should be fine in 99% of cases (as this will be "frozen" at the time when the child span will be created, by which time the parent span has hopefully already "stabilized") - does this make sense?
lforst
approved these changes
Apr 2, 2024
size-limit report 📦
|
mydea
force-pushed
the
fn/more-node-tests
branch
from
4451405 to
5237667
Compare
mydea
added a commit
that referenced
this pull request
Apr 2, 2024
This depends on #11363 being merged first...
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
The node integration tests (correctly!) showed that we were not
propagating baggage 100% correct anymore. Turns out there were a few
issues with what we did before:
## Do not freeze DSC onto span `traceState` when not continuing incoming
trace
The main issue was, that we did not actually set the `transaction`,
`sampled` and `sample_rate` baggage entries. This was the case because
we only picked these from the trace state, but we only set this fully in
our own `startSpan()` APIs, not when auto instrumentation created spans.
So I changed this to _not_ set the trace state in `startSpan()` APIs,
but instead leave it empty _unless_ we are actually continuing an
incoming trace. Whenever we are _not_ continuing an incoming trace,
we'll now generate the DSC at injection-time based of the actual span
data.
So to summarize, we now set the `dsc` trace state in two places:
1. In the propagator, when we `extract` an incoming trace.
2. In our trace functions, when we use `forceTransaction` which we treat
as continuing an incoming trace.
## No merging of existing baggage headers
Another issue that came up is that we did not properly handled other
`baggage` entires being set. This is because of how OTEL does injection,
where it passes the `carrier` as object which we treated as `unknown`
and basically ignored. This meant that if a user did e.g. `fetch({
headers: { baggage: 'my=value' }})`, we would overwrite the baggage and
not keep the exiting value(s). This PR fixes this as well by ensuring we
keep this around.
## Future improvements
After talking with @lforst, we may be able to improve/streamline this
further by getting rid of the DSC trace state.
The logic could be:
1. If there is a DSC on the scope's propagation context, use that.
2. Else, use the root span
We may update to this logic internally in a follow up step!
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
This depends on getsentry#11363 being merged first...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The node integration tests (correctly!) showed that we were not propagating baggage 100% correct anymore. Turns out there were a few issues with what we did before:
Do not freeze DSC onto span
traceStatewhen not continuing incoming traceThe main issue was, that we did not actually set the
transaction,sampledandsample_ratebaggage entries. This was the case because we only picked these from the trace state, but we only set this fully in our ownstartSpan()APIs, not when auto instrumentation created spans. So I changed this to not set the trace state instartSpan()APIs, but instead leave it empty unless we are actually continuing an incoming trace. Whenever we are not continuing an incoming trace, we'll now generate the DSC at injection-time based of the actual span data.So to summarize, we now set the
dsctrace state in two places:extractan incoming trace.forceTransactionwhich we treat as continuing an incoming trace.No merging of existing baggage headers
Another issue that came up is that we did not properly handled other
baggageentires being set. This is because of how OTEL does injection, where it passes thecarrieras object which we treated asunknownand basically ignored. This meant that if a user did e.g.fetch({ headers: { baggage: 'my=value' }}), we would overwrite the baggage and not keep the exiting value(s). This PR fixes this as well by ensuring we keep this around.Future improvements
After talking with @lforst, we may be able to improve/streamline this further by getting rid of the DSC trace state.
The logic could be:
We may update to this logic internally in a follow up step!