From 2953a290ff6d9ef1a9d2730f4c991f16e560cb30 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 31 Oct 2019 17:23:47 -0400 Subject: [PATCH] feat: validate metric names (#468) * 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 --- packages/opentelemetry-metrics/src/Meter.ts | 51 +++++++- .../opentelemetry-metrics/test/Meter.test.ts | 115 +++++++++++++++--- 2 files changed, 141 insertions(+), 25 deletions(-) diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index a882b5e048..9d889530af 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -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, @@ -46,7 +50,13 @@ export class Meter implements types.Meter { createMeasure( name: string, options?: types.MetricOptions - ): Metric { + ): types.Metric { + 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'); } @@ -61,7 +71,13 @@ export class Meter implements types.Meter { createCounter( name: string, options?: types.MetricOptions - ): Metric { + ): types.Metric { + 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, @@ -83,7 +99,13 @@ export class Meter implements types.Meter { createGauge( name: string, options?: types.MetricOptions - ): Metric { + ): types.Metric { + 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, @@ -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)); + } } diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 5567cc2338..e227cc5103 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -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; @@ -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); @@ -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), { @@ -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); @@ -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); @@ -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); @@ -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); @@ -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); }); @@ -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); @@ -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), { @@ -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); @@ -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); @@ -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); @@ -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); @@ -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); + }); + }); }); });