Skip to content

Commit

Permalink
remove unnecessary necessity of predeclared, known-at-declaration-tim…
Browse files Browse the repository at this point in the history 
…e labelset for metrics

+ improved validation testing

Change-type: minor
Connects-to: siimon#298
Signed-off-by: dt-rush <[email protected]>
  • Loading branch information
@dt-rush
dt-rush committed Apr 19, 2020
1 parent 6151462 commit a39a31a
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 19 deletions.
19 changes: 14 additions & 5 deletions lib/counter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,24 +44,23 @@ 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);
}
}

const reset = function () {
this.hashMap = {};

if (this.labelNames.length === 0) {
if (this.labelNames.size === 0) {
this.hashMap = setValue({}, 0);
}
};
Expand All @@ -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;

Expand Down
19 changes: 15 additions & 4 deletions lib/gauge.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -157,17 +157,28 @@ 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);
};
}

function reset() {
this.hashMap = {};

if (this.labelNames.length === 0) {
if (this.labelNames.size === 0) {
this.hashMap = setValue({}, 0, {});
}
}
Expand Down
19 changes: 15 additions & 4 deletions lib/histogram.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
{},
Expand Down Expand Up @@ -87,15 +87,15 @@ 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),
};
}

remove() {
const labels = getLabels(this.labelNames, arguments);
const labels = getLabels([...this.labelNames], arguments);
removeLabels.call(this, this.hashMap, labels);
}
}
Expand Down Expand Up @@ -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)}`,
Expand Down
5 changes: 4 additions & 1 deletion lib/metric.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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');
}

Expand Down
21 changes: 16 additions & 5 deletions lib/summary.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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: {},
Expand Down Expand Up @@ -91,15 +91,15 @@ 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),
};
}

remove() {
const labels = getLabels(this.labelNames, arguments);
const labels = getLabels([...this.labelNames], arguments);
removeLabels.call(this, this.hashMap, labels);
}
}
Expand Down Expand Up @@ -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)}`,
Expand Down
69 changes: 69 additions & 0 deletions test/validationTest.js
View file
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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' ]",
);
});
});
});

0 comments on commit a39a31a

Please sign in to comment.