-
-
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
Changes from 9 commits
2b3eaf1
0842114
7a570db
eb7a5c1
ae8e345
62a1b2c
ca4bafa
31ecf58
17a03a7
1f75f5c
0686eaa
a5e65f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,13 +8,14 @@ import { | |
| parseStringToURLObject, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
| setHttpStatus, | ||
| startSpan, | ||
| startSpanManual, | ||
| winterCGHeadersToDict, | ||
| withIsolationScope, | ||
| } from '@sentry/core'; | ||
| import type { CloudflareOptions } from './client'; | ||
| import { addCloudResourceContext, addCultureContext, addRequest } from './scope-utils'; | ||
| import { init } from './sdk'; | ||
| import { classifyResponseStreaming } from './utils/streaming'; | ||
|
|
||
| interface RequestHandlerWrapperOptions { | ||
| options: CloudflareOptions; | ||
|
|
@@ -98,26 +99,64 @@ export function wrapRequestHandler( | |
| // Note: This span will not have a duration unless I/O happens in the handler. This is | ||
| // because of how the cloudflare workers runtime works. | ||
| // See: https://developers.cloudflare.com/workers/runtime-apis/performance/ | ||
| return startSpan( | ||
| { | ||
| name, | ||
| attributes, | ||
| }, | ||
| async span => { | ||
| try { | ||
| const res = await handler(); | ||
| setHttpStatus(span, res.status); | ||
| return res; | ||
| } catch (e) { | ||
| if (captureErrors) { | ||
| captureException(e, { mechanism: { handled: false, type: 'auto.http.cloudflare' } }); | ||
| } | ||
| throw e; | ||
| } finally { | ||
| waitUntil?.(flush(2000)); | ||
|
|
||
| // Use startSpanManual to control when span ends (needed for streaming responses) | ||
| return startSpanManual({ name, attributes }, async span => { | ||
| let res: Response; | ||
|
|
||
| try { | ||
| res = await handler(); | ||
| setHttpStatus(span, res.status); | ||
| } catch (e) { | ||
| span.end(); | ||
| if (captureErrors) { | ||
| captureException(e, { mechanism: { handled: false, type: 'auto.http.cloudflare' } }); | ||
| } | ||
| }, | ||
| ); | ||
| waitUntil?.(flush(2000)); | ||
| throw e; | ||
| } | ||
|
|
||
| // Classify response to detect actual streaming | ||
| const classification = classifyResponseStreaming(res); | ||
|
|
||
| if (classification.isStreaming && classification.response.body) { | ||
| // Streaming response detected - monitor consumption to keep span alive | ||
| const [clientStream, monitorStream] = classification.response.body.tee(); | ||
|
|
||
| // Monitor stream consumption and end span when complete | ||
| const streamMonitor = (async () => { | ||
| const reader = monitorStream.getReader(); | ||
|
|
||
| try { | ||
| let done = false; | ||
| while (!done) { | ||
| 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)); | ||
| } | ||
| })(); | ||
|
||
|
|
||
| waitUntil?.(streamMonitor); | ||
|
||
|
|
||
| // Return response with client stream | ||
| return new Response(clientStream, { | ||
| status: classification.response.status, | ||
| statusText: classification.response.statusText, | ||
| headers: classification.response.headers, | ||
| }); | ||
| } | ||
|
|
||
| // Non-streaming response - end span immediately and return original | ||
| span.end(); | ||
| waitUntil?.(flush(2000)); | ||
| return classification.response; | ||
| }); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| export type StreamingGuess = { | ||
| response: Response; | ||
| isStreaming: boolean; | ||
| }; | ||
|
|
||
| /** | ||
| * Classifies a Response as streaming or non-streaming. | ||
| * | ||
| * Heuristics: | ||
| * - No body → not streaming | ||
| * - Known streaming Content-Types → streaming (SSE, NDJSON, JSON streaming) | ||
| * - text/plain without Content-Length → streaming (some AI APIs) | ||
| * - Otherwise → not streaming (conservative default, including HTML/SSR) | ||
| * | ||
| * 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅 |
||
| if (!res.body) { | ||
| return { response: res, isStreaming: false }; | ||
| } | ||
|
|
||
| const contentType = res.headers.get('content-type') ?? ''; | ||
| const contentLength = res.headers.get('content-length'); | ||
|
|
||
| // Streaming: Known streaming content types | ||
| // - text/event-stream: Server-Sent Events (Vercel AI SDK, real-time APIs) | ||
| // - application/x-ndjson, application/ndjson: Newline-delimited JSON | ||
| // - application/stream+json: JSON streaming | ||
| // - text/plain (without Content-Length): Some AI APIs use this for streaming text | ||
| if ( | ||
| /^text\/event-stream\b/i.test(contentType) || | ||
| /^application\/(x-)?ndjson\b/i.test(contentType) || | ||
| /^application\/stream\+json\b/i.test(contentType) || | ||
| (/^text\/plain\b/i.test(contentType) && !contentLength) | ||
| ) { | ||
| return { response: res, isStreaming: true }; | ||
| } | ||
|
|
||
| // Default: treat as non-streaming | ||
| return { response: res, isStreaming: false }; | ||
| } | ||
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