-
Notifications
You must be signed in to change notification settings - Fork 496
Fix span reporting when actor aborts with pending waitUntil tasks #5709
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: main
Are you sure you want to change the base?
Conversation
|
Tests build on tests added in another change, so ignore the base until that's merged. |
|
cc @kentonv I see you added this originally, so I plan to request your review after I get some buy in from Felix. |
ba5bbd0 to
fbe364b
Compare
The description is misleading here – span reporting doesn't "partially fail", we merely fall back to the return event timestamp as the IncomingRequest was removed prematurely. The span metadata will still be reported in full, only the timestamp may be off. |
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 lines up with what I had in mind for this issue as described in #5282 (comment) – it should solve the issue at hand but I can't rule out that reordering actions in ~IoContext_IncomingRequest would cause other issues, maybe there was a reason for having the current order, which looks less intuitive than the one being proposed here.
1e160c1 to
d3ade51
Compare
The span attached to a subrequest is held by the fetch promise, which can be passed to waitUntilTasks. When an actor aborts while another request's waitUntil task is still pending, span reporting can partially fail. Span reporting relies on timing from the current request. If there are reasons not to reorder ~IncomingRequest to cancel pending operations before removing from the list, we can reconsider the assumption that an active request is available during that time.
Co-authored-by: Dan Lapid <dlapid@cloudflare.com>
Updated desc. |
fbe364b to
5a7b8eb
Compare
CodSpeed Performance ReportMerging #5709 will degrade performances by 8.86%Comparing Summary
Benchmarks breakdown
Footnotes
|
The span attached to a subrequest is held by the fetch promise, which can be passed to waitUntilTasks. When an actor aborts while another request's waitUntil task is still pending, span duration may be incorrect. Span reporting relies on timing from the current request.
If there are reasons not to reorder ~IncomingRequest to cancel pending operations before removing from the list, we can reconsider the assumption that an active request is available during that time.