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(sdk-metrics-base): meter registration #2666

Merged
merged 7 commits into from
Jan 8, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ export class Meter implements metrics.Meter {

// instrumentation library required by spec to be on meter
// spec requires provider config changes to apply to previously created meters, achieved by holding a reference to the provider
constructor(private _meterProviderSharedState: MeterProviderSharedState, private _instrumentationLibrary: InstrumentationLibrary) { }
constructor(private _meterProviderSharedState: MeterProviderSharedState, private _instrumentationLibrary: InstrumentationLibrary) {
this._meterProviderSharedState.meters.push(this);
}

/** this exists just to prevent ts errors from unused variables and may be removed */
getInstrumentationLibrary(): InstrumentationLibrary {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ export class MeterProvider {
private _sharedState: MeterProviderSharedState;
private _shutdown = false;

constructor(options: MeterProviderOptions) {
this._sharedState = new MeterProviderSharedState(options.resource ?? Resource.empty());
constructor(options?: MeterProviderOptions) {
this._sharedState = new MeterProviderSharedState(options?.resource ?? Resource.empty());
}

getMeter(name: string, version = '', options: metrics.MeterOptions = {}): metrics.Meter {
Copy link
Member

Choose a reason for hiding this comment

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

If the schema URL is part of the identity, shouldn't it be a top-level property and not buried in options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, there is only one requirement for meter creation:

Implementations MUST NOT require users to repeatedly obtain a Meter again with the same name+version+schema_url to pick up configuration changes. This can be achieved either by allowing to work with an outdated configuration or by ensuring that new configuration applies also to previously returned Meters.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter

It also suggests that:

It is unspecified whether or under which conditions the same or different Meter instances are returned from this function.

So, as long as we propagate the configurations to all meters created without another getMeter call, it should be spec-compliant.

As we are using the MeterProviderSharedState pattern to share the meter provider configurations, I'd believe we've already met the bar that the created meters can automatically get configuration updates without the need to call getMeter again. So it seems that we actually don't have to return an identical meter for each getMeter call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another problem is that the spec is said instrument names are unique in a meter:

Meter implementations MUST return an error when multiple Instruments are registered under the same Meter instance using the same name.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument

So, if we are going to return a new meter instance for every getMeter, how do we define "same Meter instance" in the above context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Submitted a spec issue for this: open-telemetry/opentelemetry-specification#2226

Expand All @@ -46,10 +46,6 @@ export class MeterProvider {
return metrics.NOOP_METER;
}

// Spec leaves it unspecified if creating a meter with duplicate
// name/version returns the same meter. We create a new one here
// for simplicity. This may change in the future.
// TODO: consider returning the same meter if the same name/version is used
return new Meter(this._sharedState, { name, version, schemaUrl: options.schemaUrl });
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class MeterProviderSharedState {

metricCollectors: MetricCollector[] = [];

meters: Map<string, Meter> = new Map();
meters: Meter[] = [];
dyladan marked this conversation as resolved.
Show resolved Hide resolved

constructor(public resource: Resource) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class MetricCollector implements MetricProducer {

async collect(): Promise<MetricData[]> {
const collectionTime = hrTime();
const results = await Promise.all(Array.from(this._sharedState.meters.values())
const results = await Promise.all(this._sharedState.meters
.map(meter => meter.collect(this, collectionTime)));

return results.reduce((cumulation, current) => cumulation.concat(current), []);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as assert from 'assert';
import { NOOP_METER } from '@opentelemetry/api-metrics-wip';
import { Meter } from '../src/Meter';
import { MeterProvider } from '../src/MeterProvider';
import { defaultResource } from './util';

describe('MeterProvider', () => {
describe('constructor', () => {
it('should construct without exceptions', () => {
const meterProvider = new MeterProvider();
assert(meterProvider instanceof MeterProvider);
});

it('construct with resource', () => {
const meterProvider = new MeterProvider({ resource: defaultResource });
assert(meterProvider instanceof MeterProvider);
});
});

describe('getMeter', () => {
it('should get a meter', () => {
const meterProvider = new MeterProvider();
const meter = meterProvider.getMeter('meter1', '1.0.0');
assert(meter instanceof Meter);
});

it('get a noop meter on shutdown', () => {
const meterProvider = new MeterProvider();
meterProvider.shutdown();
const meter = meterProvider.getMeter('meter1', '1.0.0');
assert.strictEqual(meter, NOOP_METER);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import { MetricReader } from '../../src';
import { MetricCollector } from '../../src/state/MetricCollector';

/**
* A test metric reader that implements no-op onForceFlush() and onShutdown() handlers.
Expand All @@ -27,4 +28,8 @@ export class TestMetricReader extends MetricReader {
protected onShutdown(): Promise<void> {
return Promise.resolve(undefined);
}

getMetricCollector(): MetricCollector {
return this['_metricProducer'] as MetricCollector;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@

import * as assert from 'assert';
import * as sinon from 'sinon';
import { MeterProvider } from '../../src';
import { AggregationTemporality } from '../../src/export/AggregationTemporality';
import { MetricData, PointDataType } from '../../src/export/MetricData';
import { MetricExporter } from '../../src/export/MetricExporter';
import { Meter } from '../../src/Meter';
import { MeterProviderSharedState } from '../../src/state/MeterProviderSharedState';
import { MetricCollector } from '../../src/state/MetricCollector';
import { defaultInstrumentationLibrary, defaultResource, assertMetricData, assertPointData } from '../util';
Expand Down Expand Up @@ -64,15 +64,15 @@ describe('MetricCollector', () => {

describe('collect', () => {
function setupInstruments(exporter: MetricExporter) {
// TODO(legendecas): setup with MeterProvider when meter identity was settled.
const meterProviderSharedState = new MeterProviderSharedState(defaultResource);
const meterProvider = new MeterProvider({ resource: defaultResource });

const reader = new TestMetricReader(exporter.getPreferredAggregationTemporality());
const metricCollector = new MetricCollector(meterProviderSharedState, reader);
meterProviderSharedState.metricCollectors.push(metricCollector);
meterProvider.addMetricReader(reader);
const metricCollector = reader.getMetricCollector();

const meter = new Meter(meterProviderSharedState, defaultInstrumentationLibrary);
meterProviderSharedState.meters.set('test-meter', meter);
const meter = meterProvider.getMeter(defaultInstrumentationLibrary.name, defaultInstrumentationLibrary.version, {
schemaUrl: defaultInstrumentationLibrary.schemaUrl,
});

return { metricCollector, meter };
}
Expand Down