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.

Sign up for GitHub

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

Jump to bottom

feat: validate metric names #468

Merged
merged 6 commits into from
Oct 31, 2019
Merged

feat: validate metric names #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
61d5f14
feat: validate metric names
dyladan Oct 30, 2019
a8620b5
fix: log invalid metric names
dyladan Oct 30, 2019
011a557
test: fix compilation errors in tests
dyladan Oct 30, 2019
a1dfe5b
style: fix linting issues
dyladan Oct 30, 2019
2e7e7e1
Merge remote-tracking branch 'upstream/master' into validate-metric-n…
dyladan Oct 30, 2019
28f228a
test: add name tests to measure metric
dyladan Oct 30, 2019
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
51 changes: 45 additions & 6 deletions packages/opentelemetry-metrics/src/Meter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@
*/

import * as types from '@opentelemetry/types';
import { ConsoleLogger } from '@opentelemetry/core';
import { CounterHandle, GaugeHandle, MeasureHandle } from './Handle';
import { Metric, CounterMetric, GaugeMetric } from './Metric';
import {
ConsoleLogger,
NOOP_COUNTER_METRIC,
NOOP_GAUGE_METRIC,
NOOP_MEASURE_METRIC,
} from '@opentelemetry/core';
import { CounterMetric, GaugeMetric } from './Metric';
import {
MetricOptions,
DEFAULT_METRIC_OPTIONS,
Expand Down Expand Up @@ -46,7 +50,13 @@ export class Meter implements types.Meter {
createMeasure(
name: string,
options?: types.MetricOptions
): Metric<MeasureHandle> {
): types.Metric<types.MeasureHandle> {
if (!this._isValidName(name)) {
this._logger.warn(
`Invalid metric name ${name}. Defaulting to noop metric implementation.`
);
return NOOP_MEASURE_METRIC;
dyladan marked this conversation as resolved.
Show resolved Hide resolved
}
// @todo: implement this method
throw new Error('not implemented yet');
}
Expand All @@ -61,7 +71,13 @@ export class Meter implements types.Meter {
createCounter(
name: string,
options?: types.MetricOptions
): Metric<CounterHandle> {
): types.Metric<types.CounterHandle> {
if (!this._isValidName(name)) {
this._logger.warn(
`Invalid metric name ${name}. Defaulting to noop metric implementation.`
);
return NOOP_COUNTER_METRIC;
}
const opt: MetricOptions = {
// Counters are defined as monotonic by default
monotonic: true,
Expand All @@ -83,7 +99,13 @@ export class Meter implements types.Meter {
createGauge(
name: string,
options?: types.MetricOptions
): Metric<GaugeHandle> {
): types.Metric<types.GaugeHandle> {
if (!this._isValidName(name)) {
this._logger.warn(
`Invalid metric name ${name}. Defaulting to noop metric implementation.`
);
return NOOP_GAUGE_METRIC;
}
const opt: MetricOptions = {
// Gauges are defined as non-monotonic by default
monotonic: false,
Expand All @@ -93,4 +115,21 @@ export class Meter implements types.Meter {
};
return new GaugeMetric(name, opt);
}

/**
* Ensure a metric name conforms to the following rules:
*
* 1. They are non-empty strings
*
* 2. The first character must be non-numeric, non-space, non-punctuation
*
* 3. Subsequent characters must be belong to the alphanumeric characters, '_', '.', and '-'.
*
* Names are case insensitive
*
* @param name Name of metric to be created
*/
private _isValidName(name: string): boolean {
return Boolean(name.match(/^[a-z][a-z0-9_.\-]*$/i));
}
}
115 changes: 96 additions & 19 deletions packages/opentelemetry-metrics/test/Meter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
*/

import * as assert from 'assert';
import { Meter, Metric } from '../src';
import { Meter, Metric, CounterMetric, GaugeMetric } from '../src';
import * as types from '@opentelemetry/types';
import { NoopLogger } from '@opentelemetry/core';
import { NoopLogger, NoopMetric } from '@opentelemetry/core';

describe('Meter', () => {
let meter: Meter;
Expand Down Expand Up @@ -48,7 +48,7 @@ describe('Meter', () => {

describe('.getHandle()', () => {
it('should create a counter handle', () => {
const counter = meter.createCounter('name');
const counter = meter.createCounter('name') as CounterMetric;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why do you need to use as - Type assertion here? Earlier it used to work, Is it because we are returning noop implementation?

Copy link
Member Author

@dyladan dyladan Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the function createCounter now returns types.Metric<types.CounterHandle> where it used to return just CounterMetric. The return type needed to be changed in order to allow returning the noop implementation which satisfies the type types.Metric<types.CounterHandle> but is not a CounterMetric

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got you. As a user, I have to use/do the same assertion? This looks little inconvenient. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you don't, because users will only use the public interface. This is only done because the tests access private fields

const handle = counter.getHandle(labelValues);
handle.add(10);
assert.strictEqual(handle['_data'], 10);
Expand All @@ -57,7 +57,7 @@ describe('Meter', () => {
});

it('should return the timeseries', () => {
const counter = meter.createCounter('name');
const counter = meter.createCounter('name') as CounterMetric;
const handle = counter.getHandle(['value1', 'value2']);
handle.add(20);
assert.deepStrictEqual(handle.getTimeSeries(hrTime), {
Expand All @@ -67,7 +67,7 @@ describe('Meter', () => {
});

it('should add positive values by default', () => {
const counter = meter.createCounter('name');
const counter = meter.createCounter('name') as CounterMetric;
const handle = counter.getHandle(labelValues);
handle.add(10);
assert.strictEqual(handle['_data'], 10);
Expand All @@ -78,7 +78,7 @@ describe('Meter', () => {
it('should not add the handle data when disabled', () => {
const counter = meter.createCounter('name', {
disabled: true,
});
}) as CounterMetric;
const handle = counter.getHandle(labelValues);
handle.add(10);
assert.strictEqual(handle['_data'], 0);
Expand All @@ -87,14 +87,14 @@ describe('Meter', () => {
it('should add negative value when monotonic is set to false', () => {
const counter = meter.createCounter('name', {
monotonic: false,
});
}) as CounterMetric;
const handle = counter.getHandle(labelValues);
handle.add(-10);
assert.strictEqual(handle['_data'], -10);
});

it('should return same handle on same label values', () => {
const counter = meter.createCounter('name');
const counter = meter.createCounter('name') as CounterMetric;
const handle = counter.getHandle(labelValues);
handle.add(10);
const handle1 = counter.getHandle(labelValues);
Expand All @@ -106,7 +106,7 @@ describe('Meter', () => {

describe('.removeHandle()', () => {
it('should remove a counter handle', () => {
const counter = meter.createCounter('name');
const counter = meter.createCounter('name') as CounterMetric;
const handle = counter.getHandle(labelValues);
assert.strictEqual(counter['_handles'].size, 1);
counter.removeHandle(labelValues);
Expand All @@ -122,18 +122,46 @@ describe('Meter', () => {
});

it('should clear all handles', () => {
const counter = meter.createCounter('name');
const counter = meter.createCounter('name') as CounterMetric;
counter.getHandle(labelValues);
assert.strictEqual(counter['_handles'].size, 1);
counter.clear();
assert.strictEqual(counter['_handles'].size, 0);
});
});

describe('names', () => {
it('should create counter with valid names', () => {
const counter1 = meter.createCounter('name1');
const counter2 = meter.createCounter(
'Name_with-all.valid_CharacterClasses'
);
assert.ok(counter1 instanceof CounterMetric);
assert.ok(counter2 instanceof CounterMetric);
});

it('should return no op metric if name is an empty string', () => {
const counter = meter.createCounter('');
assert.ok(counter instanceof NoopMetric);
});

it('should return no op metric if name does not start with a letter', () => {
const counter1 = meter.createCounter('1name');
const counter_ = meter.createCounter('_name');
assert.ok(counter1 instanceof NoopMetric);
assert.ok(counter_ instanceof NoopMetric);
});

it('should return no op metric if name is an empty string contain only letters, numbers, ".", "_", and "-"', () => {
const counter = meter.createCounter('name with invalid characters^&*(');
assert.ok(counter instanceof NoopMetric);
});
});
});

describe('#gauge', () => {
it('should create a gauge', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
assert.ok(gauge instanceof Metric);
});

Expand All @@ -149,7 +177,7 @@ describe('Meter', () => {

describe('.getHandle()', () => {
it('should create a gauge handle', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
const handle = gauge.getHandle(labelValues);
handle.set(10);
assert.strictEqual(handle['_data'], 10);
Expand All @@ -158,7 +186,7 @@ describe('Meter', () => {
});

it('should return the timeseries', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
const handle = gauge.getHandle(['v1', 'v2']);
handle.set(150);
assert.deepStrictEqual(handle.getTimeSeries(hrTime), {
Expand All @@ -168,7 +196,7 @@ describe('Meter', () => {
});

it('should go up and down by default', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
const handle = gauge.getHandle(labelValues);
handle.set(10);
assert.strictEqual(handle['_data'], 10);
Expand All @@ -179,7 +207,7 @@ describe('Meter', () => {
it('should not set the handle data when disabled', () => {
const gauge = meter.createGauge('name', {
disabled: true,
});
}) as GaugeMetric;
const handle = gauge.getHandle(labelValues);
handle.set(10);
assert.strictEqual(handle['_data'], 0);
Expand All @@ -188,14 +216,14 @@ describe('Meter', () => {
it('should not set negative value when monotonic is set to true', () => {
const gauge = meter.createGauge('name', {
monotonic: true,
});
}) as GaugeMetric;
const handle = gauge.getHandle(labelValues);
handle.set(-10);
assert.strictEqual(handle['_data'], 0);
});

it('should return same handle on same label values', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
const handle = gauge.getHandle(labelValues);
handle.set(10);
const handle1 = gauge.getHandle(labelValues);
Expand All @@ -207,7 +235,7 @@ describe('Meter', () => {

describe('.removeHandle()', () => {
it('should remove the gauge handle', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
const handle = gauge.getHandle(labelValues);
assert.strictEqual(gauge['_handles'].size, 1);
gauge.removeHandle(labelValues);
Expand All @@ -223,12 +251,61 @@ describe('Meter', () => {
});

it('should clear all handles', () => {
const gauge = meter.createGauge('name');
const gauge = meter.createGauge('name') as GaugeMetric;
gauge.getHandle(labelValues);
assert.strictEqual(gauge['_handles'].size, 1);
gauge.clear();
assert.strictEqual(gauge['_handles'].size, 0);
});
});

describe('names', () => {
it('should create gauges with valid names', () => {
const gauge1 = meter.createGauge('name1');
const gauge2 = meter.createGauge(
'Name_with-all.valid_CharacterClasses'
);
assert.ok(gauge1 instanceof GaugeMetric);
assert.ok(gauge2 instanceof GaugeMetric);
});

it('should return no op metric if name is an empty string', () => {
const gauge = meter.createGauge('');
assert.ok(gauge instanceof NoopMetric);
});

it('should return no op metric if name does not start with a letter', () => {
const gauge1 = meter.createGauge('1name');
const gauge_ = meter.createGauge('_name');
assert.ok(gauge1 instanceof NoopMetric);
assert.ok(gauge_ instanceof NoopMetric);
});

it('should return no op metric if name is an empty string contain only letters, numbers, ".", "_", and "-"', () => {
const gauge = meter.createGauge('name with invalid characters^&*(');
assert.ok(gauge instanceof NoopMetric);
});
});
});

describe('#measure', () => {
describe('names', () => {
it('should return no op metric if name is an empty string', () => {
const gauge = meter.createMeasure('');
assert.ok(gauge instanceof NoopMetric);
});

it('should return no op metric if name does not start with a letter', () => {
const gauge1 = meter.createMeasure('1name');
const gauge_ = meter.createMeasure('_name');
assert.ok(gauge1 instanceof NoopMetric);
assert.ok(gauge_ instanceof NoopMetric);
});

it('should return no op metric if name is an empty string contain only letters, numbers, ".", "_", and "-"', () => {
const gauge = meter.createMeasure('name with invalid characters^&*(');
assert.ok(gauge instanceof NoopMetric);
});
});
});
});