Cluster support #147

zbjornson · 2017-07-30T07:49:47Z

Hello! This is an opening gambit for supporting aggregated metrics for use with node's cluster module. It's not ready for merge; looking for feedback first as to whether or not this would go in prom-client or a separate module that wraps prom-client.

It's only lightly tested, and there are no unit tests yet.

I made the changes completely isolated in a separate file that is not loaded by default. From reading issues #82 and #80, I wasn't sure if a PR that provides this functionality would be accepted, so I wanted the changes to work if the functionality was provided as a separate Node.js module. However, that means that the metric.aggregate stuff is a bit ugly. If this is something you want in prom-client, I could definitely add the aggregation mode to the metrics classes.

Ref #82
Ref #80

/cc @goofballLogic and @tommiv in case you're interested

tommiv · 2017-07-30T07:58:33Z

We actually implemented aggregated metrics via https://github.com/prometheus/statsd_exporter and "push" model statsd driver for node, but it'll be nice to eliminate an additional entity. Thank you.

siimon · 2017-08-01T06:59:55Z

Thanks for your PR :)

Im not against adding cluster support, the thing I am worried about is supporting it in the long term since I lack the experience and time to get up to speed with clustering node processes.

However, but I understand that it's something that probably should be in prom-client since a lot of people apparently cluster out the node processes, so I don't see any reason to not merge it when it looks ready.

A thought, would it be interesting to apply some kind of label to the metric to indicate which node in the cluster the metric is from?

tommiv · 2017-08-01T07:02:36Z

A thought, would it be interesting to apply some kind of label to the metric to indicate which node in the cluster the metric is from?

This is definetely a good idea, we used similar tag in all our setups.

zbjornson · 2017-08-01T15:30:19Z

Thanks for the feedback. I'll work on cleaning it up. I'm happy to help with maintenance of this; I'm more familiar with the cluster module than I ever wanted to be, and I think most prod node.js servers use it.

As far as metric labels: This acts as an aggregator, so all worker processes appear as one. This seemed to be the preferred approach for the Python (gunicorn) and Rails clients; the concern discussed there was that exposing all workers separately would massively increase the number of time series, and percentage metrics like CPU and memory usage would be fractions of an unknown denominator.

Nonetheless, the example file this PR shows setting a metric label to a worker's ID, and the result is that it doesn't get aggregated, so you can keep a metric separate if you want to.

Does that seem reasonable? @tommiv satisfy your use case?

tommiv · 2017-08-01T15:41:07Z

Does that seem reasonable? @tommiv satisfy your use case?

I thought we're talking about a physical node address (or VM), not the cluster node id. But now I see I misread the previous message. We don't split up metrics to the cluster nodes, it makes no sense indeed.

SimenB

Missing tests, but I think it's a great start, and belongs in this lib 😄

SimenB · 2017-08-03T04:13:12Z

example/cluster.js

+	metricsServer.get('/cluster_metrics', (req, res) => {
+		promCluster.clusterMetrics((err, metrics) => {
+			//eslint-disable-next-line no-console
+			if (err) console.log(err);


Meh, it's an example

SimenB · 2017-08-03T04:15:14Z

lib/cluster.js

+			process.send({
+				type: GET_METRICS_RES,
+				requestId: message.requestId,
+				metrics: Registry.globalRegistry.getMetricsAsJSON()


Registry should probably not be hard-coded

SimenB · 2017-08-03T04:17:06Z

lib/cluster.js

+	 *     `registry.getMetricsAsJSON()`.
+	 * @return {Registry} aggregated registry.
+	 */
+	Registry.aggregate = function(metricsArr) {


We have a merge, but that just includes different metrics and doesn't handle dupes?

If so, this function should be added next to merge, not live here

SimenB · 2017-08-03T04:17:46Z

lib/cluster.js

+	 * @param {Function} callback (err, metrics) => any
+	 * @return {undefined} undefined
+	 */
+	Registry.prototype.clusterMetrics = function(callback) {


Don't modify the prototype. Either add directly or use inheritance

SimenB · 2017-08-03T04:18:44Z

lib/cluster.js

+	// define an `aggregate` method, or will be summed by default.
+	// TODO These are here for the sake of isolation, but obviously would be
+	// nicer if they were defined in the metrics/ files.
+	// TODO Might be nice to have multiple aggregates for some, like the


We already export them, right?

SimenB · 2017-08-03T04:19:57Z

lib/cluster.js

+					}
+				});
+			});
+			byLabels.forEach(values => {


result.values = byLabels.map etc

He's using a Map, I don't thing you can call .map directly,
he could convert it to an Array using Array.from an go from there.

SimenB · 2017-08-03T04:20:55Z

lib/cluster.js

+	 * @param {Function} aggregatorFn function to apply to values.
+	 * @return {Function} aggregator function
+	 */
+	const AggregatorFactory = function(aggregatorFn) {


Does this factory have to be within the if? Reviewing on mobile, so not too easy seeing if you close over anything

SimenB · 2017-08-03T04:21:51Z

README.md

+
+To solve this, you can aggregate all of the workers' metrics in the master process. See example folder for a sample usage. By default, custom metrics are summed across workers, and default metrics use sensible defaults. To use a different aggregation method, define an `aggregate()` function for the metric's name in the global registry in the *master* process. (See the `AggregatorFactory` in `lib/cluster.js` for details.)
+
+Note: You must use the default global registry in cluster mode.


In the current arrangement, the cluster master is the one that fulfills the metrics request by querying all of the workers and aggregating their local metrics. The workers need to know which registry to send their local metrics from, so the global registry is easy. I'll try to figure out a nice way to support other registries.

SimenB · 2017-08-03T04:22:59Z

example/cluster.js

+	});
+
+	metricsServer.listen(3001);
+	//eslint-disable-next-line no-console


Can just add a eslintrc in examples/ which disables this rule

SimenB · 2017-08-03T04:23:15Z

.eslintrc

@@ -2,7 +2,8 @@
  "plugins": ["prettier"],
  "extends": "eslint:recommended",
  "env": {
-	"node": true
+	"node": true,
+	"es6": true


Yep! Based on the other new language features you're already using, I don't think this changes the oldest supported version of node.js.

We support node 4 and up, and Map has been in node since 0.12, so we're good 🙂

zbjornson · 2017-08-03T18:39:37Z

Thanks very much for the review. Will work this weekend on making it nice and removing the parts that were awkward as a result of trying to keep it isolated.

zbjornson · 2017-08-07T05:47:19Z

Most comments resolved and most tests added. Need a few more tests, and there are still some parts of this that I don't love and that I want to think about better ways of doing, so please hang on another day or so. (Namely: (1) The global registry still has to be used. (2) The way aggregators are looked up is sort of ugly.)

SimenB · 2017-08-07T07:58:05Z

README.md

@@ -278,6 +288,10 @@ If you need to get a reference to a previously registered metric, you can use `r
 You can remove all metrics by calling `register.clear()`. You can also remove a single metric by calling
 `register.removeSingleMetric(*name of metric*)`.

+##### Cluster metrics
+
+You can get aggregated metrics for all worker processes in a cluster by running `register.clusterMetrics((err, metrics) => {...})`, which will be called with an error, or with the aggregated metrics string for prometheus to consume.


@siimon do we want callback or promise api?

Since cluster api is very much callback based, keeping the cb might be natural

Easy enough to do both: return a promise and accept a callback.

Sorry, haven't had the time to look at this in detail. Having callbacks is a must have imo, but I'm fine with both aswell.

I prefer Promise these days because of async-await, which is in node 8 which will be LTS in a couple of months. Makes for cleaner code.

That said, I don't feel strongly about it

SimenB · 2017-08-07T07:59:50Z

lib/cluster.js

+	}
+}
+
+module.exports = AggregatorRegistry;


Can you move the export to the bottom?

SimenB · 2017-08-07T08:02:15Z

lib/metricAggregators.js

+			if (values[0].metricName) {
+				valObj.metricName = values[0].metricName;
+			}
+			// TODO how do we aggregate timestamps? Average? Omit?


either keep the max or omit, I think. I'm not sure what makes the most sense...

Yeah not sure either, I'm more leaning towards omitting, since I rather provide no data at all than misguiding data

siimon · 2017-08-08T06:43:09Z

lib/histogram.js

@@ -64,6 +65,7 @@ class Histogram {

 		this.name = config.name;
 		this.help = config.help;
+		this.aggregate = config.aggregator || AGGREGATORS.SUM;


Couldn't you set this in the object assign step instead? Will be easier to see all the defaults if they are set on the same place

Originally I had it there, but then it has to be repeated where the 2nd constructor form (formal arguments) is handled:

if (isObject(name)) { // once here } else { config = { // again here } }

siimon · 2017-08-08T06:45:22Z

lib/metricAggregators.js

+/**
+ * Functions that can be used to aggregate metrics from multiple registries.
+ */
+exports.AGGREGATORS = {


Im thinking these might belong in a separate file? Not sure about the uppercase function names either

I think they fit fine in here. I also find uppercase odd, though 🙂

AGGREGATORS.sum or AGGREGATORS.Sum or aggregators.sum?

SimenB

This is starting to look really good! 🙂

SimenB · 2017-08-08T14:25:44Z

.eslintrc

@@ -55,6 +56,12 @@
 		"no-shadow": "off",
 		"no-unused-expressions": "off"
 	  }
+	},
+	{
+		"files": ["example/**/*.js"],


SimenB · 2017-08-08T14:27:14Z

README.md

@@ -278,6 +288,10 @@ If you need to get a reference to a previously registered metric, you can use `r
 You can remove all metrics by calling `register.clear()`. You can also remove a single metric by calling
 `register.removeSingleMetric(*name of metric*)`.

+##### Cluster metrics
+
+You can get aggregated metrics for all worker processes in a cluster by running `register.clusterMetrics((err, metrics) => {...})`, which will be called with an error, or with the aggregated metrics string for prometheus to consume.


I prefer Promise these days because of async-await, which is in node 8 which will be LTS in a couple of months. Makes for cleaner code.

That said, I don't feel strongly about it

SimenB · 2017-08-08T14:30:22Z

lib/cluster.js

+const DEFAULT_METRIC_AGGREGATORS = {
+	nodejs_version_info: AGGREGATORS.FIRST,
+	process_start_time_seconds: AGGREGATORS.OMIT,
+	nodejs_eventloop_lag_seconds: AGGREGATORS.AVERAGE


feels sorta icky to hard-code these. what about custom metrics? Can you filter out all non-sum metrics by checking the metric's type at aggregation-time?

after doing Registry.globalRegistry.getSingleMetric(metricName); further down, figure out strategy. If type is not available, either do an instanceof or add a type function to the metrics

(Re: first comment) Yeah, this is my highest priority to clean up. These aggregation methods definitely shouldn't be defined twice; right now they're here and in lib/metrics/xxx.js. The factory functions exported from lib/metrics/ don't make it easy to retrieve the metric's aggregate/aggregator value, so I had been playing with calling those factory functions with a mocked registry, but chaos ensued. To be continued...

SimenB · 2017-08-08T14:34:03Z

lib/cluster.js

+const Registry = require('./registry');
+const AGGREGATORS = require('./metricAggregators').AGGREGATORS;
+
+const GET_METRICS_REQ = 'prom-client:getMetricsReq';


Can this be a Symbol instead of a string? Not sure how serialization works here

Not as far as I know, without using Symbol.for(string) (which I don't think is any better than just using the string).

Yeah, just the string is fine then 🙂

SimenB · 2017-08-08T14:35:44Z

lib/metricAggregators.js

+		};
+		// Gather metrics by labels.
+		const byLabels = new Map();
+		metrics.forEach(metric => {


this part seems duplicated from aggregate. Can it be put in utils? (I realize the diff is .name vs .labels)

SimenB · 2017-08-08T14:36:38Z

lib/metricAggregators.js

+/**
+ * Functions that can be used to aggregate metrics from multiple registries.
+ */
+exports.AGGREGATORS = {


I think they fit fine in here. I also find uppercase odd, though 🙂

SimenB · 2017-08-08T14:37:25Z

lib/metrics/eventLoopLag.js

@@ -16,7 +17,8 @@ module.exports = registry => {
 	const gauge = new Gauge({
 		name: NODEJS_EVENTLOOP_LAG,
 		help: 'Lag of event loop in seconds.',
-		registers: registry ? [registry] : undefined
+		registers: registry ? [registry] : undefined,
+		aggregator: AGGREGATORS.AVERAGE


Should the Gauge class itself set this? So all get it for free? Or is there some use-case where it makes sense to not do so?

I suppose there are some special-cases (like nodejs_version_info), but can a sensible default be set?

Everything defaults to sum via code in the Gauge/Histogram/Summary/Counter classes (example), but there are three metrics with non-default methods:

nodejs_version_info uses the first value (all values will be the same)

process_starttime_seconds is omitted (all values will be slightly different -- open to suggestions)

nodejs_eventloop_lag averages

japrescott · 2017-08-14T20:02:32Z

lib/cluster.js

+
+				if (request.failed) return; // Callback already run with Error.
+
+				const registry = Registry.aggregate(request.responses);


this needs to refer to AggregatorRegistry.

japrescott · 2017-08-14T20:04:18Z

example/cluster.js

+	}
+
+	metricsServer.get('/cluster_metrics', (req, res) => {
+		AggregatorRegistry.clusterMetrics((err, metrics) => {


clusterMetrics is not a static method and thus can only be called on an Instance - of which there is none in your example code

D34THWINGS · 2017-08-26T00:15:02Z

Can't wait to see this working, good job guys ! ❤️

zbjornson · 2017-08-26T19:03:59Z

Sorry for the delay folks. Will be pushing an update this weekend.

zbjornson · 2017-08-28T00:10:02Z

lib/cluster.js

+				type: GET_METRICS_RES,
+				requestId: message.requestId,
+				// TODO see if we can support the non-global registry also.
+				metrics: Registry.globalRegistry.getMetricsAsJSON()


This is the last remaining TODO. The worker processes need to know which registry to send metrics from for aggregation, so using the globalRegistry was easy.

Options I can think of:

We send metrics from all registries. This seems like it would create a mess.

We provide a way for users to specify which registry to use. Something like Registry.aggregationSource = new Registry(), then the above line becomes Registry.aggregationSource .getMetricsAsJSON().

I don't know what the use cases are for multiple registries, so I'm not sure what makes sense to do.

We have no way of knowing about every registry

I think we should do this. Unsure about the API, though. Maybe a AggregatorRegistry.setRegistries?

zbjornson · 2017-08-28T00:11:29Z

Alright, I think I've addressed all the comments, including adding a Promise interface in addition to the callback interface. All the ugly code around storing and looking up metric aggregator functions is much nicer now.

Thanks everyone for the continuing feedback!

SimenB

This is really starting to come together! Just missing supporting custom registries (as you've noted, #147 (comment)) and I think it's good to land 🙂

SimenB · 2017-08-28T07:29:55Z

lib/cluster.js

+			});
+		});
+
+		// Aggregate gathered metrics. Default to summation.


is that default needed if all the metric types has a default?

SimenB · 2017-08-28T07:30:36Z

lib/util.js

@@ -89,3 +89,21 @@ exports.printDeprecationCollectDefaultMetricsNumber = timeout => {
 		`prom-client - A number to defaultMetrics is deprecated, please use \`collectDefaultMetrics({ timeout: ${timeout} })\`.`
 	);
 };
+
+class Grouper extends Map {


SimenB · 2017-08-28T07:30:49Z

test/aggregatorsTest.js

@@ -0,0 +1,91 @@
+'use strict';
+
+xdescribe('aggregators', () => {


remove the x

siimon · 2017-08-29T05:14:01Z

I agree, looks really good! Another small detail, update the changelog :)

SimenB

(wrong button lasts time...)

lib/metricAggregators.js

zbjornson · 2017-09-03T03:40:46Z

All remaining bits addressed.

Note that the test output says "Warning: Possible EventEmitter memory leak detected. 11 message listeners added. Use emitter.setMaxListeners() to increase limit". This is not a real leak (doesn't happen outside of jest); see jestjs/jest#4413. If you want I can add a jest config file and a setup file containing emitter.setMaxListeners(0).

Thank you!

SimenB

This LGTM, great PR!

I'd like for @siimon to take a new look as well before merging 🙂

siimon · 2017-09-04T06:01:57Z

I agree! Great work and it's highly appreciated! I'll merge and publish a new minor!

And thanks @SimenB et al for great reviews!

zbjornson · 2017-09-04T15:43:54Z

Thanks everyone!

zbjornson force-pushed the cluster branch 2 times, most recently from 8a9893e to add8f4b Compare August 1, 2017 01:54

SimenB requested changes Aug 3, 2017

View reviewed changes

zbjornson force-pushed the cluster branch from add8f4b to 626ed92 Compare August 7, 2017 05:39

SimenB reviewed Aug 7, 2017

View reviewed changes

siimon reviewed Aug 8, 2017

View reviewed changes

SimenB requested changes Aug 8, 2017

View reviewed changes

japrescott reviewed Aug 14, 2017

View reviewed changes

zbjornson force-pushed the cluster branch from 626ed92 to 001515a Compare August 28, 2017 00:01

zbjornson commented Aug 28, 2017

View reviewed changes

zbjornson force-pushed the cluster branch from 001515a to 4b9218d Compare August 28, 2017 00:21

SimenB approved these changes Aug 28, 2017

View reviewed changes

SimenB requested changes Aug 29, 2017

View reviewed changes

dawidpawlowski reviewed Aug 31, 2017

View reviewed changes

lib/metricAggregators.js Outdated Show resolved Hide resolved

Support aggregating metrics from a cluster

448e671

zbjornson force-pushed the cluster branch from 4b9218d to 448e671 Compare September 3, 2017 03:39

SimenB approved these changes Sep 3, 2017

View reviewed changes

siimon merged commit 4c5b6c7 into siimon:master Sep 4, 2017

zbjornson deleted the cluster branch September 4, 2017 15:43

disjunction mentioned this pull request Aug 23, 2018

Missing support for node cluster jochen-schweizer/express-prom-bundle#16

Closed

mkreidenweis-schulmngr mentioned this pull request Jan 8, 2021

Advanced event loop monitoring not aggregated correctly across cluster #418

Closed


		To solve this, you can aggregate all of the workers' metrics in the master process. See example folder for a sample usage. By default, custom metrics are summed across workers, and default metrics use sensible defaults. To use a different aggregation method, define an `aggregate()` function for the metric's name in the global registry in the master process. (See the `AggregatorFactory` in `lib/cluster.js` for details.)

		Note: You must use the default global registry in cluster mode.


		if (request.failed) return; // Callback already run with Error.

		const registry = Registry.aggregate(request.responses);

		@@ -0,0 +1,91 @@
		'use strict';

		xdescribe('aggregators', () => {

Cluster support #147

Cluster support #147

Conversation

zbjornson commented Jul 30, 2017 • edited Loading

tommiv commented Jul 30, 2017

siimon commented Aug 1, 2017

tommiv commented Aug 1, 2017 • edited Loading

zbjornson commented Aug 1, 2017

tommiv commented Aug 1, 2017 • edited Loading

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbjornson commented Aug 3, 2017

zbjornson commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D34THWINGS commented Aug 26, 2017

zbjornson commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbjornson commented Aug 28, 2017

SimenB left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siimon commented Aug 29, 2017

SimenB left a comment

Choose a reason for hiding this comment

zbjornson commented Sep 3, 2017

SimenB left a comment • edited Loading

Choose a reason for hiding this comment

siimon commented Sep 4, 2017

zbjornson commented Sep 4, 2017

zbjornson commented Jul 30, 2017 •

edited

Loading

tommiv commented Aug 1, 2017 •

edited

Loading

tommiv commented Aug 1, 2017 •

edited

Loading

SimenB left a comment •

edited

Loading

SimenB left a comment •

edited

Loading