Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove unnecessary necessity of predeclared, known-at-declaration-time labelset for metrics #368

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions lib/counter.js
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
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
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
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
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
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' ]",
);
});
});
});