From a39a31ab626fc19ed02dc3d39e53ed5360246668 Mon Sep 17 00:00:00 2001 From: dt-rush Date: Sun, 19 Apr 2020 02:45:14 -0400 Subject: [PATCH] remove unnecessary necessity of predeclared, known-at-declaration-time labelset for metrics + improved validation testing Change-type: minor Connects-to: #298 Signed-off-by: dt-rush --- lib/counter.js | 19 +++++++++--- lib/gauge.js | 19 +++++++++--- lib/histogram.js | 19 +++++++++--- lib/metric.js | 5 ++- lib/summary.js | 21 ++++++++++--- test/validationTest.js | 69 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 133 insertions(+), 19 deletions(-) diff --git a/lib/counter.js b/lib/counter.js index b7bcc726..4fbd38f5 100644 --- a/lib/counter.js +++ b/lib/counter.js @@ -44,16 +44,15 @@ class Counter extends Metric { } labels() { - const labels = getLabels(this.labelNames, arguments) || {}; + const labels = getLabels([...this.labelNames], arguments) || {}; const hash = hashObject(labels); - validateLabel(this.labelNames, labels); return { inc: inc.call(this, labels, hash), }; } remove() { - const labels = getLabels(this.labelNames, arguments) || {}; + const labels = getLabels([...this.labelNames], arguments) || {}; return removeLabels.call(this, this.hashMap, labels); } } @@ -61,7 +60,7 @@ class Counter extends Metric { const reset = function () { this.hashMap = {}; - if (this.labelNames.length === 0) { + if (this.labelNames.size === 0) { this.hashMap = setValue({}, 0); } }; @@ -75,8 +74,18 @@ const inc = function (labels, hash) { throw new Error('It is not possible to decrease a counter'); } + // if strictLabelNames, verify that no new labels are added, and if not, + // then simply add any labels appearing for the first time to the set + // of labelnames on this metric + if (this.strictLabelNames) { + validateLabel([...this.labelNames], labels); + } else { + // adding to Set is idempotent, so simply add all + for (const labelName in labels) { + this.labelNames.add(labelName); + } + } labels = labels || {}; - validateLabel(this.labelNames, labels); const incValue = value === null || value === undefined ? 1 : value; diff --git a/lib/gauge.js b/lib/gauge.js index 8eaf19a9..1f4492e4 100644 --- a/lib/gauge.js +++ b/lib/gauge.js @@ -97,7 +97,7 @@ class Gauge extends Metric { } labels() { - const labels = getLabels(this.labelNames, arguments); + const labels = getLabels([...this.labelNames], arguments); return { inc: inc.call(this, labels), dec: dec.call(this, labels), @@ -108,7 +108,7 @@ class Gauge extends Metric { } remove() { - const labels = getLabels(this.labelNames, arguments); + const labels = getLabels([...this.labelNames], arguments); removeLabels.call(this, this.hashMap, labels); } } @@ -157,9 +157,20 @@ function set(labels) { throw new TypeError(`Value is not a valid number: ${util.format(value)}`); } + // if strictLabelNames, verify that no new labels are added, and if not, + // then simply add any labels appearing for the first time to the set + // of labelnames on this metric + if (this.strictLabelNames) { + validateLabel([...this.labelNames], labels); + } else { + // adding to Set is idempotent, so simply add all + for (const labelName in labels) { + this.labelNames.add(labelName); + } + } + labels = labels || {}; - validateLabel(this.labelNames, labels); this.hashMap = setValue(this.hashMap, value, labels); }; } @@ -167,7 +178,7 @@ function set(labels) { function reset() { this.hashMap = {}; - if (this.labelNames.length === 0) { + if (this.labelNames.size === 0) { this.hashMap = setValue({}, 0, {}); } } diff --git a/lib/histogram.js b/lib/histogram.js index 24983c84..d81e3016 100644 --- a/lib/histogram.js +++ b/lib/histogram.js @@ -30,7 +30,7 @@ class Histogram extends Metric { Object.freeze(this.bucketValues); Object.freeze(this.upperBounds); - if (this.labelNames.length === 0) { + if (this.labelNames.size === 0) { this.hashMap = { [hashObject({})]: createBaseValues( {}, @@ -87,7 +87,7 @@ class Histogram extends Metric { } labels() { - const labels = getLabels(this.labelNames, arguments); + const labels = getLabels([...this.labelNames], arguments); return { observe: observe.call(this, labels), startTimer: startTimer.call(this, labels), @@ -95,7 +95,7 @@ class Histogram extends Metric { } remove() { - const labels = getLabels(this.labelNames, arguments); + const labels = getLabels([...this.labelNames], arguments); removeLabels.call(this, this.hashMap, labels); } } @@ -134,7 +134,18 @@ function observe(labels) { return value => { const labelValuePair = convertLabelsAndValues(labels, value); - validateLabel(this.labelNames, labelValuePair.labels); + // if strictLabelNames, verify that no new labels are added, and if not, + // then simply add any labels appearing for the first time to the set + // of labelnames on this metric + if (this.strictLabelNames) { + validateLabel([...this.labelNames], labels); + } else { + // adding to Set is idempotent, so simply add all + for (const labelName in labels) { + this.labelNames.add(labelName); + } + } + if (!Number.isFinite(labelValuePair.value)) { throw new TypeError( `Value is not a valid number: ${util.format(labelValuePair.value)}`, diff --git a/lib/metric.js b/lib/metric.js index 593eee3b..1c41609e 100644 --- a/lib/metric.js +++ b/lib/metric.js @@ -22,6 +22,9 @@ class Metric { defaults, config, ); + // labelNames are passed to constructor as array for user-friendliness, + // but interally, we use them as a Set + this.labelNames = new Set(this.labelNames); if (!this.registers) { // in case config.registers is `undefined` this.registers = [globalRegistry]; @@ -35,7 +38,7 @@ class Metric { if (!validateMetricName(this.name)) { throw new Error('Invalid metric name'); } - if (!validateLabelName(this.labelNames)) { + if (!validateLabelName([...this.labelNames])) { throw new Error('Invalid label name'); } diff --git a/lib/summary.js b/lib/summary.js index c8033472..5681bb8b 100644 --- a/lib/summary.js +++ b/lib/summary.js @@ -6,8 +6,8 @@ const util = require('util'); const type = 'summary'; const { getLabels, hashObject, removeLabels } = require('./util'); -const { validateLabel } = require('./validation'); const { Metric } = require('./metric'); +const { validateLabel } = require('./validation'); const timeWindowQuantiles = require('./timeWindowQuantiles'); const DEFAULT_COMPRESS_COUNT = 1000; // every 1000 measurements @@ -25,7 +25,7 @@ class Summary extends Metric { throw new Error('quantile is a reserved label keyword'); } - if (this.labelNames.length === 0) { + if (this.labelNames.size === 0) { this.hashMap = { [hashObject({})]: { labels: {}, @@ -91,7 +91,7 @@ class Summary extends Metric { } labels() { - const labels = getLabels(this.labelNames, arguments); + const labels = getLabels([...this.labelNames], arguments); return { observe: observe.call(this, labels), startTimer: startTimer.call(this, labels), @@ -99,7 +99,7 @@ class Summary extends Metric { } remove() { - const labels = getLabels(this.labelNames, arguments); + const labels = getLabels([...this.labelNames], arguments); removeLabels.call(this, this.hashMap, labels); } } @@ -149,7 +149,18 @@ function observe(labels) { return value => { const labelValuePair = convertLabelsAndValues(labels, value); - validateLabel(this.labelNames, this.labels); + // if strictLabelNames, verify that no new labels are added, and if not, + // then simply add any labels appearing for the first time to the set + // of labelnames on this metric + if (this.strictLabelNames) { + validateLabel([...this.labelNames], labels); + } else { + // adding to Set is idempotent, so simply add all + for (const labelName in labels) { + this.labelNames.add(labelName); + } + } + if (!Number.isFinite(labelValuePair.value)) { throw new TypeError( `Value is not a valid number: ${util.format(labelValuePair.value)}`, diff --git a/test/validationTest.js b/test/validationTest.js index 437b9c7f..e4aafb3a 100644 --- a/test/validationTest.js +++ b/test/validationTest.js @@ -1,6 +1,33 @@ 'use strict'; describe('validation', () => { + describe('validateMetricName', () => { + const { validateMetricName } = require('../lib/validation'); + + it('should validate a valid metric name', () => { + const validName = + 'instance:node_cpu_used_percent:100x_sum_rate_divideNCPU'; + expect(validateMetricName(validName)).toEqual(true); + }); + + it('should not validate an invalid metric name', () => { + expect(validateMetricName(['a counter'])).toEqual(false); + }); + }); + + describe('validateLabelName', () => { + const { validateLabelName } = require('../lib/validation'); + + it('should validate a valid label name', () => { + const validNames = ['method', 'METHOD', 'net_iface', 'k8s_version']; + expect(validateLabelName(validNames)).toEqual(true); + }); + + it('should not validate an invalid label name', () => { + expect(validateLabelName(['/etc/issue'])).toEqual(false); + }); + }); + describe('validateLabel', () => { const validateLabel = require('../lib/validation').validateLabel; @@ -18,4 +45,46 @@ describe('validation', () => { ); }); }); + + describe('strictLabelNames', () => { + const Counter = require('../lib/counter'); + const Gauge = require('../lib/gauge'); + + it('should not throw on unknown label by default', () => { + const c = new Counter({ + name: 'api_complete_requests_total', + help: 'number of requests completed as a counter', + labelNames: ['method', 'status_code'], + }); + expect(() => { + c.inc({ + method: 'PATCH', + status_code: '409', + path: '/device/v2/:uuid/state', + queue: 'state_patch', + }); + }).not.toThrowError(); + }); + + it('should throw on unknown label if strictLabelNames: true', () => { + const g = new Gauge({ + name: 'api_complete_requests_inflight', + help: 'number of requests currently being processed', + labelNames: ['method', 'path'], + strictLabelNames: true, + }); + expect(() => { + g.set( + { + method: 'GET', + path: '/device/v2/:uuid/state', + queue: 'state_patch', + }, + 550, + ); + }).toThrowError( + "Added label \"queue\" is not included in initial labelset: [ 'method', 'path' ]", + ); + }); + }); });