fix: count a timeout once, and let the failure rate see it - #57
Merged
Merged
Conversation
Two rules, one owner each. onTimeout owns the timeouts counter. Giving up is what that counter means, and that hook is the only place that knows whose deadline elapsed. It no longer books totalFailed at all: every node already has one path that books it when the request ends there, resolve for the root client and the fan-out join for everything below, and both fire for every reason rather than only this one. resolve owns the root's totalFailed, for every reason alike, and no longer adds a second timeout to the client's counter. Before this, a client calling its dependency directly was credited twice by two paths for one dead request, and a node between the client and the slow dependency was credited twice again by onTimeout and the join. Measured over 30s, per topology, node failures against what the system booked: client -> service client 2743 -> 1494 system 1494 client -> api -> db api 2362 -> 1446 system 1446 partial 26% loss client 820 -> 410 system 410 The timeout rate no longer outruns the load: 50 rps offered read 91 timeouts a second and now reads 46. With the counting fixed, errorRate can include timeouts, which is the bug this started from. It left them out of both halves, so the cell the canvas labels "failing" was the failure rate among requests that did not time out, and a caller losing a quarter of its traffic read 0%. That case now reads 26%, and a middle node losing everything to its dependency reads 100% where it read 0%. Attribution still lands where the waiting happened. In client -> api -> db the api holds the timeouts and the client holds none, because the client never gave up: it was handed a failure. The client's totalFailed still counts it, from resolve. timeoutAttribution.test.ts pins both rules across all three topologies: no double at the root, the root still booked when a node below it gave up, no node failing more often than the system did, a client that cannot time out faster than it offers load, the counter landing on whoever waited, and the failure rate tracking the traffic actually lost. Seven of the ten fail against the old engine. Closes xevrion#51.
|
@yashksaini-coder is attempting to deploy a commit to the whoarrryou's projects Team on Vercel. A member of the Team first needs to authorize it. |
Owner
|
Good catch on the chain case, you were right and my suggestion would have broken it. Checked the numbers, they hold. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51. Both halves in one PR, as you asked, since the second is not verifiable without the
first.
Where I ended up, and where it differs from (a)
You said
onTimeoutis the single owner and the timeout arm inresolveshould go. That is whatthis does for the
timeoutscounter.totalFailedneeded one more step, and I would rather showyou the measurements than argue it.
Dropping the arm literally, so
resolveskips the client entirely on a timeout, fixesclient -> serviceand breaksclient -> api -> db:The client is losing 100% of its traffic and reports a third of it.
onTimeoutcredits whoevergave up, which there is the api, so removing the
resolvebranch leaves nothing booking the rootat all.
Then chasing that turned up the same double one level down.
engine.ts:2111books the parent'sfailure when a fan-out join completes with a failed child, and
onTimeouthad already booked thesame node, so the api came out at 2362 against a system total of 1446.
So the split that holds everywhere is:
onTimeoutowns thetimeoutscounter, and books nototalFailedat all. Every nodealready has one path that books that when the request ends there:
resolvefor the root client,the join for everything below it. Both fire for every reason rather than only this one.
resolveowns the root'stotalFailed, for every reason alike, and no longer adds a secondtimeout to the client's counter.
That is one rule each, which I think is the thing you were after. It also means the shed and
generic-error arms you flagged are untouched:
client.totalFailed++still runs for every reason,so a root shed keeps its count and needed no splitting.
Measured
30 seconds per topology, node failures against what the system booked:
client -> service, clientclient -> api -> db, apiclient -> api -> db, clientThe timeout rate no longer outruns the load: 50 rps offered read 91 timeouts a second, now 46.
And the bug this started from, once the counting is right:
A middle node losing everything to its dependency reads 100% where it read 0%.
Attribution still lands where the waiting happened. In
client -> api -> dbthe api holds thetimeouts and the client holds none, because the client never gave up, it was handed a failure.
The client's
totalFailedstill counts it, fromresolve.Tests
timeoutAttribution.test.ts, ten cases over the three topologies: no double at the root, the rootstill booked when a node below it gave up, no node failing more often than the system did, a
client that cannot time out faster than it offers load, the counter landing on whoever waited, and
the failure rate tracking the traffic actually lost.
Seven of the ten fail against the old engine, checked by putting it back. The three that pass are
the ones that should: a client that gives up directly is still credited, and the bounds hold
either way.
921 tests pass. typecheck, lint and format:check clean, lint on the same 26 warnings as main.