feat(browser): Run static beforeSendSpan for INP spans - #22877
Conversation
INP is always emitted as a v2 span. In static trace lifecycle it goes through the standalone send path, which only honored a streamed beforeSendSpan, so a plain (v1 SpanJSON) callback was silently skipped for INP. Users opting out of span streaming could not scrub the INP element name or route. Split the standalone send path by callback type: a plain beforeSendSpan now scrubs the span in its native v1 SpanJSON shape, then converts it forward to v2 (the same one-way conversion gen_ai spans use to reach the v2 span path). No reverse v2 to v1 conversion is introduced, and captureSpan stays stream-only.
size-limit report 📦
|
7dd9dd3 to
3a52992
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…eforeSendSpan The static-callback standalone path bypassed `captureSpan`, so it never emitted the `preprocessSpan`/`processSpan` hooks. Integrations that enrich spans through those hooks stopped running for INP when a plain `beforeSendSpan` was set, most notably Replay no longer attaching `sentry.replay_id`. Convert the post-callback v1 SpanJSON to the intermediate StreamedSpanJSON and run the hooks on it before serializing, matching the streaming pipeline. Still no reverse v2 -> v1 conversion: the user callback runs on v1 first, hooks run on the forward-converted v2 JSON.
… standalone spans Keep the hook/callback order consistent with `captureSpan`: run `preprocessSpan`/`processSpan` first, then the user's v1 `beforeSendSpan`, so the callback sees integration enrichment (e.g. Replay's `sentry.replay_id`) and can act on it. The hooks mutate the v2 `StreamedSpanJSON`; reflect their attribute and name changes back onto the v1 `SpanJSON` before invoking the callback. The v1 status is a free-form message while v2 is only `'ok' | 'error'`, so snapshot the original v1 status and restore it rather than round-trip through the lossy v2 form.
…version v1 SpanJSON mirrors op/origin/profile_id/exclusive_time as top-level fields alongside their attributes. A static `beforeSendSpan` edits the top-level field, but the forward converter only copied `data`, so those edits were dropped from the sent v2 span. Fold the top-level fields back into attributes (top-level wins over the initially identical attribute), the inverse of `getSpanJSON`.
| const { isolationScope: spanIsolationScope, scope: spanScope } = getCapturedScopesOnSpan(span); | ||
| const finalScopeData = getCombinedScopeData(spanIsolationScope, spanScope); | ||
|
|
||
| const commonAttributes = commonSpanAttributes(serializedSegmentSpan, client, finalScopeData); |
There was a problem hiding this comment.
m: to make sure we stay compatible with the static beforeSendSpan callback, we shouldn't apply the scopeData.attributes to it. The reason is, scope data attributes can have a {unit, value} object value which might be unexpected for users who deal primarily with transactions. Scope attributes are not applied to transactions, only to streamed spans. I think it's okay not to apply scope attributes to INP spans, even if they're technically v2 spans.
| const streamedSpanJSON = spanJsonToStreamedSpanJSON(spanJSON); | ||
| client.emit('preprocessSpan', streamedSpanJSON); | ||
| client.emit('processSpan', streamedSpanJSON); | ||
|
|
||
| // Reflect the hook enrichment back onto the v1 JSON so `beforeSendSpan` (which runs on v1, after the | ||
| // hooks, as in `captureSpan`) sees it. Attributes and name map cleanly. The v1 status is a free-form | ||
| // message, but v2 only has `'ok' | 'error'`, so restore the original v1 status rather than lose detail. | ||
| const originalStatus = spanJSON.status; | ||
| spanJSON.data = streamedSpanJSON.attributes as SpanJSON['data']; | ||
| spanJSON.description = streamedSpanJSON.name; | ||
| spanJSON.status = originalStatus; |
There was a problem hiding this comment.
m: I think we can just omit calling the hooks and partially applying the modified data. Or did we miss data on the span that gets added via hook consumers? My concern is primarily that
- this requires another conversion
- hook listeners might add data to this span like its a regular streamed span (e.g. scope attributes).
Unless we need data form any of the hook listeners, I'd prefer skipping calling and backmapping.
There was a problem hiding this comment.
Doesn't replay_id rely on processSpan hook for the INP? or is it something we can ignore here?
There was a problem hiding this comment.
Ah yeah right... I guess we need the attributes that are on the current v2 span or on the v1 span we had on v10 (https://github.com/getsentry/sentry-javascript/blob/v10/packages/browser-utils/src/metrics/utils.ts#L80). Not sure if we actually need replay_id specifically but we used to send it so we should continue doing so. It's fine then to leave this as-is I'd say.
There was a problem hiding this comment.
l: if we can avoid calling the hooks, I don't think we need to modify this file at all, right? a spanJsonToSerializedStreamedSpan call at the end should be enough
…emission
Follow the gen_ai extraction pattern: a standalone span with a static beforeSendSpan
is plainly converted from v1 SpanJSON to a v2 span, without running the
preprocessSpan/processSpan hooks or back-mapping. This drops the extra conversion
and avoids hook consumers adding streamed-span-only data (e.g. scope attributes) to
what a static callback treats as a transaction-shaped span.
Because the hooks no longer run, Replay can't attach the replay id itself, so set it
(and the buffering flag) on the INP span at emission in browser-utils, mirroring how
v1 standalone web vital spans did it on v10. Scope attributes are also excluded from
the static path, as they can hold {unit, value} objects unexpected for such callbacks.
Revert spanJsonToStreamedSpan to its original form. The static standalone path now plainly converts like gen_ai, so it doesn't need op/origin/profile_id/exclusive_time folded back from top-level SpanJSON fields. A static beforeSendSpan that mutates a top-level field (rather than the data attribute) won't take effect, matching how the converter already behaves for gen_ai spans, and keeps this file untouched by the PR.
…ic lifecycle Add TODO(standalone) markers to the two remaining pieces that only exist to serve the static trace lifecycle: the `includeScopeAttributes` flag on `commonSpanAttributes` and the `getReplayAttributes` helper. Both go away once transactions are dropped (v12), when INP always streams and Replay's `processSpan` attaches the replay id itself.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fc7c722. Configure here.
|
|
||
| return { | ||
| 'sentry.replay_id': replayId, | ||
| 'sentry._internal.replay_is_buffering': replay!.getRecordingMode() === 'buffer' ? true : undefined, |
There was a problem hiding this comment.
Non-null assertion lacks safety comment
Low Severity
getReplayAttributes uses a non-null assertion on replay with no comment explaining why a safer narrow isn't possible. The same replay lookup in logs/metrics already avoids this with optional chaining after the replayId check.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit fc7c722. Configure here.
|
|
||
| const processedSpan = beforeSendSpan(spanJSON) || (showSpanDropWarning(), spanJSON); | ||
|
|
||
| return spanJsonToSerializedStreamedSpan(processedSpan); |
There was a problem hiding this comment.
Callback op changes not preserved
Medium Severity
captureStandaloneSpanWithStaticCallback converts the post-callback v1 SpanJSON with spanJsonToSerializedStreamedSpan, which only copies data into v2 attributes. Top-level op and origin edits from a static beforeSendSpan therefore never reach the sent INP span, even though description and data changes do.
Reviewed by Cursor Bugbot for commit fc7c722. Configure here.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I think this looks better. thanks for reworking!
2 participants
This fixes a
beforeSendSpangap for INP spans in the static trace lifecycle, without reintroducing v1 spans.INP is always sent as a v2 span. When span streaming is disabled it goes through the standalone send path, which runs
captureSpan. That only honors abeforeSendSpanwrapped withwithStreamedSpan, so a plain callback (the one static users write, typed for v1SpanJSON) was silently skipped for INP.This also lets it call the
preprocessSpanandprocessSpanhooks.