-
Notifications
You must be signed in to change notification settings - Fork 413
Cluster fixes #789
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
Open
jdmarshall
wants to merge
6
commits into
prometheus:main
Choose a base branch
from
jdmarshall:clusterFixes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Cluster fixes #789
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9260f95
Rework cluster and worker lifecycle logic to be more similar.
jdmarshall 580ca69
Fixing missing unref for BroadcastChannel.
jdmarshall 6336df8
Export stats from the primary thread.
jdmarshall a652f07
Rework state tracking to support bot #155 and #788
jdmarshall 50b96f0
fix(cluster): skip responses after IPC disconnect
alencristen 1adc9d5
Simplify process.send sanity checks.
jdmarshall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||
| * cluster master. | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const { debuglog } = require('node:util'); | ||||||||||||||||||||||||||||||||||||||||||||||
| const Registry = require('./registry'); | ||||||||||||||||||||||||||||||||||||||||||||||
| // We need to lazy-load the 'cluster' module as some application servers - | ||||||||||||||||||||||||||||||||||||||||||||||
| // namely Passenger - crash when it is imported. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -31,17 +32,25 @@ let cluster = () => { | |||||||||||||||||||||||||||||||||||||||||||||
| return data; | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const debug = debuglog('prom:metrics:cluster'); | ||||||||||||||||||||||||||||||||||||||||||||||
| const ANNOUNCEMENT = '@prometheus-io/client:announcement'; | ||||||||||||||||||||||||||||||||||||||||||||||
| const GET_METRICS_REQ = '@prometheus-io/client:getMetricsReq'; | ||||||||||||||||||||||||||||||||||||||||||||||
| const GET_METRICS_RES = '@prometheus-io/client:getMetricsRes'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| let registries = [Registry.globalRegistry]; | ||||||||||||||||||||||||||||||||||||||||||||||
| let requestCtr = 0; // Concurrency control | ||||||||||||||||||||||||||||||||||||||||||||||
| let listenersAdded = false; | ||||||||||||||||||||||||||||||||||||||||||||||
| const requests = new Map(); // Pending requests for workers' local metrics. | ||||||||||||||||||||||||||||||||||||||||||||||
| const workers = new Map(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| class AggregatorRegistry extends Registry { | ||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
| * Create a Registry. | ||||||||||||||||||||||||||||||||||||||||||||||
| * @param regContentType | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| constructor(regContentType = Registry.PROMETHEUS_CONTENT_TYPE) { | ||||||||||||||||||||||||||||||||||||||||||||||
| super(regContentType); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| addListeners(); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -53,9 +62,9 @@ class AggregatorRegistry extends Registry { | |||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| clusterMetrics() { | ||||||||||||||||||||||||||||||||||||||||||||||
| const requestId = requestCtr++; | ||||||||||||||||||||||||||||||||||||||||||||||
| const workers = Object.values(cluster().workers) | ||||||||||||||||||||||||||||||||||||||||||||||
| .filter(worker => worker.isConnected()) | ||||||||||||||||||||||||||||||||||||||||||||||
| .sort((left, right) => left.id - right.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| const orderedWorkers = [...workers.values()].sort( | ||||||||||||||||||||||||||||||||||||||||||||||
| (left, right) => left.id - right.id, | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| return new Promise((resolve, reject) => { | ||||||||||||||||||||||||||||||||||||||||||||||
| let settled = false; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -78,36 +87,43 @@ class AggregatorRegistry extends Registry { | |||||||||||||||||||||||||||||||||||||||||||||
| responseHandlers, | ||||||||||||||||||||||||||||||||||||||||||||||
| done, | ||||||||||||||||||||||||||||||||||||||||||||||
| errorTimeout: setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||
| const err = new Error('Operation timed out.'); | ||||||||||||||||||||||||||||||||||||||||||||||
| const err = new Error( | ||||||||||||||||||||||||||||||||||||||||||||||
| `Operation timed out. ${request.responseHandlers.size} outstanding responses.`, | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
| request.done(err); | ||||||||||||||||||||||||||||||||||||||||||||||
| }, 5000), | ||||||||||||||||||||||||||||||||||||||||||||||
| }, 5_000), | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
| requests.set(requestId, request); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const message = { | ||||||||||||||||||||||||||||||||||||||||||||||
| type: GET_METRICS_REQ, | ||||||||||||||||||||||||||||||||||||||||||||||
| requestId, | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (workers.length === 0) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // No workers were up | ||||||||||||||||||||||||||||||||||||||||||||||
| process.nextTick(() => done(undefined, '')); | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const responsePromises = workers.map( | ||||||||||||||||||||||||||||||||||||||||||||||
| const workerMetrics = orderedWorkers.map( | ||||||||||||||||||||||||||||||||||||||||||||||
| worker => | ||||||||||||||||||||||||||||||||||||||||||||||
| new Promise((resolveResponse, rejectResponse) => { | ||||||||||||||||||||||||||||||||||||||||||||||
| responseHandlers.set(worker.id, { | ||||||||||||||||||||||||||||||||||||||||||||||
| resolve: resolveResponse, | ||||||||||||||||||||||||||||||||||||||||||||||
| reject: rejectResponse, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| worker.send(message); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| worker.send({ | ||||||||||||||||||||||||||||||||||||||||||||||
| type: GET_METRICS_REQ, | ||||||||||||||||||||||||||||||||||||||||||||||
| requestId, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Promise.all(responsePromises) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then(metrics => Registry.aggregate(metrics.flat()).metrics()) | ||||||||||||||||||||||||||||||||||||||||||||||
| const myMetrics = Promise.all( | ||||||||||||||||||||||||||||||||||||||||||||||
| registries.map(r => r.getMetricsAsJSON()), | ||||||||||||||||||||||||||||||||||||||||||||||
| ).then(metrics => { | ||||||||||||||||||||||||||||||||||||||||||||||
| return { metrics }; | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (workerMetrics.length === 0) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('No workers found for requestId', requestId); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const allMetrics = [myMetrics, ...workerMetrics]; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Promise.all(allMetrics) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then(responses => responses.flatMap(response => response.metrics)) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then(metrics => Registry.aggregate(metrics).metrics()) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then(result => done(undefined, result), done); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -158,54 +174,145 @@ class AggregatorRegistry extends Registry { | |||||||||||||||||||||||||||||||||||||||||||||
| * @returns {void} | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| function addListeners() { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (listenersAdded) return; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (listenersAdded) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| listenersAdded = true; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (cluster().isPrimary) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Listen for worker responses to requests for local metrics | ||||||||||||||||||||||||||||||||||||||||||||||
| cluster().on('message', (worker, message) => { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (message.type === GET_METRICS_RES) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const request = requests.get(message.requestId); | ||||||||||||||||||||||||||||||||||||||||||||||
| replaceListener('message', cluster(), primaryListener); | ||||||||||||||||||||||||||||||||||||||||||||||
| replaceListener('disconnect', cluster(), disconnect); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (request === undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| announce(); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| replaceListener('message', process, workerListener); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const response = request.responseHandlers.get(worker.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (response === undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| request.responseHandlers.delete(worker.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (typeof process.send !== 'function') { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('worker has no process.send()'); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else if (!process.connected) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('worker is not connected to parent process'); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| process.send({ type: ANNOUNCEMENT }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (message.error) { | ||||||||||||||||||||||||||||||||||||||||||||||
| response.reject(new Error(message.error)); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| response.resolve(message.metrics); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
| * Watch for metrics events and aggregator announcements | ||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||
| * Whereas clusters are a top-level activity, multiple modules may start their | ||||||||||||||||||||||||||||||||||||||||||||||
| * own workers and require telemetry collection. | ||||||||||||||||||||||||||||||||||||||||||||||
| * @param message {MessageEvent} | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| async function workerListener(message) { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (message.type === ANNOUNCEMENT) { | ||||||||||||||||||||||||||||||||||||||||||||||
| process.send({ type: ANNOUNCEMENT }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else if (message.type === GET_METRICS_REQ) { | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| const metrics = await Promise.all( | ||||||||||||||||||||||||||||||||||||||||||||||
| registries.map(r => r.getMetricsAsJSON()), | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (!process.connected) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('Connection to primary lost.'); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| process.send({ | ||||||||||||||||||||||||||||||||||||||||||||||
| type: GET_METRICS_RES, | ||||||||||||||||||||||||||||||||||||||||||||||
| requestId: message.requestId, | ||||||||||||||||||||||||||||||||||||||||||||||
| metrics, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Respond to master's requests for worker's local metrics. | ||||||||||||||||||||||||||||||||||||||||||||||
| process.on('message', message => { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (message.type === GET_METRICS_REQ) { | ||||||||||||||||||||||||||||||||||||||||||||||
| Promise.all(registries.map(r => r.getMetricsAsJSON())) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then(metrics => { | ||||||||||||||||||||||||||||||||||||||||||||||
| process.send({ | ||||||||||||||||||||||||||||||||||||||||||||||
| type: GET_METRICS_RES, | ||||||||||||||||||||||||||||||||||||||||||||||
| requestId: message.requestId, | ||||||||||||||||||||||||||||||||||||||||||||||
| metrics, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||
| .catch(error => { | ||||||||||||||||||||||||||||||||||||||||||||||
| process.send({ | ||||||||||||||||||||||||||||||||||||||||||||||
| type: GET_METRICS_RES, | ||||||||||||||||||||||||||||||||||||||||||||||
| requestId: message.requestId, | ||||||||||||||||||||||||||||||||||||||||||||||
| error: error.message, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('Error sending to primary', error); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!process.connected) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('Connection to primary lost.'); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| process.send({ | ||||||||||||||||||||||||||||||||||||||||||||||
| type: GET_METRICS_RES, | ||||||||||||||||||||||||||||||||||||||||||||||
| requestId: message.requestId, | ||||||||||||||||||||||||||||||||||||||||||||||
| error: error.message, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
| * Add workers to the aggregation list when they are announced. | ||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||
| * Whereas clusters are a top-level activity, multiple modules may start their | ||||||||||||||||||||||||||||||||||||||||||||||
| * own workers and require telemetry collection. | ||||||||||||||||||||||||||||||||||||||||||||||
| * @param event {MessageEvent} | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| async function primaryListener(worker, event) { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (event.type === ANNOUNCEMENT) { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (workers.has(worker.id)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('duplicate worker announcement', worker.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| workers.set(worker.id, worker); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else if (event.type === GET_METRICS_RES) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const request = requests.get(event.requestId); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (request === undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('unexpected results from worker', worker.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const response = request.responseHandlers.get(worker.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (response === undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| request.responseHandlers.delete(worker.id); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (event.error) { | ||||||||||||||||||||||||||||||||||||||||||||||
| response.reject(new Error(event.error)); | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| response.resolve({ | ||||||||||||||||||||||||||||||||||||||||||||||
| threadId: worker.id, | ||||||||||||||||||||||||||||||||||||||||||||||
| metrics: event.metrics, | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| function disconnect(event) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('worker disconnected', event.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| workers.delete(event.id); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| function announce() { | ||||||||||||||||||||||||||||||||||||||||||||||
| for (const worker of Object.values(cluster().workers)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (worker.isConnected()) { | ||||||||||||||||||||||||||||||||||||||||||||||
| worker.send({ type: ANNOUNCEMENT }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
| * Replace any listeners with new ones. | ||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||
| * @param messageType | ||||||||||||||||||||||||||||||||||||||||||||||
| * @param emitter {EventEmitter} | ||||||||||||||||||||||||||||||||||||||||||||||
| * @param fn | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| function replaceListener(messageType, emitter, fn) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Reloading a module creates a unique instance of each function, so the | ||||||||||||||||||||||||||||||||||||||||||||||
| // identity checks is cluster.off() will fail. | ||||||||||||||||||||||||||||||||||||||||||||||
| const functionString = fn.toString(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| for (const listener of emitter.listeners(messageType)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line eqeqeq | ||||||||||||||||||||||||||||||||||||||||||||||
| if (functionString == listener) { | ||||||||||||||||||||||||||||||||||||||||||||||
| debug('removing duplicate listener', messageType); | ||||||||||||||||||||||||||||||||||||||||||||||
| emitter.off(messageType, listener); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| emitter.on(messageType, fn); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+295
to
317
Member
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. For above.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
| module.exports = AggregatorRegistry; | ||||||||||||||||||||||||||||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
I've been trying to understand an issue flagged by LLM. "replaceListener" was removing listeners from the "old" registry, but that means you can lose metrics as they won't be called anymore. So reverting to simply doing
on()withoutreplaceListenerfixes that, see diff and regression test. Is this what you also flagged here ?It does reintroduce #155 warning in "listeners don't accumulate" in test/clusterTest.js, but there is a way to fix it apparently (in a separate PR):
Regression test for this:
Uh oh!
There was an error while loading. Please reload this page.
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.
Major edit:
Because the responses are being aggregated through a promise, only the first result was ever being seen anyway. In fact what you were probably always seeing before was the oldest or second oldest metrics per process, based on when the event was delivered and processing time to gather the metrics. Which is exactly the wrong data for functional and integration tests.
To the best of my knowledge prom-client has never worked with hot reload. Let alone well. And anyone would see that it doesn't within a few minutes of trying, especially if they used older versions that were especially crabby about this.
We have a bigger problem with what to do about dead workers. Because their metrics disappear when they do, and since we are gathering them, we are getting the wrong answers for counts and gauges. #803 which is a problem since the general wisdom is 'let the process crash' when unhandledException or unhandledRejection fires.
What I think that suggests is an update to the README, suggesting you let a Prometheus sidecar handle the aggregation in Serious Projects rather than using cluster.js or worker.js
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.
I'm sorry I'm having a hard time understanding this :( I'll let this go after this.
In the proposed test, there's no first/second result, it's one result per instance. Also using
on()doesn't change that we have a promise which still keeps only one result if there were multiple. Shouldn't some test break if usingon()changed something about handling multiple messages? E.g.does not error out on unexpected (or late) responsesoraggregates worker responses in worker id ordertest?The proposed test is about multiple instance at the same time sending just one message and in that case
replaceListenermakes us completely lose one set of results.I've asked the LLM to show what's the worse that can happen if we keep replaceListener and under what circumstances, I'm ok with approving this PR and filing an issue instead based on this:
PR #789 — risk of keeping
replaceListenerAssessment of the worst-case outcome if prometheus/client_js#789
is approved with
replaceListenerretained inlib/cluster.js.Tested against commit
1adc9d5("Simplify process.send sanity checks."), Node v20.9.0,compared against the same tree with
replaceListenerreverted to plainon().The worst case is worse than "missing metrics" — it's a crash of the cluster primary,
and it's deterministic.
The failure chain
replaceListenerstrips both listeners from the earlier instance —messageanddisconnect. That second one is what escalates this. Reproduced 3/3, app registryconstructed first, a nested copy loading later:
Three stages:
workersmap, but responses no longer route to it. Worse than the empty-map case —that returned fast and wrong; this hangs the metrics endpoint for 5 s and then 500s.
disconnectlistener went with themessageone.worker.send()on a dead worker emitsERR_IPC_CHANNEL_CLOSEDas an'error'event on the Worker object. Nothing listens for it, so Node throws and the primary
exits — taking the whole cluster down, since the primary is the process manager.
Same scenario with plain
on():wc_counter=1in 2 ms, correctly reflecting thesurviving worker.
There's an uncomfortable detail here — the PR body says "Fixes #563", and
#563 is "[BUG] ERR_IPC_CHANNEL_CLOSED
showing up on new deployments".
replaceListenercan produce precisely that error, fatally.How likely is it
All of these must hold:
primaryListenersource — different versions won't dedup, so this needs same-versionduplication.
ClusterRegistryin the primary. Requiring twice is harmless;addListeners()only runs from the constructor. This is the narrow link — most appsconstruct once. It opens up when a framework plugin or APM wrapper instruments
alongside app code.
so it's roughly a coin flip on init order.
is a matter of time, and is the normal response to
unhandledRejection.So: unlikely to hit most users, near-certain for anyone who does hit it, and it recurs on
every boot with that dependency tree. Detectability is poor — the only signal is a
debug()line behindNODE_DEBUG=prom:metrics:cluster, and the symptoms point nowherenear prom-client.
If you approve anyway
Reasonable position — the PR is a large net improvement, and it targets unreleased v0.16,
so there's runway. Two things worth asking for as a condition, both cheap:
Restore the
isConnected()filter inclusterMetrics()— the.filter(worker => worker.isConnected())this PR dropped. Tested: it doesn't fix themetric loss, but it does downgrade the crash to a timeout:
A broken metrics endpoint is survivable; a dead primary isn't. This is worth having
regardless of how the
replaceListenerargument lands, since the samesend()-to-a-dead-worker race exists in a narrower window even with plainon().File a follow-up issue with the reproduction, so the decision is recorded rather than
lost in a review thread.
For what it's worth, the actual fix stays small —
−26/+3, no interaction with anythingelse in the PR — so "approve with
on()" costs the author very little compared toshipping a known primary-crash path.
Appendix: reproduction
Not part of the original assessment — included because the scratch copies used above are
session-temporary. Requires two trees: one as the PR stands, one with
replaceListener('message', cluster(), primaryListener)/replaceListener('disconnect', cluster(), disconnect)/replaceListener('message', process, workerListener)reverted to the equivalent.on(...)calls.