-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(cloudflare): Keep http root span alive until streaming responses are consumed #18087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e7d2476 to
b9d02cf
Compare
size-limit report 📦
|
2875b4f to
eb7a5c1
Compare
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.
|
febee8b to
fadd2eb
Compare
d729aae to
31ecf58
Compare
packages/cloudflare/src/request.ts
Outdated
| const result = await reader.read(); | ||
| done = result.done; | ||
| } | ||
| } catch { | ||
| // Stream error or cancellation - will end span in finally | ||
| } finally { | ||
| reader.releaseLock(); | ||
| span.end(); | ||
| waitUntil?.(flush(2000)); | ||
| } | ||
| })(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, let's try to be extra-careful here :)
packages/cloudflare/src/request.ts
Outdated
| } | ||
| })(); | ||
|
|
||
| waitUntil?.(streamMonitor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: why do we need this call? (just asking, no objections)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment here! We just have to wait for stream consumption and end span when complete
packages/cloudflare/src/request.ts
Outdated
| const result = await reader.read(); | ||
| done = result.done; | ||
| } | ||
| } catch { | ||
| // Stream error or cancellation - will end span in finally | ||
| } finally { | ||
| reader.releaseLock(); | ||
| span.end(); | ||
| waitUntil?.(flush(2000)); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, let's try to be extra-careful here :)
| } | ||
|
|
||
| // Classify response to detect actual streaming | ||
| const classification = classifyResponseStreaming(res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: what happens if we mis-classify a non-streaming response as a stream response? Would we break anything with creating the stream readers?
I guess the other way around isn't "dangerous", given we'd just end the span too early, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so for false positives (non-streaming → classified as streaming) functionality wise it won't break anything, the only downside is slightly more overhead (creating an unnecessary stream monitor), and for false negatives yes you're correct, it would end span too early, but that's our best guess for now
| * We avoid probing the stream to prevent blocking on transform streams (like injectTraceMetaTags) | ||
| * or SSR streams that may not have data ready immediately. | ||
| */ | ||
| export function classifyResponseStreaming(res: Response): StreamingGuess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l/nit: Is there a reason why we return the response as well? I don't see any changes being made to it, so could a caller just directly reuse res instead of using the returned response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch! I was mutating the res in a previous commit, these are just leftovers 😅
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes! I think with this, we can at least ensure that the http.server stays alive. I'm a bit worried about the span not being active long enough though (because startSpanManual keeps the span only active as long as the callback is executed). But I think we can give this a try because at least the span length should now be correct.
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | [`10.27.0` -> `10.28.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/10.27.0/10.28.0) |  |  | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@​sentry/react)</summary> ### [`v10.28.0`](https://github.com/getsentry/sentry-javascript/releases/tag/10.28.0) [Compare Source](getsentry/sentry-javascript@10.27.0...10.28.0) ##### Important Changes - **feat(core): Make `matcher` parameter optional in `makeMultiplexedTransport` ([#​10798](getsentry/sentry-javascript#10798 The `matcher` parameter in `makeMultiplexedTransport` is now optional with a sensible default. This makes it much easier to use the multiplexed transport for sending events to multiple DSNs based on runtime configuration. **Before:** ```javascript import { makeFetchTransport, makeMultiplexedTransport } from '@​sentry/browser'; const EXTRA_KEY = 'ROUTE_TO'; const transport = makeMultiplexedTransport(makeFetchTransport, args => { const event = args.getEvent(); if (event?.extra?.[EXTRA_KEY] && Array.isArray(event.extra[EXTRA_KEY])) { return event.extra[EXTRA_KEY]; } return []; }); Sentry.init({ transport, // ... other options }); // Capture events with routing info Sentry.captureException(error, { extra: { [EXTRA_KEY]: [ { dsn: 'https://key1@​sentry.io/project1', release: 'v1.0.0' }, { dsn: 'https://key2@​sentry.io/project2' }, ], }, }); ``` **After:** ```javascript import { makeFetchTransport, makeMultiplexedTransport, MULTIPLEXED_TRANSPORT_EXTRA_KEY } from '@​sentry/browser'; // Just pass the transport generator - the default matcher handles the rest! Sentry.init({ transport: makeMultiplexedTransport(makeFetchTransport), // ... other options }); // Capture events with routing info using the exported constant Sentry.captureException(error, { extra: { [MULTIPLEXED_TRANSPORT_EXTRA_KEY]: [ { dsn: 'https://key1@​sentry.io/project1', release: 'v1.0.0' }, { dsn: 'https://key2@​sentry.io/project2' }, ], }, }); ``` The default matcher looks for routing information in `event.extra[MULTIPLEXED_TRANSPORT_EXTRA_KEY]`. You can still provide a custom matcher function for advanced use cases. - **feat(nextjs): Support cacheComponents on turbopack ([#​18304](getsentry/sentry-javascript#18304 This release adds support for `cacheComponents` on turbopack builds. We are working on adding support for this feature in webpack builds as well. ##### Other Changes - feat: Publish AWS Lambda Layer for Node 24 ([#​18327](getsentry/sentry-javascript#18327)) - feat(browser): Expose langchain instrumentation ([#​18342](getsentry/sentry-javascript#18342)) - feat(browser): Expose langgraph instrumentation ([#​18345](getsentry/sentry-javascript#18345)) - feat(cloudflare): Allow specifying a custom fetch in Cloudflare transport options ([#​18335](getsentry/sentry-javascript#18335)) - feat(core): Add `isolateTrace` option to `Sentry.withMonitor()` ([#​18079](getsentry/sentry-javascript#18079)) - feat(deps): bump [@​sentry/webpack-plugin](https://github.com/sentry/webpack-plugin) from 4.3.0 to 4.6.1 ([#​18272](getsentry/sentry-javascript#18272)) - feat(nextjs): Add cloudflare `waitUntil` detection ([#​18336](getsentry/sentry-javascript#18336)) - feat(node): Add LangChain v1 support ([#​18306](getsentry/sentry-javascript#18306)) - feat(remix): Add parameterized transaction naming for routes ([#​17951](getsentry/sentry-javascript#17951)) - fix(cloudflare): Keep http root span alive until streaming responses are consumed ([#​18087](getsentry/sentry-javascript#18087)) - fix(cloudflare): Wait for async events to finish ([#​18334](getsentry/sentry-javascript#18334)) - fix(core): `continueTrace` doesn't propagate given trace ID if active span exists ([#​18328](getsentry/sentry-javascript#18328)) - fix(node-core): Handle custom scope in log messages without parameters ([#​18322](getsentry/sentry-javascript#18322)) - fix(opentelemetry): Ensure Sentry spans don't leak when tracing is disabled ([#​18337](getsentry/sentry-javascript#18337)) - fix(react-router): Use underscores in trace origin values ([#​18351](getsentry/sentry-javascript#18351)) - chore(tanstackstart-react): Export custom inits from tanstackstart-react ([#​18369](getsentry/sentry-javascript#18369)) - chore(tanstackstart-react)!: Remove empty placeholder implementations ([#​18338](getsentry/sentry-javascript#18338)) <details> <summary><strong>Internal Changes</strong></summary> - chore: Allow URLs as issue ([#​18372](getsentry/sentry-javascript#18372)) - chore(changelog): Add entry for [#​18304](getsentry/sentry-javascript#18304) ([#​18329](getsentry/sentry-javascript#18329)) - chore(ci): Add action to track all PRs as issues ([#​18363](getsentry/sentry-javascript#18363)) - chore(github): Adjust `BUGBOT.md` rules to flag invalid op and origin values during review ([#​18352](getsentry/sentry-javascript#18352)) - ci: Add action to create issue on gitflow merge conflicts ([#​18319](getsentry/sentry-javascript#18319)) - ci(deps): bump actions/checkout from 5 to 6 ([#​18268](getsentry/sentry-javascript#18268)) - ci(deps): bump peter-evans/create-pull-request from 7.0.8 to 7.0.9 ([#​18361](getsentry/sentry-javascript#18361)) - test(cloudflare): Add typechecks for cloudflare-worker e2e test ([#​18321](getsentry/sentry-javascript#18321)) </details> #### Bundle size 📦 | Path | Size | | ----------------------------------------------------------------------------------------------------- | --------- | | [@​sentry/browser](https://github.com/sentry/browser) | 24.22 KB | | [@​sentry/browser](https://github.com/sentry/browser) - with treeshaking flags | 22.76 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Tracing) | 40.57 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Profiling) | 45.05 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) | 78.08 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) - with treeshaking flags | 68.05 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay with Canvas) | 82.65 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback) | 94.61 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. Feedback) | 40.51 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. sendFeedback) | 28.8 KB | | [@​sentry/browser](https://github.com/sentry/browser) (incl. FeedbackAsync) | 33.66 KB | | [@​sentry/react](https://github.com/sentry/react) | 25.9 KB | | [@​sentry/react](https://github.com/sentry/react) (incl. Tracing) | 42.72 KB | | [@​sentry/vue](https://github.com/sentry/vue) | 28.56 KB | | [@​sentry/vue](https://github.com/sentry/vue) (incl. Tracing) | 42.32 KB | | [@​sentry/svelte](https://github.com/sentry/svelte) | 24.24 KB | | CDN Bundle | 26.57 KB | | CDN Bundle (incl. Tracing) | 41.22 KB | | CDN Bundle (incl. Tracing, Replay) | 76.9 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) | 82.23 KB | | CDN Bundle - uncompressed | 78.09 KB | | CDN Bundle (incl. Tracing) - uncompressed | 122.4 KB | | CDN Bundle (incl. Tracing, Replay) - uncompressed | 235.71 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 248.17 KB | | [@​sentry/nextjs](https://github.com/sentry/nextjs) (client) | 44.88 KB | | [@​sentry/sveltekit](https://github.com/sentry/sveltekit) (client) | 40.92 KB | | [@​sentry/node-core](https://github.com/sentry/node-core) | 50.06 KB | | [@​sentry/node](https://github.com/sentry/node) | 155.7 KB | | [@​sentry/node](https://github.com/sentry/node) - without tracing | 90.67 KB | | [@​sentry/aws-serverless](https://github.com/sentry/aws-serverless) | 105.61 KB | </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi4yNy4xIiwidXBkYXRlZEluVmVyIjoiNDIuMjcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Reviewed-on: https://git.foxden.network/foxCaves/foxCaves/pulls/12 Co-authored-by: Renovate <[email protected]> Co-committed-by: Renovate <[email protected]>
Fixes: https://linear.app/getsentry/issue/JS-1103/spans-are-not-flushed-to-dashboard-when-using-streamtext-with-vercel
The Cloudflare request wrapper was ending the root HTTP span immediately when the handler returned a streaming Response (e.g.
result.toTextStreamResponse()). Since Vercel AI child spans only finish after the stream is consumed by the client, they were filtered out by Sentry'sisFullFinishedSpancheck, resulting in transactions with 0 spans.This PR implements a streaming response detection and handles this from within the http handler:
Created
classifyResponseStreaming()helperUpdated request wrapper
startSpan()tostartSpanManual()for manual span control