prom-client
prom-client copied to clipboard
siimon
fix cluster metrics when AggregatorReg isn't created on workers
When using the cluster module, you must now call promclient.setupClusterWorker() from each cluster worker.
v13.2.0 introduced a change from v13.1.0 (#449) that broke cluster metrics if an AggregatorRegistry was not instantiated on each cluster worker. The example in examples/cluster.js shows instantiation of an AggregatorRegistry in the workers, so users following that example were not affected by this change. However, the example was not written as intended: new AggregatorRegistry() should only be called on the cluster master. examples/cluster.js has been updated to show the full, correct usage.
This also unblocks a new approach to #182.
Desperated need this fix! I'm getting some errors running metrics on nodejs cluster. The metrics are not being aggregated, it´s simply does not work on cluster. Is this a fix for it?
Any plans on release this fix?
I figured out master cluster process metrics are not being aggregated with worker process metrics on clusterMetrics() call.
Should it be? Or I'm guessing wrong?
I did a change pushing master cluster metrics and it works... please check below.
function addListeners() {
if (listenersAdded) return;
listenersAdded = true;
if (cluster().isMaster) {
// 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);
if (message.error) {
request.done(new Error(message.error));
return;
}
message.metrics.forEach(registry => request.responses.push(registry));
request.pending--;
if (request.pending === 0) {
// finalize
requests.delete(message.requestId);
clearTimeout(request.errorTimeout);
// ------------------------------------
// CHECK HERE BEGIN
// ------------------------------------
Promise.all(registries.map(r => r.getMetricsAsJSON()))
.then(metrics => {
request.responses.push(...metrics)
const registry = AggregatorRegistry.aggregate(request.responses);
const promString = registry.metrics();
request.done(null, promString);
})
.catch(error => {
console.error(error)
});
// ------------------------------------
// CHECK HERE END
// ------------------------------------
}
}
});
}
if (cluster().isWorker) {
// 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,
});
});
}
});
}
}
@lucianodltec check out #280. That PR just needs tests. If you're willing to work on them, that would be awesome!
Hi @zbjornson! Thanks for making this fix. Do you know if there is any plan to merge it?
@SimenB will do shortly. I want to revisit this PR to see if I can make both patterns work so it's not a breaking change in either direction.
@zbjornson
This would be great to unblock some work, looking forward to this being merged!
👋 👋 Hey team!! Any movement on this issue? We are in need of this fix, hoping it's landing soon 🤞
@zbjornson I landed next on master and did a prerelease. So any breaking changes should be fine to land
@zbjornson Any update on this one? We are stuck with 13.1.0 and would be happy to migrate to latest. Thank you in advance 🤞
@zbjornson I plan to release a stable v15 this week. Will you be able to work on the two issues in https://github.com/siimon/prom-client/issues?q=is:open+label:semver-major? If yes, I'll hold off - if not I'll just roll it out 🙂 Been more than half a year since exemplars landed without a release 😅