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

Author: dyladan

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<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');
   }
@@ -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,
@@ -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,
@@ -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));
+  }
 }

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);
+      });
+    });
   });
 });