feat(core): Always flush streamed span traces after segment end by default - #23714
feat(core): Always flush streamed span traces after segment end by default#23714mydea wants to merge 8 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I think we can do something better here which is what we decided on last week but I didn't have time to implement yet: Let's just always flush at segment end (with the delay), regardless of environment.
In Python, we already made that change after noticing that the 5s flushing interval accumulates a lot of memory in high-throughput environments. Since we make one request per trace anyway, the number of requests shouldn't be drastically higher.
That is, as long as we can expect that traces are generally well-formed (i.e. child spans ending before their segment span). For malformed traces, with long-running child spans we would increase the request count. I think this is a fine tradeoff IMHO.
If you have (or anyone else has) concerns about this, I'd prefer keeping the two integrations separate because the merge increases bundle size for the browser version. Which was the reason why I opted against combining them initially.
sounds good to me, so I'll just change this so that the default for flushing is true I'd say, so you can still opt out? |
mydea
changed the title
ea19f07 to
023fef5
Compare
… shared integration
The browser and server span streaming integrations only differed in their
eager-flush trigger, so collapse them into a single core integration
parameterized by a `flushOnSegmentEnd` option. The browser install site
(`browserSpanApi`) passes `{ flushOnSegmentEnd: true }`; the server side
(`ServerRuntimeClient`) keeps the same call and behavior.
The integration is now exported from `shared-exports` (a single shared symbol
across all entries) instead of being duplicated in `browser-exports` and
`server-exports`. Because both entries previously resolved to *different*
symbols, the name was an ambiguous star export that `index.ts` disambiguated
explicitly; now that it resolves to the same symbol, that explicit re-export
collided with the star exports and is removed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `flushOnSegmentEnd` 500ms `setTimeout` now lives in shared core code and `flushOnSegmentEnd` is a public option, so enabling it on a Node runtime could keep a process alive until the timer fires. Wrap it in `safeUnref` (as the `SpanBuffer` already does for its own flush timer); it's a no-op in the browser, where this is the default path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
023fef5 to
5f6ffb5
Compare
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for reworking this!
Small positive side-effect for our tests: Span streaming tests should now run faster, given we flush way more quickly.
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.
Reviewed by Cursor Bugbot for commit 5f6ffb5. Configure here.
|
|
||
| export declare const linkedErrorsIntegration: typeof clientSdk.linkedErrorsIntegration; | ||
| export declare const contextLinesIntegration: typeof clientSdk.contextLinesIntegration; | ||
| export declare const spanStreamingIntegration: typeof clientSdk.spanStreamingIntegration; |
There was a problem hiding this comment.
Types drop public integration export
Medium Severity
Removing the explicit spanStreamingIntegration re-export from framework index.types.ts files drops it from the public type surface. TypeScript export * from both client and server omits colliding names, so consumers of packages such as @sentry/astro and @sentry/nextjs can no longer import it. This is flagged because the review rules treat removal of a publicly exported function without a deprecation notice as a breaking change. The same explicit re-export is still required for other shared core symbols such as withStaticSpan.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5f6ffb5. Configure here.
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
2 participants
The browser (
browserSpanStreaming.ts) and server (spanStreaming.ts) span streaming integrations were near-identical: theafterSpanEndbuffering was byte-for-byte the same, and they only differed in the eager-flush trigger. This collapses them into a single core integration, and makes flushing a trace shortly after its segment span ends the default for all runtimes, browser and server alike.Both install sites now call
spanStreamingIntegration()with no arguments:browserSpanApi._INTERNAL_ensureBrowserSpanStreamingandServerRuntimeClientshare one behavior. A trace is flushed:flushTraceSpans(the Cloudflare drain),SpanBuffer's own timeout/size thresholds.Why default segment-end flushing on for the server too
Previously the server relied solely on the buffer thresholds and only the browser flushed on segment end, to avoid the server emitting more, smaller envelopes and to keep the browser's 500ms timer from holding the Node event loop open. The event-loop concern is already handled by
safeUnrefon the timer (a no-op in the browser), so there's no longer a reason to gate the segment-end path by runtime — flushing traces timely once their segment ends is the behavior we want everywhere.The
flushOnSegmentEndoption is retained (now defaulting totrue) so a runtime can still opt out and fall back to threshold-only flushing.Why keep it in core rather than moving to browser-utils/server-utils
The integrations are consumed internally by core —
ServerRuntimeClientauto-adds it, andbrowserSpanApi/idleSpanauto-install it (a deliberate tree-shaking indirection). Sincebrowser-utilsandserver-utilsboth depend on@sentry/core, core cannot import back from them without a circular dependency. Collapsing in-core sidesteps that entirely.Export change
The integration now lives in
shared-exportsas a single shared symbol (star-exported by all entries) instead of being duplicated inbrowser-exportsandserver-exports. Previously the two entries resolved to different symbols, making the name an ambiguous star export thatindex.tsdisambiguated with an explicit re-export. Now that both resolve to the same symbol, that explicit line collided with the star exports (leaving the root namespaceundefined) and is removed. ThestartSpan/startInactiveSpan/startSpanManualdisambiguation stays, as those remain genuinely distinct browser/server variants.