Skip to content

Commit

Permalink
feat: validate metric names (#468)
Browse files Browse the repository at this point in the history 
* feat: validate metric names

* fix: log invalid metric names

* test: fix compilation errors in tests

* style: fix linting issues

* test: add name tests to measure metric
  • Loading branch information
@dyladan @mayurkale22
dyladan authored and mayurkale22 committed Oct 31, 2019
1 parent 5c49c6c commit 2953a29
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 25 deletions.
51 changes: 45 additions & 6 deletions packages/opentelemetry-metrics/src/Meter.ts
View file
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;
}
// @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
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;
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);
});
});
});
});

0 comments on commit 2953a29

Please sign in to comment.