fix(sentry): stop billing spans that declared a parent as segments - #45581
Open
MajorLift wants to merge 3 commits into
Open
fix(sentry): stop billing spans that declared a parent as segments#45581MajorLift wants to merge 3 commits into
MajorLift wants to merge 3 commits into
Conversation
An in-process parent whose span is not in `tracesByKey` resolves to `null` — the same value `resolveParentSpan` returns for "no parent intended". `startSpan` cannot distinguish the two, sees the serialized trace ids, and routes to `continueTrace` with `parentSpan: undefined`, promoting the span to a segment. A segment is a billed transaction. The existing suite covers the map-lookup hit but not the miss, and asserts `parentSpan: undefined` as correct — so the defect is currently encoded as the specification and cannot be detected by any test. Marked `.failing` because it asserts the intended behaviour, which the implementation does not yet have. Flip to `it` when the fix lands. Refs MetaMask/MetaMask-planning#7569
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Contributor
Builds ready [18931b5] [reused from 102760a]
⚡ Performance Benchmarks (Total: 🟢 0 pass · 🟡 0 warn · 🔴 1 fail)
Bundle size diffs
|
`startSpan` conflated three distinct states, two of which produced a billed transaction for a span that asked to be a child: - `undefined` — no parent declared. Ambient inheritance plus promotion is intended, and is unchanged. - `null` — a parent was intended but no span exists. `traceCallback` is typed `(span: Sentry.Span | null)`, so callers receive and pass back `null` whenever the SDK created none. This was being promoted. - declared but unresolvable — an in-process context whose `tracesByKey` lookup missed fell through to `continueTrace` with `parentSpan: undefined`, minting a segment for a lookup failure. Checks are now nullish rather than falsy, since `TraceContext` is `unknown` and `0`/`''`/`false` reached the no-parent branch. `hasInProcessIdentity` separates a local context from a genuine cross-process continuation, so only the latter takes `continueTrace`. Measured before the fix: ~44.6% of extension transactions carried a `parent_span` id while billed as roots, across 25 transaction families. Fixes MetaMask/MetaMask-planning#7569
MajorLift
changed the title
Contributor
Builds ready [b8cfbbf]
⚡ Performance Benchmarks (Total: 🟢 11 pass · 🟡 9 warn · 🔴 3 fail)
Bundle sizes
|
8 tasks
|
Contributor
Builds ready [0c9a825]
⚡ Performance Benchmarks (Total: 🟢 16 pass · 🟡 6 warn · 🔴 2 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
reviewed
Aug 18, 2026
| const { data: attributes, name, parentContext, startTime, op } = request; | ||
| let parentSpan = resolveParentSpan(parentContext); | ||
|
|
||
| // `undefined` and `null` are different statements and must not be collapsed: |
Member
There was a problem hiding this comment.
This seems to contradict the changes made in resolveParentSpan, where we return null if parentContext is null or undefined
Gudahtt
reviewed
Aug 18, 2026
| */ | ||
| function resolveParentSpan(parentContext: unknown): Sentry.Span | null { | ||
| if (!parentContext) { | ||
| // Nullish, not falsy: `TraceContext` is `unknown`, so `0` / `''` / `false` |
Member
There was a problem hiding this comment.
Nit: This explanation seems a bit excessive. Is it really helpful to list all nullish values here? Also, why use a different term here than the variable TraceContext vs parentContext.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
CHANGELOG entry: null
Summary
startSpanconflated three distinct parent states, two of which billed a transaction for a span that asked to be a child. Measured at ~44.6% of extension transactions across 25 families.TraceContextisunknown, so0/''/falsewere reaching the no-parent branch.undefinedandnullare no longer collapsed:traceCallbackis typed(span: Sentry.Span | null), so a caller receivesnullwhenever the SDK created no span and passes it straight back. That was being promoted to a transaction.continueTracewithparentSpan: undefined.The three states, before and after
parentContextundefined— no parent declaredforceTransaction→ segmentnull— parent intended, no spanforceTransaction→ segment0/''/false{_name}in-process, map misscontinueTrace→ segment{_name, _traceId, _spanId}, map misscontinueTrace→ segment{_traceId, _spanId}onlycontinueTrace→ segmentContext: the suite currently asserts the defect as correct
shared/lib/trace.test.tsalready covers this path incross-process trace context (continueTrace), and stays green — because one of its cases asserts the segment-producing value as the expected outcome:No test anywhere asserts that a span is not a segment. Every
forceTransactionreference in the suite lives in the sampler tests, which measure sample rates rather than trace shape. That test will need to be corrected as part of the fix, or the fix will present as a regression.The existing block also covers the map-lookup hit (
falls back to map lookup when _name is also present) but not the miss. The miss is the defect, and is what this test adds.Measured impact
Measured 2026-08-17, project
metamask(273505), production, 15–17 Aug (after theAggregatedBalanceSelectorinbound filter, so free of that spike):parent_spanidSegments carrying a parent id, live in Sentry · true roots, for comparison
Across 25 transaction families, including
Provider Create Accounts (v2 - batched),Wallet Alignment,UI Startup,Contact Sync Full,BackendWebSocketService ConnectionandSmart Transactions: *.Relationship to #45405 (stop force-promoting child spans)
#45405 touches the same function and should land in a compatible order, but addresses a different condition:
!parentSpan && !parentContextforceTransaction!parentSpan && hasDistributedTraceIds(parentContext)continueTracewithparentSpan: undefined#45405 does not change the
continueTracebranch, so it does not resolve #7569. Expect a textual conflict instartSpanif both land — this PR adds no production code, so resolution should be trivial.Test plan
yarn jest shared/lib/trace.test.ts— two new cases, existing cases unchangedpasses parentSpan as undefined inside continueTrace callbackstill passes: it uses a{_traceId, _spanId}-only context, which is a genuine cross-process continuation and still takes that pathis_transaction:true has:parent_span— expect a large drop, with the residual explicable as genuine cross-process trafficstartSpanand its callers, with no runtime signal behind it. CI is the first execution, and the diff warrants a careful read rather than a rubber stamp.Rollout note. This removes a large fraction of billed transactions across many families at once. That is the intent, but the drop should be expected rather than investigated as a regression — worth announcing before it lands.