-
Notifications
You must be signed in to change notification settings - Fork 428
Record cluster worker scrape failures #850
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| */ | ||
|
|
||
| const { debuglog } = require('node:util'); | ||
| const Histogram = require('./histogram'); | ||
| const Registry = require('./registry'); | ||
| const { waitFor } = require('./util'); | ||
|
|
||
|
|
@@ -41,6 +42,14 @@ const GET_METRICS_REQ = '@prometheus-io/client:getMetricsReq'; | |
| const GET_METRICS_RES = '@prometheus-io/client:getMetricsRes'; | ||
| const GOODBYE = '@prometheus-io/client:goodbye'; | ||
|
|
||
| const clusterWorkerScrapeFailures = new Histogram({ | ||
| name: 'prom_client_cluster_worker_scrape_failures', | ||
| help: 'Number of workers that failed to return metrics during a failed cluster scrape.', | ||
| buckets: [0, 1, 2, 4, 8, 16, 32], | ||
| // Register on construction, so importing the module does not add metrics. | ||
| registers: [], | ||
| }); | ||
|
|
||
| let registries = [Registry.globalRegistry]; | ||
| let listenersAdded = false; | ||
| let requestCtr = 0; // Concurrency control | ||
|
|
@@ -55,6 +64,7 @@ class AggregatorRegistry extends Registry { | |
| */ | ||
| constructor(regContentType = Registry.PROMETHEUS_CONTENT_TYPE) { | ||
| super(regContentType); | ||
| Registry.globalRegistry.registerMetric(clusterWorkerScrapeFailures); | ||
|
|
||
| addListeners(); | ||
| } | ||
|
|
@@ -88,11 +98,14 @@ class AggregatorRegistry extends Registry { | |
| ); | ||
|
|
||
| const responsePromises = [this.#selfMetrics(), ...workerMetrics]; | ||
| const timeoutError = new Error('Timeout'); | ||
| const request = { | ||
| responseHandlers, | ||
| workerFailures: 0, | ||
| promise: waitFor( | ||
| this.#gather(requestId, metricSnapshot, responsePromises), | ||
| 5_000, | ||
| timeoutError, | ||
| ), | ||
| }; | ||
|
|
||
|
|
@@ -105,7 +118,22 @@ class AggregatorRegistry extends Registry { | |
|
|
||
| return await request.promise; | ||
| } catch (err) { | ||
| if (err.message === 'Timeout') { | ||
| const timedOut = err === timeoutError; | ||
| let failedWorkers = request.workerFailures; | ||
|
|
||
| if (timedOut) { | ||
| failedWorkers += request.responseHandlers.size; | ||
| } | ||
|
|
||
| clusterWorkerScrapeFailures.observe(failedWorkers); | ||
|
|
||
| // Sending can throw before request.promise is awaited. | ||
| for (const response of request.responseHandlers.values()) { | ||
| response.reject(err); | ||
| } | ||
| request.promise.catch(() => {}); | ||
|
|
||
| if (timedOut) { | ||
| throw new Error( | ||
| `Operation timed out. ${request.responseHandlers.size} outstanding responses.`, | ||
| ); | ||
|
|
@@ -118,8 +146,22 @@ class AggregatorRegistry extends Registry { | |
| } | ||
|
|
||
| async #selfMetrics() { | ||
| const metrics = await Promise.all( | ||
| registries.map(r => r.getMetricsAsJSON()), | ||
| ); | ||
| // Custom registries may not contain the coordinator's internal metric. | ||
| if ( | ||
| !registries.some( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if it's being added to the registry you don't need this check. |
||
| r => | ||
| r.getSingleMetric(clusterWorkerScrapeFailures.name) === | ||
| clusterWorkerScrapeFailures, | ||
| ) | ||
| ) { | ||
| metrics.push([await clusterWorkerScrapeFailures.get()]); | ||
| } | ||
|
|
||
| return { | ||
| metrics: await Promise.all(registries.map(r => r.getMetricsAsJSON())), | ||
| metrics, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -348,6 +390,7 @@ async function primaryListener(worker, event) { | |
| request.responseHandlers.delete(worker.id); | ||
|
|
||
| if (event.error) { | ||
| request.workerFailures++; | ||
| response.reject(new Error(event.error)); | ||
| } else { | ||
| response.resolve({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
|
|
||
| const { debuglog } = require('node:util'); | ||
| const worker = require('node:worker_threads'); | ||
| const Histogram = require('./histogram'); | ||
| const Registry = require('./registry'); | ||
| const { waitFor } = require('./util'); | ||
|
|
||
|
|
@@ -36,6 +37,14 @@ const GET_METRICS_REQ = '@prometheus-io/client:getMetricsReq'; | |
| const GET_METRICS_RES = '@prometheus-io/client:getMetricsRes'; | ||
| const GOODBYE = '@prometheus-io/client:goodbye'; | ||
|
|
||
| const workerScrapeFailures = new Histogram({ | ||
| name: 'prom_client_worker_scrape_failures', | ||
| help: 'Number of workers that failed to return metrics during a failed worker scrape.', | ||
| buckets: [0, 1, 2, 4, 8, 16, 32], | ||
| // Register on construction, so importing the module does not add metrics. | ||
| registers: [], | ||
| }); | ||
|
|
||
| const ANNOUNCEMENT_CHANNEL = new BroadcastChannel( | ||
| '@prometheus-io/client:announce', | ||
| ).unref(); | ||
|
|
@@ -60,6 +69,7 @@ class WorkerRegistry extends Registry { | |
| primary = isMainThread, | ||
| ) { | ||
| super(regContentType); | ||
| Registry.globalRegistry.registerMetric(workerScrapeFailures); | ||
| this.primary = primary; | ||
|
|
||
| addListeners(primary); | ||
|
|
@@ -78,17 +88,12 @@ class WorkerRegistry extends Registry { | |
| ); | ||
|
|
||
| if (orderedWorkers.length === 0) { | ||
| if (historicMetrics.length === 0) { | ||
| debug('No data found for requestId', requestId); | ||
| return ''; | ||
| } else { | ||
| debug('No workers found for requestId', requestId); | ||
| } | ||
| debug('No workers found for requestId', requestId); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this change? |
||
| } | ||
|
|
||
| const metricSnapshot = historicMetrics; | ||
| const responseHandlers = new Map(); | ||
| const responsePromises = orderedWorkers.map( | ||
| const workerMetrics = orderedWorkers.map( | ||
| entry => | ||
| new Promise((resolveResponse, rejectResponse) => { | ||
| responseHandlers.set(entry.name, { | ||
|
|
@@ -98,11 +103,15 @@ class WorkerRegistry extends Registry { | |
| }), | ||
| ); | ||
|
|
||
| const responsePromises = [this.#selfMetrics(), ...workerMetrics]; | ||
| const timeoutError = new Error('Timeout'); | ||
| const request = { | ||
| responseHandlers, | ||
| workerFailures: 0, | ||
| promise: waitFor( | ||
| this.#gather(requestId, metricSnapshot, responsePromises), | ||
| 5_000, | ||
| timeoutError, | ||
| ), | ||
| }; | ||
|
|
||
|
|
@@ -117,7 +126,22 @@ class WorkerRegistry extends Registry { | |
|
|
||
| return await request.promise; | ||
| } catch (err) { | ||
| if (err.message === 'Timeout') { | ||
| const timedOut = err === timeoutError; | ||
| let failedWorkers = request.workerFailures; | ||
|
|
||
| if (timedOut) { | ||
| failedWorkers += request.responseHandlers.size; | ||
| } | ||
|
|
||
| workerScrapeFailures.observe(failedWorkers); | ||
|
|
||
| // Sending can throw before request.promise is awaited. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| for (const response of request.responseHandlers.values()) { | ||
| response.reject(err); | ||
| } | ||
| request.promise.catch(() => {}); | ||
|
|
||
| if (timedOut) { | ||
| throw new Error( | ||
| `Operation timed out. ${request.responseHandlers.size} outstanding responses.`, | ||
| ); | ||
|
|
@@ -129,6 +153,23 @@ class WorkerRegistry extends Registry { | |
| } | ||
| } | ||
|
|
||
| async #selfMetrics() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also not needed. This thread is listening for the broadcast channel that sends the metrics. So again double reporting. |
||
| const metrics = await Promise.all( | ||
| registries.map(r => r.getMetricsAsJSON()), | ||
| ); | ||
| // Custom registries may not contain the coordinator's internal metric. | ||
| if ( | ||
| !registries.some( | ||
| r => | ||
| r.getSingleMetric(workerScrapeFailures.name) === workerScrapeFailures, | ||
| ) | ||
| ) { | ||
| metrics.push([await workerScrapeFailures.get()]); | ||
| } | ||
|
|
||
| return { metrics }; | ||
| } | ||
|
|
||
| /** | ||
| * Collect the data for a metrics request. | ||
| * @param requestId {number} | ||
|
|
@@ -277,11 +318,11 @@ function addListeners(primary) { | |
| announce(name, false); | ||
| } | ||
| } else if (message.type === GET_METRICS_REQ) { | ||
| const metrics = await Promise.all( | ||
| registries.map(r => r.getMetricsAsJSON()), | ||
| ); | ||
|
|
||
| try { | ||
| const metrics = await Promise.all( | ||
| registries.map(r => r.getMetricsAsJSON()), | ||
| ); | ||
|
|
||
| channel.postMessage({ | ||
| type: GET_METRICS_RES, | ||
| requestId: message.requestId, | ||
|
|
@@ -375,6 +416,7 @@ async function primaryListener(event) { | |
| request.responseHandlers.delete(workerName); | ||
|
|
||
| if (workerMessage.error) { | ||
| request.workerFailures++; | ||
| response.reject(new Error(workerMessage.error)); | ||
| } else { | ||
| response.resolve({ | ||
|
|
||
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 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.