-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(test-utils): Add a way to wait for single spans for Span streaming #18986
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
base: lms/feat-span-streaming-poc
Are you sure you want to change the base?
Conversation
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.
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.
| for (const envelopeItem of envelopeItems) { | ||
| if (!isSpanV2EnvelopeItem(envelopeItem)) { | ||
| return 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.
Early return skips valid Span V2 items in envelope
Medium Severity
The waitForSpanV2 function returns false immediately when encountering any non-Span V2 envelope item, rather than skipping it and continuing to check other items. If an envelope contains mixed item types (e.g., an event item followed by a Span V2 item), the function will never find valid Span V2 spans because it exits on the first non-matching item. The logic differs from waitForSpansV2, which correctly uses an if (isSpanV2EnvelopeItem) pattern to skip non-Span V2 items.
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 don't think it can be mixed
size-limit report 📦
|
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 (const envelopeItem of envelopeItems) { | ||
| if (!isSpanV2EnvelopeItem(envelopeItem)) { | ||
| return 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.
should we continue here?
| return false | |
| continue; |
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.
That would be this comment: #18986 (comment)
Not sure if it is possible to mix envelope items
How it should be used is in the JSDoc. It worked quite well for my Cloudflare tests
Closes #18987 (added automatically)