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.

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

feat: add unit to view instrument selection criteria #3647

Merged
merged 6 commits into from
Mar 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
* perf(propagator-jaeger): improve deserializeSpanContext performance [#3541](https://github.com/open-telemetry/opentelemetry-js/pull/3541) @doochik
* feat: support TraceState in SamplingResult [#3530](https://github.com/open-telemetry/opentelemetry-js/pull/3530) @raphael-theriault-swi
* feat(sdk-trace-base): add diagnostic logging when spans are dropped [#3610](https://github.com/open-telemetry/opentelemetry-js/pull/3610) @neoeinstein
* feat: add unit to view instrument selection criteria [#3647](https://github.com/open-telemetry/opentelemetry-js/pull/3647) @jlabatut

### :bug: (Bug Fix)

Expand Down
9 changes: 8 additions & 1 deletion packages/sdk-metrics/src/view/InstrumentSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,23 @@
*/

import { InstrumentType } from '../InstrumentDescriptor';
import { PatternPredicate, Predicate } from './Predicate';
import { ExactPredicate, PatternPredicate, Predicate } from './Predicate';

export interface InstrumentSelectorCriteria {
name?: string;
type?: InstrumentType;
unit?: string;
}

export class InstrumentSelector {
private _nameFilter: Predicate;
private _type?: InstrumentType;
private _unitFilter: Predicate;

constructor(criteria?: InstrumentSelectorCriteria) {
this._nameFilter = new PatternPredicate(criteria?.name ?? '*');
this._type = criteria?.type;
this._unitFilter = new ExactPredicate(criteria?.unit);
}

getType() {
Expand All @@ -38,4 +41,8 @@ export class InstrumentSelector {
getNameFilter() {
return this._nameFilter;
}

getUnitFilter() {
return this._unitFilter;
}
}
2 changes: 2 additions & 0 deletions packages/sdk-metrics/src/view/RegistrationConflicts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function getTypeConflictResolutionRecipe(
const selector: InstrumentSelectorCriteria = {
name: otherDescriptor.name,
type: otherDescriptor.type,
unit: otherDescriptor.unit,
};

const selectorString = JSON.stringify(selector);
Expand All @@ -73,6 +74,7 @@ export function getDescriptionResolutionRecipe(
const selector: InstrumentSelectorCriteria = {
name: otherDescriptor.name,
type: otherDescriptor.type,
unit: otherDescriptor.unit,
};

const selectorString = JSON.stringify(selector);
Expand Down
13 changes: 13 additions & 0 deletions packages/sdk-metrics/src/view/View.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,14 @@ export type ViewOptions = {
* instrumentName: 'my.instruments.requests'
*/
instrumentName?: string;
/**
* Instrument selection criteria:
* The unit of the Instrument(s).
*
* @example <caption>select all instruments with unit 'ms'</caption>
* instrumentUnit: 'ms'
*/
instrumentUnit?: string;
/**
* Instrument selection criteria:
* The name of the Meter. No wildcard support, name must match the meter exactly.
Expand Down Expand Up @@ -113,6 +121,7 @@ function isSelectorNotProvided(options: ViewOptions): boolean {
return (
options.instrumentName == null &&
options.instrumentType == null &&
options.instrumentUnit == null &&
options.meterName == null &&
options.meterVersion == null &&
options.meterSchemaUrl == null
Expand Down Expand Up @@ -161,6 +170,9 @@ export class View {
* @param viewOptions.instrumentType
* Instrument selection criteria:
* The original type of the Instrument(s).
* @param viewOptions.instrumentUnit
* Instrument selection criteria:
* The unit of the Instrument(s).
* @param viewOptions.meterName
* Instrument selection criteria:
* The name of the Meter. No wildcard support, name must match the meter exactly.
Expand Down Expand Up @@ -213,6 +225,7 @@ export class View {
this.instrumentSelector = new InstrumentSelector({
name: viewOptions.instrumentName,
type: viewOptions.instrumentType,
unit: viewOptions.instrumentUnit,
});
this.meterSelector = new MeterSelector({
name: viewOptions.meterName,
Expand Down
3 changes: 2 additions & 1 deletion packages/sdk-metrics/src/view/ViewRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export class ViewRegistry {
return (
(selector.getType() === undefined ||
instrument.type === selector.getType()) &&
selector.getNameFilter().match(instrument.name)
selector.getNameFilter().match(instrument.name) &&
selector.getUnitFilter().match(instrument.unit)
);
}

Expand Down
72 changes: 71 additions & 1 deletion packages/sdk-metrics/test/MeterProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@
*/

import * as assert from 'assert';
import { MeterProvider, InstrumentType, DataPointType } from '../src';
import {
MeterProvider,
InstrumentType,
DataPointType,
ExplicitBucketHistogramAggregation,
HistogramMetricData,
} from '../src';
import {
assertScopeMetrics,
assertMetricData,
Expand Down Expand Up @@ -463,6 +469,70 @@ describe('MeterProvider', () => {
}
);
});

it('with instrument unit should apply view to only the selected instrument unit', async () => {
// Add views with different boundaries for each unit.
const msBoundaries = [0, 1, 2, 3, 4, 5];
const sBoundaries = [10, 50, 250, 1000];

const meterProvider = new MeterProvider({
resource: defaultResource,
views: [
new View({
instrumentUnit: 'ms',
aggregation: new ExplicitBucketHistogramAggregation(msBoundaries),
}),
new View({
instrumentUnit: 's',
aggregation: new ExplicitBucketHistogramAggregation(sBoundaries),
}),
],
});

const reader = new TestMetricReader();
meterProvider.addMetricReader(reader);

// Create meter and histograms, with different units.
const meter = meterProvider.getMeter('meter1', 'v1.0.0');
const histogram1 = meter.createHistogram('test-histogram-ms', {
unit: 'ms',
});
const histogram2 = meter.createHistogram('test-histogram-s', {
unit: 's',
});

// Record values for both.
histogram1.record(1);
histogram2.record(1);

// Perform collection.
const { resourceMetrics, errors } = await reader.collect();

assert.strictEqual(errors.length, 0);
// Results came only from one Meter
assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);

// InstrumentationScope matches the only created Meter.
assertScopeMetrics(resourceMetrics.scopeMetrics[0], {
name: 'meter1',
version: 'v1.0.0',
});

// Two metrics are collected ('test-histogram-ms' and 'test-histogram-s')
assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 2);

// Check if the boundaries are applied to the correct instrument.
assert.deepStrictEqual(
(resourceMetrics.scopeMetrics[0].metrics[0] as HistogramMetricData)
.dataPoints[0].value.buckets.boundaries,
msBoundaries
);
assert.deepStrictEqual(
(resourceMetrics.scopeMetrics[0].metrics[1] as HistogramMetricData)
.dataPoints[0].value.buckets.boundaries,
sBoundaries
);
});
});

describe('shutdown', () => {
Expand Down
44 changes: 44 additions & 0 deletions packages/sdk-metrics/test/view/ViewRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,50 @@ describe('ViewRegistry', () => {
assert.strictEqual(views[0].name, 'histogram');
}
});

it('should match view with instrument unit', () => {
const registry = new ViewRegistry();
registry.addView(
new View({
name: 'ms_view',
instrumentName: 'default_metric',
instrumentUnit: 'ms',
})
);
registry.addView(
new View({
name: 's_view',
instrumentName: 'default_metric',
instrumentUnit: 's',
})
);

{
const views = registry.findViews(
{
...defaultInstrumentDescriptor,
unit: 'ms',
},
defaultInstrumentationScope
);

assert.strictEqual(views.length, 1);
assert.strictEqual(views[0].name, 'ms_view');
}

{
const views = registry.findViews(
{
...defaultInstrumentDescriptor,
unit: 's',
},
defaultInstrumentationScope
);

assert.strictEqual(views.length, 1);
assert.strictEqual(views[0].name, 's_view');
}
});
});

describe('MeterSelector', () => {
Expand Down