From 4d662cf84d75647e92f898dbabcefd7945f561d6 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 7 Sep 2023 13:41:06 +0800 Subject: [PATCH] fix(sdk-metrics): metric names should be case-insensitive (#4059) * fix(sdk-metrics): metric names should be case-insensitive * fixup! * fixup! --- CHANGELOG.md | 1 + .../sdk-metrics/src/InstrumentDescriptor.ts | 18 ++- packages/sdk-metrics/src/utils.ts | 4 + .../test/InstrumentDescriptor.test.ts | 106 ++++++++++++++++++ packages/sdk-metrics/test/Meter.test.ts | 58 +++++++--- packages/sdk-metrics/test/util.ts | 3 + 6 files changed, 173 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 049f668cfc..47f735d6ad 100644 --- 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 index 01eabe2614..ac742fc0ee 100644 --- 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 index 835de92fe1..9a8f80abde 100644 --- 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 index 7d8796cbf1..5bf3196693 100644 --- 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 index 4ffe41607d..e40d6aa507 100644 --- 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 index fa7a54b07f..ef081cf67a 100644 --- 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[] = [ {},