diff --git a/CHANGELOG.md b/CHANGELOG.md index ab253b45b20..ed3bf2a212c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se * refactor(sdk-trace-base)!: remove `new Span` constructor in favor of `Tracer.startSpan` API [#5048](https://github.com/open-telemetry/opentelemetry-js/pull/5048) @david-luna * refactor(sdk-trace-base)!: remove `BasicTracerProvider.addSpanProcessor` API in favor of constructor options. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5134) @david-luna * refactor(sdk-trace-base)!: make `resource` property private in `BasicTracerProvider` and remove `getActiveSpanProcessor` API. [#5192](https://github.com/open-telemetry/opentelemetry-js/pull/5192) @david-luna +* feat(sdk-metrics)!: extract `IMetricReader` interface and use it over abstract class [#5311](https://github.com/open-telemetry/opentelemetry-js/pull/5311) + * (user-facing): `MeterProviderOptions` now provides the more general `IMetricReader` type over `MetricReader` + * If you accept `MetricReader` in your public interface, consider accepting the more general `IMetricReader` instead to avoid unintentional breaking changes * feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters [#5239](https://github.com/open-telemetry/opentelemetry-js/pull/5239) @pichlermarc * When extending `BasicTracerProvider`, the class offered multiple methods to facilitate the creation of exporters and auto-pairing with `SpanProcessor`s. * This functionality has been removed - users may now pass `SpanProcessor`s to the base class constructor when extending diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index d06a791cc8e..49d09322049 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -13,6 +13,8 @@ All notable changes to experimental packages in this project will be documented * The internal OpenTelemetry data model dropped the concept of instrument type on exported metrics, therefore mapping it is not necessary anymore. * feat(instrumentation-fetch)!: passthrough original response to `applyCustomAttributes` hook [#5281](https://github.com/open-telemetry/opentelemetry-js/pull/5281) @chancancode * Previously, the fetch instrumentation code unconditionally clones every `fetch()` response in order to preserve the ability for the `applyCustomAttributes` hook to consume the response body. This is fundamentally unsound, as it forces the browser to buffer and retain the response body until it is fully received and read, which crates unnecessary memory pressure on large or long-running response streams. In extreme cases, this is effectively a memory leak and can cause the browser tab to crash. If your use case for `applyCustomAttributes` requires access to the response body, please chime in on [#5293](https://github.com/open-telemetry/opentelemetry-js/issues/5293). +* feat(sdk-node)!: use `IMetricReader` over `MetricReader` [#5311](https://github.com/open-telemetry/opentelemetry-js/pull/5311) + * (user-facing): `NodeSDKConfiguration` now provides the more general `IMetricReader` type over `MetricReader` ### :rocket: (Enhancement) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index e3920bfa395..6fcdeb4832f 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -54,7 +54,7 @@ import { OTLPMetricExporter as OTLPHttpMetricExporter } from '@opentelemetry/exp import { PrometheusExporter as PrometheusMetricExporter } from '@opentelemetry/exporter-prometheus'; import { MeterProvider, - MetricReader, + IMetricReader, ViewOptions, ConsoleMetricExporter, PeriodicExportingMetricReader, @@ -82,7 +82,7 @@ export type MeterProviderConfig = { /** * Reference to the MetricReader instance by the NodeSDK */ - reader?: MetricReader; + reader?: IMetricReader; /** * List of {@link ViewOptions}s that should be passed to the MeterProvider */ @@ -107,8 +107,8 @@ function getValueInMillis(envName: string, defaultValue: number): number { * * @returns MetricReader[] if appropriate environment variables are configured */ -function configureMetricProviderFromEnv(): MetricReader[] { - const metricReaders: MetricReader[] = []; +function configureMetricProviderFromEnv(): IMetricReader[] { + const metricReaders: IMetricReader[] = []; const metricsExporterList = process.env.OTEL_METRICS_EXPORTER?.trim(); if (!metricsExporterList) { return metricReaders; @@ -395,16 +395,16 @@ export class NodeSDK { logs.setGlobalLoggerProvider(loggerProvider); } - const metricReadersFromEnv: MetricReader[] = + const metricReadersFromEnv: IMetricReader[] = configureMetricProviderFromEnv(); if (this._meterProviderConfig || metricReadersFromEnv.length > 0) { - const readers: MetricReader[] = []; + const readers: IMetricReader[] = []; if (this._meterProviderConfig?.reader) { readers.push(this._meterProviderConfig.reader); } if (readers.length === 0) { - metricReadersFromEnv.forEach((r: MetricReader) => readers.push(r)); + metricReadersFromEnv.forEach((r: IMetricReader) => readers.push(r)); } const meterProvider = new MeterProvider({ diff --git a/experimental/packages/opentelemetry-sdk-node/src/types.ts b/experimental/packages/opentelemetry-sdk-node/src/types.ts index 0683a3d886f..713d616413c 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/types.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/types.ts @@ -19,7 +19,7 @@ import { TextMapPropagator } from '@opentelemetry/api'; import { Instrumentation } from '@opentelemetry/instrumentation'; import { Detector, DetectorSync, IResource } from '@opentelemetry/resources'; import { LogRecordProcessor } from '@opentelemetry/sdk-logs'; -import { MetricReader, ViewOptions } from '@opentelemetry/sdk-metrics'; +import { IMetricReader, ViewOptions } from '@opentelemetry/sdk-metrics'; import { Sampler, SpanExporter, @@ -35,7 +35,7 @@ export interface NodeSDKConfiguration { /** @deprecated use logRecordProcessors instead*/ logRecordProcessor: LogRecordProcessor; logRecordProcessors?: LogRecordProcessor[]; - metricReader: MetricReader; + metricReader: IMetricReader; views: ViewOptions[]; instrumentations: (Instrumentation | Instrumentation[])[]; resource: IResource; diff --git a/packages/sdk-metrics/src/MeterProvider.ts b/packages/sdk-metrics/src/MeterProvider.ts index 0d8d3e13b4d..f201356c05c 100644 --- a/packages/sdk-metrics/src/MeterProvider.ts +++ b/packages/sdk-metrics/src/MeterProvider.ts @@ -22,7 +22,7 @@ import { createNoopMeter, } from '@opentelemetry/api'; import { IResource, Resource } from '@opentelemetry/resources'; -import { MetricReader } from './export/MetricReader'; +import { IMetricReader } from './export/MetricReader'; import { MeterProviderSharedState } from './state/MeterProviderSharedState'; import { MetricCollector } from './state/MetricCollector'; import { ForceFlushOptions, ShutdownOptions } from './types'; @@ -35,7 +35,7 @@ export interface MeterProviderOptions { /** Resource associated with metric telemetry */ resource?: IResource; views?: ViewOptions[]; - readers?: MetricReader[]; + readers?: IMetricReader[]; } /** diff --git a/packages/sdk-metrics/src/export/MetricProducer.ts b/packages/sdk-metrics/src/export/MetricProducer.ts index 87af8a1210d..88adbd36d7f 100644 --- a/packages/sdk-metrics/src/export/MetricProducer.ts +++ b/packages/sdk-metrics/src/export/MetricProducer.ts @@ -29,7 +29,7 @@ export interface MetricCollectOptions { } /** - * This is a public interface that represent an export state of a MetricReader. + * This is a public interface that represent an export state of a IMetricReader. */ export interface MetricProducer { /** diff --git a/packages/sdk-metrics/src/export/MetricReader.ts b/packages/sdk-metrics/src/export/MetricReader.ts index c70b4d17eab..3781e514168 100644 --- a/packages/sdk-metrics/src/export/MetricReader.ts +++ b/packages/sdk-metrics/src/export/MetricReader.ts @@ -18,7 +18,7 @@ import * as api from '@opentelemetry/api'; import { AggregationTemporality } from './AggregationTemporality'; import { MetricProducer } from './MetricProducer'; import { CollectionResult, InstrumentType } from './MetricData'; -import { FlatMap, callWithTimeout } from '../utils'; +import { callWithTimeout, FlatMap } from '../utils'; import { CollectionOptions, ForceFlushOptions, @@ -38,16 +38,22 @@ export interface MetricReaderOptions { * Aggregation selector based on metric instrument types. If no views are * configured for a metric instrument, a per-metric-reader aggregation is * selected with this selector. + * + *

NOTE: the provided function MUST be pure */ aggregationSelector?: AggregationSelector; /** * Aggregation temporality selector based on metric instrument types. If * not configured, cumulative is used for all instruments. + * + *

NOTE: the provided function MUST be pure */ aggregationTemporalitySelector?: AggregationTemporalitySelector; /** * Cardinality selector based on metric instrument types. If not configured, * a default value is used. + * + *

NOTE: the provided function MUST be pure */ cardinalitySelector?: CardinalitySelector; /** @@ -59,11 +65,75 @@ export interface MetricReaderOptions { metricProducers?: MetricProducer[]; } +/** + * Reads metrics from the SDK. Implementations MUST follow the Metric Reader Specification as well as the requirements + * listed in this interface. Consider extending {@link MetricReader} to get a specification-compliant base implementation + * of this interface + */ +export interface IMetricReader { + /** + * Set the {@link MetricProducer} used by this instance. **This should only be called once by the + * SDK and should be considered internal.** + * + *

NOTE: implementations MUST throw when called more than once + * + * @param metricProducer + */ + setMetricProducer(metricProducer: MetricProducer): void; + + /** + * Select the {@link AggregationOption} for the given {@link InstrumentType} for this + * reader. + * + *

NOTE: implementations MUST be pure + */ + selectAggregation(instrumentType: InstrumentType): AggregationOption; + + /** + * Select the {@link AggregationTemporality} for the given + * {@link InstrumentType} for this reader. + * + *

NOTE: implementations MUST be pure + */ + selectAggregationTemporality( + instrumentType: InstrumentType + ): AggregationTemporality; + + /** + * Select the cardinality limit for the given {@link InstrumentType} for this + * reader. + * + *

NOTE: implementations MUST be pure + */ + selectCardinalityLimit(instrumentType: InstrumentType): number; + + /** + * Collect all metrics from the associated {@link MetricProducer} + */ + collect(options?: CollectionOptions): Promise; + + /** + * Shuts down the metric reader, the promise will reject after the optional timeout or resolve after completion. + * + *

NOTE: this operation MAY continue even after the promise rejects due to a timeout. + * @param options options with timeout. + */ + shutdown(options?: ShutdownOptions): Promise; + + /** + * Flushes metrics read by this reader, the promise will reject after the optional timeout or resolve after completion. + * + *

NOTE: this operation MAY continue even after the promise rejects due to a timeout. + * @param options options with timeout. + */ + forceFlush(options?: ForceFlushOptions): Promise; +} + /** * A registered reader of metrics that, when linked to a {@link MetricProducer}, offers global * control over metrics. */ -export abstract class MetricReader { +export abstract class MetricReader implements IMetricReader { // Tracks the shutdown state. // TODO: use BindOncePromise here once a new version of @opentelemetry/core is available. private _shutdown = false; @@ -85,16 +155,6 @@ export abstract class MetricReader { this._cardinalitySelector = options?.cardinalitySelector; } - /** - * Set the {@link MetricProducer} used by this instance. **This should only be called by the - * SDK and should be considered internal.** - * - * To add additional {@link MetricProducer}s to a {@link MetricReader}, pass them to the - * constructor as {@link MetricReaderOptions.metricProducers}. - * - * @internal - * @param metricProducer - */ setMetricProducer(metricProducer: MetricProducer) { if (this._sdkMetricProducer) { throw new Error( @@ -105,28 +165,16 @@ export abstract class MetricReader { this.onInitialized(); } - /** - * Select the {@link AggregationOption} for the given {@link InstrumentType} for this - * reader. - */ selectAggregation(instrumentType: InstrumentType): AggregationOption { return this._aggregationSelector(instrumentType); } - /** - * Select the {@link AggregationTemporality} for the given - * {@link InstrumentType} for this reader. - */ selectAggregationTemporality( instrumentType: InstrumentType ): AggregationTemporality { return this._aggregationTemporalitySelector(instrumentType); } - /** - * Select the cardinality limit for the given {@link InstrumentType} for this - * reader. - */ selectCardinalityLimit(instrumentType: InstrumentType): number { return this._cardinalitySelector ? this._cardinalitySelector(instrumentType) @@ -158,9 +206,6 @@ export abstract class MetricReader { */ protected abstract onForceFlush(): Promise; - /** - * Collect all metrics from the associated {@link MetricProducer} - */ async collect(options?: CollectionOptions): Promise { if (this._sdkMetricProducer === undefined) { throw new Error('MetricReader is not bound to a MetricProducer'); @@ -204,12 +249,6 @@ export abstract class MetricReader { }; } - /** - * Shuts down the metric reader, the promise will reject after the optional timeout or resolve after completion. - * - *

NOTE: this operation will continue even after the promise rejects due to a timeout. - * @param options options with timeout. - */ async shutdown(options?: ShutdownOptions): Promise { // Do not call shutdown again if it has already been called. if (this._shutdown) { @@ -227,12 +266,6 @@ export abstract class MetricReader { this._shutdown = true; } - /** - * Flushes metrics read by this reader, the promise will reject after the optional timeout or resolve after completion. - * - *

NOTE: this operation will continue even after the promise rejects due to a timeout. - * @param options options with timeout. - */ async forceFlush(options?: ForceFlushOptions): Promise { if (this._shutdown) { api.diag.warn('Cannot forceFlush on already shutdown MetricReader.'); diff --git a/packages/sdk-metrics/src/index.ts b/packages/sdk-metrics/src/index.ts index d18832c64bf..da48809a759 100644 --- a/packages/sdk-metrics/src/index.ts +++ b/packages/sdk-metrics/src/index.ts @@ -45,7 +45,11 @@ export { export { PushMetricExporter } from './export/MetricExporter'; -export { MetricReader, MetricReaderOptions } from './export/MetricReader'; +export { + IMetricReader, + MetricReader, + MetricReaderOptions, +} from './export/MetricReader'; export { PeriodicExportingMetricReader, diff --git a/packages/sdk-metrics/src/state/MetricCollector.ts b/packages/sdk-metrics/src/state/MetricCollector.ts index 1a3fd23360f..02faf0ae531 100644 --- a/packages/sdk-metrics/src/state/MetricCollector.ts +++ b/packages/sdk-metrics/src/state/MetricCollector.ts @@ -21,8 +21,8 @@ import { InstrumentType, ScopeMetrics, } from '../export/MetricData'; -import { MetricProducer, MetricCollectOptions } from '../export/MetricProducer'; -import { MetricReader } from '../export/MetricReader'; +import { MetricCollectOptions, MetricProducer } from '../export/MetricProducer'; +import { IMetricReader } from '../export/MetricReader'; import { ForceFlushOptions, ShutdownOptions } from '../types'; import { MeterProviderSharedState } from './MeterProviderSharedState'; @@ -34,7 +34,7 @@ import { MeterProviderSharedState } from './MeterProviderSharedState'; export class MetricCollector implements MetricProducer { constructor( private _sharedState: MeterProviderSharedState, - private _metricReader: MetricReader + private _metricReader: IMetricReader ) {} async collect(options?: MetricCollectOptions): Promise { diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts index 161e9711fa8..df0ef281831 100644 --- a/packages/sdk-metrics/test/Instruments.test.ts +++ b/packages/sdk-metrics/test/Instruments.test.ts @@ -24,7 +24,6 @@ import { Histogram, InstrumentType, MeterProvider, - MetricReader, } from '../src'; import { InstrumentDescriptor } from '../src/InstrumentDescriptor'; import { @@ -40,6 +39,7 @@ import { defaultResource, } from './util'; import { ObservableResult, ValueType } from '@opentelemetry/api'; +import { IMetricReader } from '../src/export/MetricReader'; describe('Instruments', () => { describe('Counter', () => { @@ -845,7 +845,7 @@ interface ValidateMetricData { } async function validateExport( - reader: MetricReader, + reader: IMetricReader, expected: ValidateMetricData ) { const { resourceMetrics, errors } = await reader.collect(); diff --git a/packages/sdk-metrics/test/export/utils.ts b/packages/sdk-metrics/test/export/utils.ts index 77b2791b203..8e3ff1e5d7a 100644 --- a/packages/sdk-metrics/test/export/utils.ts +++ b/packages/sdk-metrics/test/export/utils.ts @@ -18,7 +18,7 @@ import { AggregationSelector, AggregationTemporalitySelector, InstrumentType, - MetricReader, + IMetricReader, PushMetricExporter, } from '../../src'; import * as assert from 'assert'; @@ -39,7 +39,7 @@ const instrumentTypes = [ * @param expectedSelector */ export function assertAggregationSelector( - reader: MetricReader | PushMetricExporter, + reader: IMetricReader | PushMetricExporter, expectedSelector: AggregationSelector ) { for (const instrumentType of instrumentTypes) { @@ -57,7 +57,7 @@ export function assertAggregationSelector( * @param expectedSelector */ export function assertAggregationTemporalitySelector( - reader: MetricReader | PushMetricExporter, + reader: IMetricReader | PushMetricExporter, expectedSelector: AggregationTemporalitySelector ) { for (const instrumentType of instrumentTypes) { diff --git a/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts b/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts index 3787308d9ec..32dab127fb8 100644 --- a/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts +++ b/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts @@ -22,9 +22,9 @@ import { AggregationType, InstrumentType, MeterProvider, - MetricReader, } from '../../src'; import { TestMetricReader } from '../export/TestMetricReader'; +import { IMetricReader } from '../../src/export/MetricReader'; describe('cumulative-exponential-histogram', () => { let clock: sinon.SinonFakeTimers; @@ -84,7 +84,7 @@ describe('cumulative-exponential-histogram', () => { ); }; - const collectNoErrors = async (reader: MetricReader) => { + const collectNoErrors = async (reader: IMetricReader) => { const { resourceMetrics, errors } = await reader.collect(); assert.strictEqual(errors.length, 0); return resourceMetrics; diff --git a/packages/sdk-metrics/test/regression/histogram-recording-nan.test.ts b/packages/sdk-metrics/test/regression/histogram-recording-nan.test.ts index 4cb73ff4984..38ae4a5c54e 100644 --- a/packages/sdk-metrics/test/regression/histogram-recording-nan.test.ts +++ b/packages/sdk-metrics/test/regression/histogram-recording-nan.test.ts @@ -17,14 +17,14 @@ import * as assert from 'assert'; import { AggregationTemporality, - MeterProvider, - MetricReader, + AggregationType, DataPoint, ExponentialHistogram, Histogram, - AggregationType, + MeterProvider, } from '../../src'; import { TestMetricReader } from '../export/TestMetricReader'; +import { IMetricReader } from '../../src/export/MetricReader'; describe('histogram-recording-nan', () => { it('exponential histogram should not count NaN', async () => { @@ -91,7 +91,7 @@ describe('histogram-recording-nan', () => { ); }); - const collectNoErrors = async (reader: MetricReader) => { + const collectNoErrors = async (reader: IMetricReader) => { const { resourceMetrics, errors } = await reader.collect(); assert.strictEqual(errors.length, 0); return resourceMetrics; diff --git a/packages/sdk-metrics/test/regression/two-metric-readers-async-instrument.test.ts b/packages/sdk-metrics/test/regression/two-metric-readers-async-instrument.test.ts index 74913f9476b..242d0ee1716 100644 --- a/packages/sdk-metrics/test/regression/two-metric-readers-async-instrument.test.ts +++ b/packages/sdk-metrics/test/regression/two-metric-readers-async-instrument.test.ts @@ -15,9 +15,10 @@ */ import * as assert from 'assert'; -import { DataPointType, MeterProvider, MetricReader } from '../../src'; +import { DataPointType, MeterProvider } from '../../src'; import { TestDeltaMetricReader } from '../export/TestMetricReader'; import { assertDataPoint, assertMetricData } from '../util'; +import { IMetricReader } from '../../src/export/MetricReader'; // https://github.com/open-telemetry/opentelemetry-js/issues/3664 @@ -46,7 +47,7 @@ describe('two-metric-readers-async-instrument', () => { await assertCollection(reader1, 9); await assertCollection(reader2, 9); - async function assertCollection(reader: MetricReader, value: number) { + async function assertCollection(reader: IMetricReader, value: number) { const { errors, resourceMetrics } = await reader.collect(); assert.strictEqual(errors.length, 0); diff --git a/packages/sdk-metrics/test/state/MeterSharedState.test.ts b/packages/sdk-metrics/test/state/MeterSharedState.test.ts index 74b4ff9a273..e484a7a9fa9 100644 --- a/packages/sdk-metrics/test/state/MeterSharedState.test.ts +++ b/packages/sdk-metrics/test/state/MeterSharedState.test.ts @@ -19,7 +19,7 @@ import * as sinon from 'sinon'; import { MeterProvider, DataPointType, - MetricReader, + IMetricReader, InstrumentType, AggregationType, ViewOptions, @@ -44,7 +44,7 @@ describe('MeterSharedState', () => { }); describe('registerMetricStorage', () => { - function setupMeter(views?: ViewOptions[], readers?: MetricReader[]) { + function setupMeter(views?: ViewOptions[], readers?: IMetricReader[]) { const meterProvider = new MeterProvider({ resource: defaultResource, views,