Record cluster worker scrape failures - #850
Conversation
Signed-off-by: SirGundir <crstalhopkin@gmail.com>
94a6e40 to
801c14c
Compare
| } | ||
|
|
||
| if (failedWorkers > 0) { | ||
| clusterWorkerScrapeFailures.observe(failedWorkers); |
There was a problem hiding this comment.
I think observing 0's when there are no workers is probably unlikely to happen but is also extra information about the class of failure we may be dealing with. As written this will never catch unclassified errors such as an NPE in the try block.
For instance, regressions in the Node API are not unknown. I work on a project that doesn't work on node 24.7-24.16, for instance, due to breaking changes in child_process.fork()
jdmarshall
left a comment
jdmarshall
left a comment
There was a problem hiding this comment.
This same change should be made in worker.js as well, please, to retain feature parity.
| const metrics = await Promise.all( | ||
| registries.map(r => r.getMetricsAsJSON()), | ||
| ); | ||
| metrics.push([await clusterWorkerScrapeFailures.get()]); |
There was a problem hiding this comment.
Are you sure you're not double-reporting this? That should be in the default registry.
Record unclassified failures, add worker-thread parity, register the histograms in the default registry, and distinguish scrape deadlines from collector errors with the same message. Add regression tests and a changelog entry. Signed-off-by: SirGundir <crstalhopkin@gmail.com>
|
Thanks for the review! I've added worker-thread support, zero-valued failure observations, and default-registry registration without double-counting. I also fixed the Timeout error mix-up and added the changelog entry. All 633 tests pass locally |
| ); | ||
|
|
||
| const responsePromises = [this.#selfMetrics(), ...workerMetrics]; | ||
| const timeoutError = new Error('Timeout'); |
There was a problem hiding this comment.
This can get a bit weird with stack traces. I think you're better off just letting waitFor create the error rather than passing one around.
| ); | ||
| // Custom registries may not contain the coordinator's internal metric. | ||
| if ( | ||
| !registries.some( |
There was a problem hiding this comment.
I think if it's being added to the registry you don't need this check.
| } else { | ||
| debug('No workers found for requestId', requestId); | ||
| } | ||
| debug('No workers found for requestId', requestId); |
|
|
||
| workerScrapeFailures.observe(failedWorkers); | ||
|
|
||
| // Sending can throw before request.promise is awaited. |
There was a problem hiding this comment.
This is an AI hallucination?
You're rejecting and then squelching the error handler after rejection? I don't think we need any of this.
| } | ||
| } | ||
|
|
||
| async #selfMetrics() { |
There was a problem hiding this comment.
Also not needed. This thread is listening for the broadcast channel that sends the metrics. So again double reporting.
2 participants
Summary
Add an internal histogram named
prom_client_cluster_worker_scrape_failuresto record failed cluster metriccollections.
Each observation contains the number of workers that failed to return metrics:
clusterMetrics()call.A histogram preserves information from consecutive failures instead of only
retaining the most recent value, as a gauge would.
Testing
Added tests covering: