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

feat: enhance error output on not valid name and label names #613

Open
wants to merge 6 commits 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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ project adheres to [Semantic Versioning](http://semver.org/).

### Changed

- Show the invalid name in the output error in the abstract class `Metric`.
- Show the invalid label names instead of a generic message in the abstract class `Metric`.
- Return an array of the invalid label names in the function `validateLabelName` instead of a boolean. It was achieved changing the implementation from the array function `every` to `forEach` and negating the test regex in favor to return the rejected ones.
- Change the conditional`if` to throw an error in case the label names are invalid using the array returned from the function `validateLabelName`.
- Unit test added in the gauge test case. It could be performed in any other metric that inherits from the abstract class `Metric`.
- Improve the memory usage of histograms when the `enableExemplars` option is disabled

### Added
Expand Down
9 changes: 6 additions & 3 deletions lib/metric.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@ class Metric {
throw new Error('Missing mandatory name parameter');
}
if (!validateMetricName(this.name)) {
throw new Error('Invalid metric name');
throw new Error(`Invalid metric name: ${this.name}`);
}
if (!validateLabelName(this.labelNames)) {
throw new Error('Invalid label name');
const invalidLabelNames = validateLabelName(this.labelNames);
if (invalidLabelNames.length > 0) {
throw new Error(
`At least one label name is invalid: ${invalidLabelNames.join(',')}`,
);
}

if (this.collect && typeof this.collect !== 'function') {
Expand Down
2 changes: 1 addition & 1 deletion lib/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ exports.validateMetricName = function (name) {
};

exports.validateLabelName = function (names = []) {
return names.every(name => labelRegexp.test(name));
return names.filter(name => !labelRegexp.test(name));
};

exports.validateLabel = function validateLabel(savedLabels, labels) {
Expand Down
39 changes: 39 additions & 0 deletions test/gaugeTest.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const { Metric } = require('../lib/metric');
const Registry = require('../index').Registry;

describe.each([
Expand All @@ -19,6 +20,44 @@ describe.each([
globalRegistry.clear();
});

describe('Metric instantiation', () => {
const defaultParams = { name: 'gauge_test', help: 'help' };

describe('happy path', () => {
it('should create a instance', async () => {
const instance = new Gauge(defaultParams);
const instanceValues = await instance.get();
expect(instance).toBeInstanceOf(Metric);
expect(instance).toBeInstanceOf(Gauge);
expect(instance.labelNames).toStrictEqual([]);
expect(instanceValues.name).toStrictEqual(defaultParams.name);
expect(instanceValues.help).toStrictEqual(defaultParams.help);
});
});

describe('un-happy path', () => {
const noValidName = 'no valid name';
it('should thrown an error due invalid metric name', () => {
expect(
() => new Gauge({ ...defaultParams, name: noValidName }),
).toThrow(new Error(`Invalid metric name: ${noValidName}`));
});

it('should thrown an error due some invalid label name', () => {
const noValidLabelNames = [noValidName, defaultParams.name];
expect(
() =>
new Gauge({
...defaultParams,
labelNames: noValidLabelNames,
}),
).toThrow(
new Error(`At least one label name is invalid: ${noValidName}`),
);
});
});
});

describe('with parameters as object', () => {
beforeEach(() => {
instance = new Gauge({ name: 'gauge_test', help: 'help' });
Expand Down