From 44592d313e296ea4c38ab3a3e4fc75929761b450 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 15 Aug 2023 01:46:58 +0800 Subject: [PATCH 1/3] fix(sdk-metrics): metric names should be case-insensitive --- CHANGELOG.md | 2 + .../sdk-metrics/src/InstrumentDescriptor.ts | 9 +- packages/sdk-metrics/src/Meter.ts | 16 +++ 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 + 7 files changed, 182 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63e669ceba0..b37167d8bed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :bug: (Bug Fix) +* fix(sdk-metrics): metric names should be case-insensitive + ### :books: (Refine Doc) ### :house: (Internal) diff --git a/packages/sdk-metrics/src/InstrumentDescriptor.ts b/packages/sdk-metrics/src/InstrumentDescriptor.ts index 01eabe26148..9d6e1f66597 100644 --- a/packages/sdk-metrics/src/InstrumentDescriptor.ts +++ b/packages/sdk-metrics/src/InstrumentDescriptor.ts @@ -16,6 +16,7 @@ import { MetricOptions, ValueType } from '@opentelemetry/api'; import { View } from './view/View'; +import { equalsCaseInsensitive } from './utils'; /** * Supported types of metric instruments. @@ -71,10 +72,16 @@ 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 ); } + +const NAME_REGEXP = /^[a-z][a-z0-9_.-]{0,62}$/i; +export function isValidName(name: string): boolean { + return name.match(NAME_REGEXP) != null; +} diff --git a/packages/sdk-metrics/src/Meter.ts b/packages/sdk-metrics/src/Meter.ts index b3d69d0dfd6..e90d68b5e80 100644 --- a/packages/sdk-metrics/src/Meter.ts +++ b/packages/sdk-metrics/src/Meter.ts @@ -25,10 +25,12 @@ import { ObservableUpDownCounter, BatchObservableCallback, Observable, + diag, } from '@opentelemetry/api'; import { createInstrumentDescriptor, InstrumentType, + isValidName, } from './InstrumentDescriptor'; import { CounterInstrument, @@ -50,6 +52,7 @@ export class Meter implements IMeter { * Create a {@link Histogram} instrument. */ createHistogram(name: string, options?: MetricOptions): Histogram { + this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.HISTOGRAM, @@ -63,6 +66,7 @@ export class Meter implements IMeter { * Create a {@link Counter} instrument. */ createCounter(name: string, options?: MetricOptions): Counter { + this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.COUNTER, @@ -76,6 +80,7 @@ export class Meter implements IMeter { * Create a {@link UpDownCounter} instrument. */ createUpDownCounter(name: string, options?: MetricOptions): UpDownCounter { + this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.UP_DOWN_COUNTER, @@ -92,6 +97,7 @@ export class Meter implements IMeter { name: string, options?: MetricOptions ): ObservableGauge { + this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.OBSERVABLE_GAUGE, @@ -113,6 +119,7 @@ export class Meter implements IMeter { name: string, options?: MetricOptions ): ObservableCounter { + this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.OBSERVABLE_COUNTER, @@ -134,6 +141,7 @@ export class Meter implements IMeter { name: string, options?: MetricOptions ): ObservableUpDownCounter { + this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.OBSERVABLE_UP_DOWN_COUNTER, @@ -173,4 +181,12 @@ export class Meter implements IMeter { observables ); } + + _warnIfInvalidName(name: string) { + if (!isValidName(name)) { + diag.warn( + `Invalid metric name: "${name}". The metric name should be a ASCII string with a length no greater than 63 characters.` + ); + } + } } diff --git a/packages/sdk-metrics/src/utils.ts b/packages/sdk-metrics/src/utils.ts index 835de92fe1b..9a8f80abde1 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 7d8796cbf13..10f4e9ed304 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 4ffe41607d9..e40d6aa5076 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 fa7a54b07fa..eb42a0d22a8 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(64), '1a', '-a', '.a', '_a']; +export const validNames = ['a', 'a'.repeat(63), 'a1', 'a-1', 'a.1', 'a_1']; + export const commonValues: number[] = [1, -1, 1.0, Infinity, -Infinity, NaN]; export const commonAttributes: MetricAttributes[] = [ {}, From 73b1e84f568cbe7d09e25a6535a5861042110931 Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 7 Sep 2023 00:51:53 +0800 Subject: [PATCH 2/3] fixup! --- packages/sdk-metrics/src/InstrumentDescriptor.ts | 11 +++++++++-- packages/sdk-metrics/src/Meter.ts | 16 ---------------- packages/sdk-metrics/test/util.ts | 4 ++-- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/packages/sdk-metrics/src/InstrumentDescriptor.ts b/packages/sdk-metrics/src/InstrumentDescriptor.ts index 9d6e1f66597..ac742fc0eee 100644 --- a/packages/sdk-metrics/src/InstrumentDescriptor.ts +++ b/packages/sdk-metrics/src/InstrumentDescriptor.ts @@ -14,7 +14,7 @@ * 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'; @@ -46,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, @@ -81,7 +86,9 @@ export function isDescriptorCompatibleWith( ); } -const NAME_REGEXP = /^[a-z][a-z0-9_.-]{0,62}$/i; +// 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/Meter.ts b/packages/sdk-metrics/src/Meter.ts index e90d68b5e80..b3d69d0dfd6 100644 --- a/packages/sdk-metrics/src/Meter.ts +++ b/packages/sdk-metrics/src/Meter.ts @@ -25,12 +25,10 @@ import { ObservableUpDownCounter, BatchObservableCallback, Observable, - diag, } from '@opentelemetry/api'; import { createInstrumentDescriptor, InstrumentType, - isValidName, } from './InstrumentDescriptor'; import { CounterInstrument, @@ -52,7 +50,6 @@ export class Meter implements IMeter { * Create a {@link Histogram} instrument. */ createHistogram(name: string, options?: MetricOptions): Histogram { - this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.HISTOGRAM, @@ -66,7 +63,6 @@ export class Meter implements IMeter { * Create a {@link Counter} instrument. */ createCounter(name: string, options?: MetricOptions): Counter { - this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.COUNTER, @@ -80,7 +76,6 @@ export class Meter implements IMeter { * Create a {@link UpDownCounter} instrument. */ createUpDownCounter(name: string, options?: MetricOptions): UpDownCounter { - this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.UP_DOWN_COUNTER, @@ -97,7 +92,6 @@ export class Meter implements IMeter { name: string, options?: MetricOptions ): ObservableGauge { - this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.OBSERVABLE_GAUGE, @@ -119,7 +113,6 @@ export class Meter implements IMeter { name: string, options?: MetricOptions ): ObservableCounter { - this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.OBSERVABLE_COUNTER, @@ -141,7 +134,6 @@ export class Meter implements IMeter { name: string, options?: MetricOptions ): ObservableUpDownCounter { - this._warnIfInvalidName(name); const descriptor = createInstrumentDescriptor( name, InstrumentType.OBSERVABLE_UP_DOWN_COUNTER, @@ -181,12 +173,4 @@ export class Meter implements IMeter { observables ); } - - _warnIfInvalidName(name: string) { - if (!isValidName(name)) { - diag.warn( - `Invalid metric name: "${name}". The metric name should be a ASCII string with a length no greater than 63 characters.` - ); - } - } } diff --git a/packages/sdk-metrics/test/util.ts b/packages/sdk-metrics/test/util.ts index eb42a0d22a8..ef081cf67a8 100644 --- a/packages/sdk-metrics/test/util.ts +++ b/packages/sdk-metrics/test/util.ts @@ -66,8 +66,8 @@ export const defaultInstrumentationScope: InstrumentationScope = { schemaUrl: 'https://opentelemetry.io/schemas/1.7.0', }; -export const invalidNames = ['', 'a'.repeat(64), '1a', '-a', '.a', '_a']; -export const validNames = ['a', 'a'.repeat(63), 'a1', 'a-1', 'a.1', 'a_1']; +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[] = [ From e0e7d789a341283f9f988cfc8eb8413db27bdea0 Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 7 Sep 2023 01:15:14 +0800 Subject: [PATCH 3/3] fixup! --- packages/sdk-metrics/test/InstrumentDescriptor.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sdk-metrics/test/InstrumentDescriptor.test.ts b/packages/sdk-metrics/test/InstrumentDescriptor.test.ts index 10f4e9ed304..5bf3196693f 100644 --- a/packages/sdk-metrics/test/InstrumentDescriptor.test.ts +++ b/packages/sdk-metrics/test/InstrumentDescriptor.test.ts @@ -46,7 +46,7 @@ describe('InstrumentDescriptor', () => { string, boolean, InstrumentDescriptor, - InstrumentDescriptor + InstrumentDescriptor, ][] = [ [ 'Compatible with identical descriptors',