-
-
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 all 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 |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| export type StreamingGuess = { | ||
| 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 { 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 { isStreaming: true }; | ||
| } | ||
|
|
||
| // Default: treat as non-streaming | ||
| return { 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