fix(sdk-metrics): metric names should be case-insensitive (#4059)

legendecas · web-flow · commit 4d662cf84d75 · 2023-09-07T08:41:06.000+03:00
* fix(sdk-metrics): metric names should be case-insensitive

* fixup!

* fixup!
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
 ### :bug: (Bug Fix)
 
 * fix(exporter-zipkin): rounding duration to the nearest int to be compliant with zipkin protocol [#4064](https://github.com/open-telemetry/opentelemetry-js/pull/4064) @n0cloud
+* fix(sdk-metrics): metric names should be case-insensitive
 
 ### :books: (Refine Doc)
 
diff --git a/packages/sdk-metrics/src/InstrumentDescriptor.ts b/packages/sdk-metrics/src/InstrumentDescriptor.ts
@@ -14,8 +14,9 @@
  * limitations under the License.
  */
 
-import { MetricOptions, ValueType } from '@opentelemetry/api';
+import { MetricOptions, ValueType, diag } from '@opentelemetry/api';
 import { View } from './view/View';
+import { equalsCaseInsensitive } from './utils';
 
 /**
  * Supported types of metric instruments.
@@ -45,6 +46,11 @@ export function createInstrumentDescriptor(
   type: InstrumentType,
   options?: MetricOptions
 ): InstrumentDescriptor {
+  if (!isValidName(name)) {
+    diag.warn(
+      `Invalid metric name: "${name}". The metric name should be a ASCII string with a length no greater than 255 characters.`
+    );
+  }
   return {
     name,
     type,
@@ -71,10 +77,18 @@ export function isDescriptorCompatibleWith(
   descriptor: InstrumentDescriptor,
   otherDescriptor: InstrumentDescriptor
 ) {
+  // Names are case-insensitive strings.
   return (
-    descriptor.name === otherDescriptor.name &&
+    equalsCaseInsensitive(descriptor.name, otherDescriptor.name) &&
     descriptor.unit === otherDescriptor.unit &&
     descriptor.type === otherDescriptor.type &&
     descriptor.valueType === otherDescriptor.valueType
   );
 }
+
+// ASCII string with a length no greater than 255 characters.
+// NB: the first character counted separately from the rest.
+const NAME_REGEXP = /^[a-z][a-z0-9_.-]{0,254}$/i;
+export function isValidName(name: string): boolean {
+  return name.match(NAME_REGEXP) != null;
+}
diff --git a/packages/sdk-metrics/src/utils.ts b/packages/sdk-metrics/src/utils.ts
@@ -190,3 +190,7 @@ export function binarySearchLB(arr: number[], value: number): number {
   }
   return -1;
 }
+
+export function equalsCaseInsensitive(lhs: string, rhs: string): boolean {
+  return lhs.toLowerCase() === rhs.toLowerCase();
+}
diff --git a/packages/sdk-metrics/test/InstrumentDescriptor.test.ts b/packages/sdk-metrics/test/InstrumentDescriptor.test.ts
@@ -17,8 +17,13 @@
 import * as assert from 'assert';
 import {
   createInstrumentDescriptor,
+  InstrumentDescriptor,
   InstrumentType,
+  isValidName,
+  isDescriptorCompatibleWith,
 } from '../src/InstrumentDescriptor';
+import { invalidNames, validNames } from './util';
+import { ValueType } from '@opentelemetry/api';
 
 describe('InstrumentDescriptor', () => {
   describe('createInstrumentDescriptor', () => {
@@ -35,4 +40,105 @@ describe('InstrumentDescriptor', () => {
       });
     }
   });
+
+  describe('isDescriptorCompatibleWith', () => {
+    const tests: [
+      string,
+      boolean,
+      InstrumentDescriptor,
+      InstrumentDescriptor,
+    ][] = [
+      [
+        'Compatible with identical descriptors',
+        true,
+        {
+          name: 'foo',
+          description: 'any descriptions',
+          unit: 'kB',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+        {
+          name: 'foo',
+          description: 'any descriptions',
+          unit: 'kB',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+      ],
+      [
+        'Compatible with case-insensitive names',
+        true,
+        {
+          name: 'foo',
+          description: '',
+          unit: '',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+        {
+          name: 'FoO',
+          description: '',
+          unit: '',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+      ],
+      [
+        'Incompatible with different names',
+        false,
+        {
+          name: 'foo',
+          description: '',
+          unit: '',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+        {
+          name: 'foobar',
+          description: '',
+          unit: '',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+      ],
+      [
+        'Incompatible with case-sensitive units',
+        false,
+        {
+          name: 'foo',
+          description: '',
+          unit: 'kB',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+        {
+          name: 'foo',
+          description: '',
+          unit: 'kb',
+          type: InstrumentType.COUNTER,
+          valueType: ValueType.DOUBLE,
+        },
+      ],
+    ];
+    for (const test of tests) {
+      it(test[0], () => {
+        assert.ok(isDescriptorCompatibleWith(test[2], test[3]) === test[1]);
+      });
+    }
+  });
+
+  describe('isValidName', () => {
+    it('should validate names', () => {
+      for (const invalidName of invalidNames) {
+        assert.ok(
+          !isValidName(invalidName),
+          `${invalidName} should be invalid`
+        );
+      }
+      for (const validName of validNames) {
+        assert.ok(isValidName(validName), `${validName} should be valid`);
+      }
+    });
+  });
 });
diff --git a/packages/sdk-metrics/test/Meter.test.ts b/packages/sdk-metrics/test/Meter.test.ts
@@ -14,8 +14,9 @@
  * limitations under the License.
  */
 
-import { Observable } from '@opentelemetry/api';
+import { Observable, diag } from '@opentelemetry/api';
 import * as assert from 'assert';
+import * as sinon from 'sinon';
 import {
   CounterInstrument,
   HistogramInstrument,
@@ -27,78 +28,86 @@ import {
 import { Meter } from '../src/Meter';
 import { MeterProviderSharedState } from '../src/state/MeterProviderSharedState';
 import { MeterSharedState } from '../src/state/MeterSharedState';
-import { defaultInstrumentationScope, defaultResource } from './util';
+import {
+  defaultInstrumentationScope,
+  defaultResource,
+  invalidNames,
+  validNames,
+} from './util';
 
 describe('Meter', () => {
+  afterEach(() => {
+    sinon.restore();
+  });
+
   describe('createCounter', () => {
-    it('should create counter', () => {
+    testWithNames('counter', name => {
       const meterSharedState = new MeterSharedState(
         new MeterProviderSharedState(defaultResource),
         defaultInstrumentationScope
       );
       const meter = new Meter(meterSharedState);
-      const counter = meter.createCounter('foobar');
+      const counter = meter.createCounter(name);
       assert(counter instanceof CounterInstrument);
     });
   });
 
   describe('createUpDownCounter', () => {
-    it('should create up down counter', () => {
+    testWithNames('UpDownCounter', name => {
       const meterSharedState = new MeterSharedState(
         new MeterProviderSharedState(defaultResource),
         defaultInstrumentationScope
       );
       const meter = new Meter(meterSharedState);
-      const upDownCounter = meter.createUpDownCounter('foobar');
+      const upDownCounter = meter.createUpDownCounter(name);
       assert(upDownCounter instanceof UpDownCounterInstrument);
     });
   });
 
   describe('createHistogram', () => {
-    it('should create histogram', () => {
+    testWithNames('Histogram', name => {
       const meterSharedState = new MeterSharedState(
         new MeterProviderSharedState(defaultResource),
         defaultInstrumentationScope
       );
       const meter = new Meter(meterSharedState);
-      const histogram = meter.createHistogram('foobar');
+      const histogram = meter.createHistogram(name);
       assert(histogram instanceof HistogramInstrument);
     });
   });
 
   describe('createObservableGauge', () => {
-    it('should create observable gauge', () => {
+    testWithNames('ObservableGauge', name => {
       const meterSharedState = new MeterSharedState(
         new MeterProviderSharedState(defaultResource),
         defaultInstrumentationScope
       );
       const meter = new Meter(meterSharedState);
-      const observableGauge = meter.createObservableGauge('foobar');
+      const observableGauge = meter.createObservableGauge(name);
       assert(observableGauge instanceof ObservableGaugeInstrument);
     });
   });
 
   describe('createObservableCounter', () => {
-    it('should create observable counter', () => {
+    testWithNames('ObservableCounter', name => {
       const meterSharedState = new MeterSharedState(
         new MeterProviderSharedState(defaultResource),
         defaultInstrumentationScope
       );
       const meter = new Meter(meterSharedState);
-      const observableCounter = meter.createObservableCounter('foobar');
+      const observableCounter = meter.createObservableCounter(name);
       assert(observableCounter instanceof ObservableCounterInstrument);
     });
   });
 
   describe('createObservableUpDownCounter', () => {
-    it('should create observable up-down-counter', () => {
+    testWithNames('ObservableUpDownCounter', name => {
       const meterSharedState = new MeterSharedState(
         new MeterProviderSharedState(defaultResource),
         defaultInstrumentationScope
       );
       const meter = new Meter(meterSharedState);
-      const observableUpDownCounter =
-        meter.createObservableUpDownCounter('foobar');
+      const observableUpDownCounter = meter.createObservableUpDownCounter(name);
       assert(
         observableUpDownCounter instanceof ObservableUpDownCounterInstrument
       );
@@ -167,3 +176,22 @@ describe('Meter', () => {
     });
   });
 });
+
+function testWithNames(type: string, tester: (name: string) => void) {
+  for (const invalidName of invalidNames) {
+    it(`should warn with invalid name ${invalidName} for ${type}`, () => {
+      const warnStub = sinon.spy(diag, 'warn');
+      tester(invalidName);
+      assert.strictEqual(warnStub.callCount, 1);
+      assert.ok(warnStub.calledWithMatch('Invalid metric name'));
+    });
+  }
+
+  for (const validName of validNames) {
+    it(`should not warn with valid name ${validName} for ${type}`, () => {
+      const warnStub = sinon.spy(diag, 'warn');
+      tester(validName);
+      assert.strictEqual(warnStub.callCount, 0);
+    });
+  }
+}
diff --git a/packages/sdk-metrics/test/util.ts b/packages/sdk-metrics/test/util.ts
@@ -66,6 +66,9 @@ export const defaultInstrumentationScope: InstrumentationScope = {
   schemaUrl: 'https://opentelemetry.io/schemas/1.7.0',
 };
 
+export const invalidNames = ['', 'a'.repeat(256), '1a', '-a', '.a', '_a'];
+export const validNames = ['a', 'a'.repeat(255), 'a1', 'a-1', 'a.1', 'a_1'];
+
 export const commonValues: number[] = [1, -1, 1.0, Infinity, -Infinity, NaN];
 export const commonAttributes: MetricAttributes[] = [
   {},

Original file line number	Diff line number	Diff line change
`@@ -190,3 +190,7 @@ export function binarySearchLB(arr: number[], value: number): number {`
`190`	`190`	`}`
`191`	`191`	`return -1;`
`192`	`192`	`}`
	`193`	`+`
	`194`	`+export function equalsCaseInsensitive(lhs: string, rhs: string): boolean {`
	`195`	`+ return lhs.toLowerCase() === rhs.toLowerCase();`
	`196`	`+}`