Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Sep 7, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44592d3
fix(sdk-metrics): metric names should be case-insensitive
legendecas Aug 14, 2023
73b1e84
fixup!
legendecas Sep 6, 2023
29818dc
Merge remote-tracking branch 'upstream/main' into metrics/metric-name
legendecas Sep 6, 2023
e0e7d78
fixup!
legendecas Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion packages/sdk-metrics/src/InstrumentDescriptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import { MetricOptions, ValueType } from '@opentelemetry/api';
import { View } from './view/View';
import { equalsCaseInsensitive } from './utils';

/**
* Supported types of metric instruments.
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is always good to include a comment with the description of the regex expression for easier reading
// ASCII string with a length no greater than 63 characters

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names were increased to <=255 character length open-telemetry/opentelemetry-specification#3648

Suggested change
const NAME_REGEXP = /^[a-z][a-z0-9_.-]{0,62}$/i;
// ASCII string with a length no greater than 255 characters
const NAME_REGEXP = /^[a-z][a-z0-9_.-]{0,255}$/i;

export function isValidName(name: string): boolean {
return name.match(NAME_REGEXP) != null;
}
16 changes: 16 additions & 0 deletions packages/sdk-metrics/src/Meter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ import {
ObservableUpDownCounter,
BatchObservableCallback,
Observable,
diag,
} from '@opentelemetry/api';
import {
createInstrumentDescriptor,
InstrumentType,
isValidName,
} from './InstrumentDescriptor';
import {
CounterInstrument,
Expand All @@ -50,6 +52,7 @@ export class Meter implements IMeter {
* Create a {@link Histogram} instrument.
*/
createHistogram(name: string, options?: MetricOptions): Histogram {
this._warnIfInvalidName(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add the warning during the call to createInstrumentDescriptor?

const descriptor = createInstrumentDescriptor(
name,
InstrumentType.HISTOGRAM,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -92,6 +97,7 @@ export class Meter implements IMeter {
name: string,
options?: MetricOptions
): ObservableGauge {
this._warnIfInvalidName(name);
const descriptor = createInstrumentDescriptor(
name,
InstrumentType.OBSERVABLE_GAUGE,
Expand All @@ -113,6 +119,7 @@ export class Meter implements IMeter {
name: string,
options?: MetricOptions
): ObservableCounter {
this._warnIfInvalidName(name);
const descriptor = createInstrumentDescriptor(
name,
InstrumentType.OBSERVABLE_COUNTER,
Expand All @@ -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,
Expand Down Expand Up @@ -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.`
);
}
}
}
4 changes: 4 additions & 0 deletions packages/sdk-metrics/src/utils.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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(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[] = [
{},
Expand Down