feat(browser): Add spanStreamingIntegration#19218
feat(browser): Add spanStreamingIntegration#19218Lms24 wants to merge 7 commits intolms/feat-span-firstfrom
spanStreamingIntegration#19218Conversation
Codecov Results 📊Generated by Codecov Action |
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/feat-browser-spanStreamingIntegration
branch
from
a294754 to
518df92
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Lms24
force-pushed
the
lms/feat-span-first
branch
from
28441df to
bad8399
Compare
Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
Lms24
force-pushed
the
lms/feat-browser-spanStreamingIntegration
branch
from
04fbd09 to
e14ab4f
Compare
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.
| path: createCDNPath('bundle.tracing.replay.logs.metrics.min.js'), | ||
| gzip: true, | ||
| limit: '81 KB', | ||
| limit: '21 KB', |
There was a problem hiding this comment.
Size limit typo: 21 KB instead of ~82 KB
High Severity
The gzip size limit for "CDN Bundle (incl. Tracing, Replay, Logs, Metrics)" was changed from 81 KB to 21 KB, which is almost certainly a typo (8 → 2). A bundle including Tracing, Replay, Logs, AND Metrics cannot be smaller than the "CDN Bundle (incl. Tracing, Replay)" at 81 KB. The uncompressed version of this same bundle is 250 KB, making 21 KB an impossible gzip ratio. This will either always fail CI or silently mask real size regressions.
| } else if (client && hasSpanStreamingEnabled(client)) { | ||
| // TODO (spans): Remove standalone span custom logic in favor of sending simple v2 web vital spans | ||
| client.emit('afterSegmentSpanEnd', this); | ||
| return; |
There was a problem hiding this comment.
Missing sampling check in streaming path sends unsampled spans
High Severity
The new streaming path has no sampling check, unlike both the standalone span path (if (this._sampled)) and the transaction path (if (this._sampled !== true)). When _startRootSpan creates an unsampled SentrySpan (with sampled: false), _onSpanEnded emits afterSpanEnd and afterSegmentSpanEnd unconditionally. The spanStreamingIntegration listener then captures and flushes the span to Sentry. Neither captureSpan nor SpanBuffer check sampling either — confirmed by searching the entire pipeline for any sampled/isRecording reference and finding none. This sends unsampled traces to Sentry, violating the sampling contract and wasting customer quota.
Additional Locations (1)
3 participants
to be merged after #19204
This PR adds the final big building block for span streaming functionality in the browser SDK:
spanStreamingIntegation.This integration:
traceLifecycle: 'stream'if not already set by users. This allows us to avoid the double-opt-in problem we usually have in browser SDKs because we want to keep integration tree-shakeable but also support the runtime-agnostictraceLifecycleoption.beforeSetup. This is allows us to safely modify client options before other integrations read it. We'll need this becausebrowserTracingIntegrationneeds to check for span streaming later on. Let me know what you think!beforeSendSpanis compatible with span streaming. If not, it falls back to static tracing (transactions).afterSpanEndhook. Once called, it will capture the span and hand it off to the span buffer.afterSegmentSpanEndhook. Once called it will flush the trace from the buffer to ensure we flush out the trace as soon as possible. In browser, it's more likely that users refresh or close the tab/window before our buffer's internal flush interval triggers. We don't have to do this but I figured it would be a good trigger point.While "final building block" sounds nice, there's still a lot of stuff to take care of in the browser. But with this in place we can also start integration-testing the browser SDKs.
ref #17836