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

feat(api-metrics): add schemaUrl to meter creations #2529

Merged
merged 4 commits into from
Nov 1, 2021
Merged

feat(api-metrics): add schemaUrl to meter creations #2529

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
94b67f1
feat(api-metrics): add schemaUrl to meter creations
legendecas Oct 11, 2021
eafb54f
fixup! update comment
legendecas Oct 11, 2021
acb658e
Merge branch 'main' into metrics-ff/schema-url
legendecas Oct 29, 2021
2f442c8
fixup! add a new positional parameter options
legendecas Oct 29, 2021
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
4 changes: 2 additions & 2 deletions experimental/packages/opentelemetry-api-metrics/src/NoopMeterProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { Meter } from './types/Meter';
import { Meter, MeterOptions } from './types/Meter';
import { MeterProvider } from './types/MeterProvider';
import { NOOP_METER } from './NoopMeter';

Expand All @@ -23,7 +23,7 @@ import { NOOP_METER } from './NoopMeter';
* for all calls to `getMeter`
*/
export class NoopMeterProvider implements MeterProvider {
getMeter(_name?: string, _version?: string): Meter {
getMeter(_name?: string, _options?: MeterOptions): Meter {
legendecas marked this conversation as resolved.
Show resolved Hide resolved
return NOOP_METER;
}
}
Expand Down
6 changes: 3 additions & 3 deletions experimental/packages/opentelemetry-api-metrics/src/api/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { Meter } from '../types/Meter';
import { Meter, MeterOptions } from '../types/Meter';
import { MeterProvider } from '../types/MeterProvider';
import { NOOP_METER_PROVIDER } from '../NoopMeterProvider';
import {
Expand Down Expand Up @@ -73,8 +73,8 @@ export class MetricsAPI {
/**
* Returns a meter from the global meter provider.
*/
public getMeter(name: string, version?: string): Meter {
return this.getMeterProvider().getMeter(name, version);
public getMeter(name: string, options?: MeterOptions): Meter {
return this.getMeterProvider().getMeter(name, options);
}

/** Remove the global meter provider */
Expand Down
14 changes: 14 additions & 0 deletions experimental/packages/opentelemetry-api-metrics/src/types/Meter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ import {
} from './Metric';
import { ObserverResult } from './ObserverResult';

/**
* An interface describes additional metadata of a meter.
*/
export interface MeterOptions {
/**
* The version of the meter or instrumentation library
*/
version?: string;
/**
* The schemaUrl of the meter or instrumentation library
*/
schemaUrl?: string;
}

/**
* An interface to allow the recording metrics.
*
Expand Down
10 changes: 5 additions & 5 deletions experimental/packages/opentelemetry-api-metrics/src/types/MeterProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,19 @@
* limitations under the License.
*/

import { Meter } from './Meter';
import { Meter, MeterOptions } from './Meter';

/**
* A registry for creating named {@link Meter}s.
*/
export interface MeterProvider {
/**
* Returns a Meter, creating one if one with the given name and version is
* not already created.
* Returns a Meter, creating one if one with the given name, version, and
* schemaUrl pair is not already created.
*
* @param name The name of the meter or instrumentation library.
* @param version The version of the meter or instrumentation library.
* @param options The options of the meter or instrumentation library.
* @returns Meter A Meter with the given name and version
*/
getMeter(name: string, version?: string): Meter;
getMeter(name: string, options?: MeterOptions): Meter;
}
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-exporter-otlp-grpc/test/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const meterProvider = new metrics.MeterProvider({
}),
});

const meter = meterProvider.getMeter('default', '0.0.1');
const meter = meterProvider.getMeter('default', { version: '0.0.1' });

const traceIdArr = [
31,
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-exporter-otlp-http/test/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const meterProvider = new metrics.MeterProvider({
}),
});

const meter = meterProvider.getMeter('default', '0.0.1');
const meter = meterProvider.getMeter('default', { version: '0.0.1' });

if (typeof Buffer === 'undefined') {
(window as any).Buffer = {
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-exporter-otlp-proto/test/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const meterProvider = new metrics.MeterProvider({
}),
});

const meter = meterProvider.getMeter('default', '0.0.1');
const meter = meterProvider.getMeter('default', { version: '0.0.1' });

export function mockCounter(): metrics.Metric<metrics.BoundCounter> & Counter {
const name = 'int-counter';
Expand Down
3 changes: 2 additions & 1 deletion experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ describe('PrometheusExporter', () => {
meterProvider = new MeterProvider({
interval: Math.pow(2, 31) - 1,
});
meter = meterProvider.getMeter('test-prometheus', '1', {
meter = meterProvider.getMeter('test-prometheus', {
version: '1',
exporter,
});
done();
Expand Down
8 changes: 6 additions & 2 deletions experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ export abstract class InstrumentationAbstract<T = any>

this._tracer = trace.getTracer(instrumentationName, instrumentationVersion);

this._meter = metrics.getMeter(instrumentationName, instrumentationVersion);
this._meter = metrics.getMeter(instrumentationName, {
version: instrumentationVersion,
});
}

/* Api to wrap instrumented method */
Expand All @@ -77,7 +79,9 @@ export abstract class InstrumentationAbstract<T = any>
public setMeterProvider(meterProvider: MeterProvider) {
this._meter = meterProvider.getMeter(
this.instrumentationName,
this.instrumentationVersion
{
version: this.instrumentationVersion,
}
);
}

Expand Down
11 changes: 8 additions & 3 deletions experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,17 @@ export class MeterProvider implements api.MeterProvider {
*
* @returns Meter A Meter with the given name and version
*/
getMeter(name: string, version?: string, config?: MeterConfig): Meter {
const key = `${name}@${version || ''}`;
getMeter(name: string, config?: MeterConfig): Meter {
const key = `${name}@${config?.version ?? ''}:${config?.schemaUrl ?? ''}`;
if (!this._meters.has(key)) {
this._meters.set(
key,
new Meter({ name, version }, config || this._config)
new Meter({
name,
version: config?.version,
// @ts-expect-error ts(2345) TODO: define the types in @opentelemetry/core
Copy link
Member

Choose a reason for hiding this comment

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

i wonder how could we can handle this more cleanly ? i expect this will happen commonly on the feature when experimental requires change on stable packages :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it is possible to link packages/* to the lerna project in experimental/. Even if it is possible, I'd believe we need a solution to publish experimental packages apart from the stable ones, that would eventually require a separate lerna project? Maybe @dyladan can chime on this.

Copy link
Member

Choose a reason for hiding this comment

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

It is not possible to link them together with the current setup. We could go back to the way it was before where all packages are part of the same lerna repo but make scripts which call lerna commands only on specific packages and use the independent version scheme? Having two groups of packages in lerna seems to be a thorny bit of business that there is no clear cut solution for.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think going back is necessary now. I would prefer to investigate alternative to lerna that suit our use case to avoid putting effort inside it, WDYT ?

schemaUrl: config?.schemaUrl
}, Object.assign({}, this._config, config))
);
}

Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-sdk-metrics-base/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { Processor } from './export/Processor';
import { MetricExporter } from './export/types';

/** MeterConfig provides an interface for configuring a Meter. */
export interface MeterConfig {
export interface MeterConfig extends api.MeterOptions {
/** Metric exporter. */
exporter?: MetricExporter;

Expand Down
3 changes: 2 additions & 1 deletion experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,8 @@ describe('Meter', () => {
});

it('should allow custom processor', () => {
const customMeter = new MeterProvider().getMeter('custom-processor', '*', {
const customMeter = new MeterProvider().getMeter('custom-processor', {
version: '*',
processor: new CustomProcessor(),
});
assert.throws(() => {
Expand Down
10 changes: 5 additions & 5 deletions experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,20 @@ describe('MeterProvider', () => {

it('should return the same Meter instance with same name & version', () => {
const provider = new MeterProvider();
const meter1 = provider.getMeter('meter1', 'ver1');
const meter2 = provider.getMeter('meter1', 'ver1');
const meter1 = provider.getMeter('meter1', { version: 'ver1' });
const meter2 = provider.getMeter('meter1', { version: 'ver1' });
assert.deepEqual(meter1, meter2);
});

it('should return different Meter instance with different name or version', () => {
const provider = new MeterProvider();

const meter1 = provider.getMeter('meter1', 'ver1');
const meter1 = provider.getMeter('meter1', { version: 'ver1' });
const meter2 = provider.getMeter('meter1');
assert.notEqual(meter1, meter2);

const meter3 = provider.getMeter('meter2', 'ver2');
const meter4 = provider.getMeter('meter3', 'ver2');
const meter3 = provider.getMeter('meter2', { version: 'ver2' });
const meter4 = provider.getMeter('meter3', { version: 'ver2' });
assert.notEqual(meter3, meter4);
});
});
Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry-core/src/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export interface ShimWrapped extends Function {
export interface InstrumentationLibrary {
readonly name: string;
readonly version?: string;
readonly schemaUrl?: string;
}

/** Defines an error handler function */
Expand Down