Skip to content

Commit

Permalink
fix(sdk-metrics): metric names should be case-insensitive (#4059)
Browse files Browse the repository at this point in the history 
* fix(sdk-metrics): metric names should be case-insensitive

* fixup!

* fixup!
  • Loading branch information
@legendecas
legendecas authored Sep 7, 2023
1 parent 9452607 commit 4d662cf
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 17 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
18 changes: 16 additions & 2 deletions packages/sdk-metrics/src/InstrumentDescriptor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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;
}
4 changes: 4 additions & 0 deletions packages/sdk-metrics/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
106 changes: 106 additions & 0 deletions packages/sdk-metrics/test/InstrumentDescriptor.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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`);
}
});
});
});
58 changes: 43 additions & 15 deletions packages/sdk-metrics/test/Meter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
);
Expand Down Expand Up @@ -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);
});
}
}
3 changes: 3 additions & 0 deletions packages/sdk-metrics/test/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [
{},
Expand Down

0 comments on commit 4d662cf

Please sign in to comment.