From e496b4437ed85fea50eb3dd4422cf73545ce6a5f Mon Sep 17 00:00:00 2001 From: raghu Date: Thu, 18 Feb 2021 08:49:06 +0530 Subject: [PATCH] fix: always use sum aggregator for sum & count sum & count metrics of summary & histogram should always be summed irrespective of the custom aggregator --- CHANGELOG.md | 2 + lib/metricAggregators.js | 93 +++++++++++++++++++++++++++++----------- test/clusterTest.js | 70 ++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8adcb1c6..9d79cf9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ project adheres to [Semantic Versioning](http://semver.org/). ### Changed +- fix: always using sum aggregator for sum & count in histogram & summary + ### Added - feat: added `zero()` to `Histogram` for setting the metrics for a given label combination to zero diff --git a/lib/metricAggregators.js b/lib/metricAggregators.js index c010d467..2f969581 100644 --- a/lib/metricAggregators.js +++ b/lib/metricAggregators.js @@ -2,6 +2,56 @@ const { Grouper, hashObject } = require('./util'); +/** + * Returns the sum of all label values passed + * @param {Object} v Label values + * @return {Number} sum of values + */ +function sum(v) { + return v.reduce((p, c) => p + c.value, 0); +} + +/** + * Returns the first label value + * @param {Object} v Label values + * @return {Number} first value. + */ +function first(v) { + return v[0].value; +} + +/** + * @return {undefined} Undefined; omits the values. + */ +function omit() {} + +/** + * Returns the mean of all label values + * @param {Object} v Label values + * @return {Number} mean of values. + */ +function average(v) { + return v.reduce((p, c) => p + c.value, 0) / v.length; +} + +/** + * Returns the minimum of all label values + * @param {Object} v Label values + * @return {Number} min of values. + */ +function min(v) { + return v.reduce((p, c) => Math.min(p, c.value), Infinity); +} + +/** + * Returns the maximum of all label values + * @param {Object} v Label values + * @return {Number} max of values. + */ +function max(v) { + return v.reduce((p, c) => Math.max(p, c.value), -Infinity); +} + /** * Returns a new function that applies the `aggregatorFn` to the values. * @param {Function} aggregatorFn function to apply to values. @@ -29,12 +79,20 @@ function AggregatorFactory(aggregatorFn) { byLabels.forEach(values => { if (values.length === 0) return; const valObj = { - value: aggregatorFn(values), labels: values[0].labels, }; if (values[0].metricName) { valObj.metricName = values[0].metricName; } + // Should always `sum` if it's `sum` or `count` that's part of histogram & summary + if ( + values[0].metricName === `${result.name}_sum` || + values[0].metricName === `${result.name}_count` + ) { + valObj.value = sum(values); + } else { + valObj.value = aggregatorFn(values); + } // NB: Timestamps are omitted. result.values.push(valObj); }); @@ -48,34 +106,19 @@ exports.AggregatorFactory = AggregatorFactory; * Functions that can be used to aggregate metrics from multiple registries. */ exports.aggregators = { - /** - * @return The sum of values. - */ - sum: AggregatorFactory(v => v.reduce((p, c) => p + c.value, 0)), - /** - * @return The first value. - */ - first: AggregatorFactory(v => v[0].value), - /** - * @return {undefined} Undefined; omits the metric. - */ - omit: () => {}, - /** - * @return The arithmetic mean of the values. - */ - average: AggregatorFactory( - v => v.reduce((p, c) => p + c.value, 0) / v.length, - ), + sum: AggregatorFactory(sum), + + first: AggregatorFactory(first), + + omit, + + average: AggregatorFactory(average), /** * @return The minimum of the values. */ - min: AggregatorFactory(v => - v.reduce((p, c) => Math.min(p, c.value), Infinity), - ), + min: AggregatorFactory(min), /** * @return The maximum of the values. */ - max: AggregatorFactory(v => - v.reduce((p, c) => Math.max(p, c.value), -Infinity), - ), + max: AggregatorFactory(max), }; diff --git a/test/clusterTest.js b/test/clusterTest.js index 9d7ad0d9..964bd9c5 100644 --- a/test/clusterTest.js +++ b/test/clusterTest.js @@ -30,6 +30,28 @@ describe('AggregatorRegistry', () => { const Registry = require('../lib/cluster'); // These mimic the output of `getMetricsAsJSON`. const metricsArr1 = [ + { + name: 'test_summary', + help: 'Example of a summary', + type: 'summary', + values: [ + { + labels: { quantile: 0.5 }, + value: 4, + }, + { + metricName: 'test_summary_sum', + labels: {}, + value: 10, + }, + { + metricName: 'test_summary_count', + labels: {}, + value: 3, + }, + ], + aggregator: 'average', + }, { name: 'test_histogram', help: 'Example of a histogram', @@ -87,6 +109,28 @@ describe('AggregatorRegistry', () => { }, ]; const metricsArr2 = [ + { + name: 'test_summary', + help: 'Example of a summary', + type: 'summary', + values: [ + { + labels: { quantile: 0.5 }, + value: 6, + }, + { + metricName: 'test_summary_sum', + labels: {}, + value: 20, + }, + { + metricName: 'test_summary_count', + labels: {}, + value: 2, + }, + ], + aggregator: 'average', + }, { name: 'test_histogram', help: 'Example of a histogram', @@ -231,5 +275,31 @@ describe('AggregatorRegistry', () => { aggregator: 'first', }); }); + + it('uses `aggregate` method defined for test_summary and summation for `count` & `sum`', async () => { + const summary = aggregated.getSingleMetric('test_summary').get(); + expect(summary).toEqual({ + name: 'test_summary', + help: 'Example of a summary', + type: 'summary', + values: [ + { + labels: { quantile: 0.5 }, + value: 5, + }, + { + labels: {}, + value: 30, + metricName: 'test_summary_sum', + }, + { + labels: {}, + value: 5, + metricName: 'test_summary_count', + }, + ], + aggregator: 'average', + }); + }); }); });