fix(cluster): aggregate metrics in a deterministic order#771
Conversation
| function sumValues(values) { | ||
| return values | ||
| .slice() | ||
| .sort((a, b) => a.value - b.value) |
There was a problem hiding this comment.
This is not the place or the field that needs to be sorted. The results need to be sorted by threadId at the caller.
And that could be achieved by creating a promise at the point of the message send, and doing a Promise.all() on the results.
Something slightly different might need to be done in worker.js
There was a problem hiding this comment.
Thanks — understood. I put determinism in the wrong layer. I will remove the aggregator-level sort and instead collect responses in stable worker-ID order in cluster.js and stable thread-ID order in worker.js, with regression tests that reverse response arrival. I understand this addresses response-order nondeterminism for a stable worker set, not changes caused by workers entering or exiting. Is that the scope you want before I update the branch?
Collect cluster responses in numeric worker-ID order with per-worker promises, and worker-thread responses in numeric thread-ID order. Add reverse-arrival regression coverage for both paths. Fixes prometheus#539 Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
| return; | ||
| } | ||
|
|
||
| for (const worker of workers) worker.send(message); |
There was a problem hiding this comment.
If this was rolled into the function starting on line 87 you wouldn't need the 'deferred()' function, which is a confusing name for what it does, since it doesn't do anything that's deferred.
There was a problem hiding this comment.
Addressed in b64dc65: removed the deferred helper and now create each response promise inline where the worker message is sent.
| if (response === undefined) { | ||
| return; | ||
| } | ||
| request.responses.delete(worker.id); |
There was a problem hiding this comment.
I think your AI is slipping. Two functions, same purpose, different APIs.
In one you're using has() to short circuit on results that have already been reported (which probably shouldn't happen anyway and could perhaps use a debug statement here) and in the other you're deleting keys from the Map.
They should be built on the same strategies.
There was a problem hiding this comment.
Addressed in b64dc65: cluster.js and worker.js now use the same per-peer response-handler lifecycle. Each removes the pending handler when a response arrives, resolves or rejects through Promise.all(), and aggregates in stable ID order. The full npm test gate passes with 543 tests.
Inline per-peer response promises and use the same response-handler lifecycle for cluster workers and worker threads. Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
2 participants
Problem
Cluster metric aggregation (
lib/metricAggregators.js) sums worker-reported values in the order workers respond to theGET_METRICS_REQmessage. Because floating-point addition is not associative, the same set of values summed in different orders can produce slightly different results (e.g.1.4765105vs1.4765104999999998). As reported in #539, this non-determinism can trigger a false "reset" detection in Prometheus and corrupt graphs.Fix
sumValues) so thesumandaverageaggregators produce a reproducible result regardless of the order in which workers respond.Checks (Node 22)
npm run test-unit— 543 passing, including the new order-independence test.Fixes #539
Disclosure: this change was prepared with AI assistance and reviewed by me before submitting. I ran the check above in a network-isolated container and published a signed, re-runnable record of that run, verifiable via GitHub artifact attestation: https://northset-oss.github.io/verification-pilot/. Contributor self-run, not a maintainer verification.