From 0f6518dd63bf86f3149437b2aaa56c880d45ef6d Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Wed, 24 Jan 2024 14:30:06 +0100 Subject: [PATCH 1/3] feat(sdk-metrics): deprecate MeterProvider.addMetricReader() in favor of 'readers' constructor option (#4427) * feat(sdk-metrics): add 'readers' constructor option, deprecate MeterProvider.addMetricReader() * fix(changelog): update changelog entry, fix format --- CHANGELOG.md | 3 ++ .../test/metricsHelper.ts | 11 ++-- .../test/metricsHelper.ts | 13 +++-- .../test/metricsHelper.ts | 11 ++-- .../test/PrometheusExporter.test.ts | 10 ++-- .../test/PrometheusSerializer.test.ts | 18 +++---- .../test/functionals/http-metrics.test.ts | 3 +- .../opentelemetry-sdk-node/src/sdk.ts | 9 ++-- packages/sdk-metrics/src/MeterProvider.ts | 13 +++++ packages/sdk-metrics/test/Instruments.test.ts | 11 ++-- .../sdk-metrics/test/MeterProvider.test.ts | 54 +++++++++---------- .../test/export/ConsoleMetricExporter.test.ts | 14 ++--- .../export/InMemoryMetricExporter.test.ts | 20 +++---- .../test/export/MetricReader.test.ts | 25 +++++---- .../cumulative-exponential-histogram.test.ts | 5 +- ...wo-metric-readers-async-instrument.test.ts | 7 ++- .../test/state/MeterSharedState.test.ts | 12 ++--- .../test/state/MetricCollector.test.ts | 8 +-- 18 files changed, 141 insertions(+), 106 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 231d8c921b7..4fe3bc7c0ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :rocket: (Enhancement) +* feat(sdk-metrics): add constructor option to add metric readers [#4427](https://github.com/open-telemetry/opentelemetry-js/pull/4427) @pichlermarc + * deprecates `MeterProvider.addMetricReader()` please use the constructor option `readers` instead. + ### :bug: (Bug Fix) * fix(sdk-trace-base): ensure attribute value length limit is enforced on span creation [#4417](https://github.com/open-telemetry/opentelemetry-js/pull/4417) @pichlermarc diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts index 14c8f3a3df5..7c9fbfd99c0 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts @@ -55,10 +55,11 @@ const testResource = new Resource({ cost: 112.12, }); -let meterProvider = new MeterProvider({ resource: testResource }); - let reader = new TestMetricReader(); -meterProvider.addMetricReader(reader); +let meterProvider = new MeterProvider({ + resource: testResource, + readers: [reader], +}); let meter = meterProvider.getMeter('default', '0.0.1'); @@ -67,6 +68,7 @@ export async function collect() { } export function setUp() { + reader = new TestMetricReader(); meterProvider = new MeterProvider({ resource: testResource, views: [ @@ -75,9 +77,8 @@ export function setUp() { instrumentName: 'int-histogram', }), ], + readers: [reader], }); - reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); meter = meterProvider.getMeter('default', '0.0.1'); } diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts index 5605a69b280..850cdae4d0f 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts @@ -72,9 +72,11 @@ const defaultResource = Resource.default().merge( }) ); -let meterProvider = new MeterProvider({ resource: defaultResource }); let reader = new TestMetricReader(); -meterProvider.addMetricReader(reader); +let meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [reader], +}); let meter = meterProvider.getMeter('default', '0.0.1'); export async function collect() { @@ -82,9 +84,12 @@ export async function collect() { } export function setUp(views?: View[]) { - meterProvider = new MeterProvider({ resource: defaultResource, views }); reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); + meterProvider = new MeterProvider({ + resource: defaultResource, + views, + readers: [reader], + }); meter = meterProvider.getMeter('default', '0.0.1'); } diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts index 5c9c29abeee..ef70a75c054 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts @@ -54,10 +54,11 @@ const testResource = new Resource({ cost: 112.12, }); -let meterProvider = new MeterProvider({ resource: testResource }); - let reader = new TestMetricReader(); -meterProvider.addMetricReader(reader); +let meterProvider = new MeterProvider({ + resource: testResource, + readers: [reader], +}); let meter = meterProvider.getMeter('default', '0.0.1'); @@ -66,6 +67,7 @@ export async function collect() { } export function setUp() { + reader = new TestMetricReader(); meterProvider = new MeterProvider({ resource: testResource, views: [ @@ -74,9 +76,8 @@ export function setUp() { instrumentName: 'int-histogram', }), ], + readers: [reader], }); - reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); meter = meterProvider.getMeter('default', '0.0.1'); } diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts index 1030203a2ce..d5122406778 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts @@ -261,8 +261,9 @@ describe('PrometheusExporter', () => { beforeEach(done => { exporter = new PrometheusExporter({}, () => { - meterProvider = new MeterProvider(); - meterProvider.addMetricReader(exporter); + meterProvider = new MeterProvider({ + readers: [exporter], + }); meter = meterProvider.getMeter('test-prometheus', '1'); done(); }); @@ -533,8 +534,9 @@ describe('PrometheusExporter', () => { let exporter: PrometheusExporter; function setup(reader: PrometheusExporter) { - meterProvider = new MeterProvider(); - meterProvider.addMetricReader(reader); + meterProvider = new MeterProvider({ + readers: [exporter], + }); meter = meterProvider.getMeter('test-prometheus'); counter = meter.createCounter('counter'); diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts index 1a39aae0039..b2d9e36f8f9 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts @@ -91,8 +91,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const counter = meter.createCounter('test_total'); @@ -141,8 +141,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const histogram = meter.createHistogram('test'); @@ -206,8 +206,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const counter = meter.createCounter('test_total', { @@ -261,8 +261,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const counter = meter.createUpDownCounter('test_total', { @@ -315,8 +315,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const counter = meter.createUpDownCounter('test_total', { @@ -369,8 +369,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const histogram = meter.createHistogram('test', { @@ -424,8 +424,8 @@ describe('PrometheusSerializer', () => { instrumentName: '*', }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const upDownCounter = meter.createUpDownCounter('test', { @@ -474,8 +474,8 @@ describe('PrometheusSerializer', () => { views: [ new View({ aggregation: new SumAggregation(), instrumentName: '*' }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const { unit, exportAll = false } = options; @@ -563,8 +563,8 @@ describe('PrometheusSerializer', () => { views: [ new View({ aggregation: new SumAggregation(), instrumentName: '*' }), ], + readers: [reader], }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('test'); const counter = meter.createUpDownCounter(name); diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-metrics.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-metrics.test.ts index d7f55ed8463..0f484f08bf8 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-metrics.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-metrics.test.ts @@ -38,13 +38,12 @@ const protocol = 'http'; const hostname = 'localhost'; const pathname = '/test'; const tracerProvider = new NodeTracerProvider(); -const meterProvider = new MeterProvider(); const metricsMemoryExporter = new InMemoryMetricExporter( AggregationTemporality.DELTA ); const metricReader = new TestMetricReader(metricsMemoryExporter); +const meterProvider = new MeterProvider({ readers: [metricReader] }); -meterProvider.addMetricReader(metricReader); instrumentation.setTracerProvider(tracerProvider); instrumentation.setMeterProvider(meterProvider); diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index 30a1bea7901..6c8cedb296e 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -350,15 +350,16 @@ export class NodeSDK { } if (this._meterProviderConfig) { + const readers: MetricReader[] = []; + if (this._meterProviderConfig.reader) { + readers.push(this._meterProviderConfig.reader); + } const meterProvider = new MeterProvider({ resource: this._resource, views: this._meterProviderConfig?.views ?? [], + readers: readers, }); - if (this._meterProviderConfig.reader) { - meterProvider.addMetricReader(this._meterProviderConfig.reader); - } - this._meterProvider = meterProvider; metrics.setGlobalMeterProvider(meterProvider); diff --git a/packages/sdk-metrics/src/MeterProvider.ts b/packages/sdk-metrics/src/MeterProvider.ts index f10cf42b9b4..68d46057794 100644 --- a/packages/sdk-metrics/src/MeterProvider.ts +++ b/packages/sdk-metrics/src/MeterProvider.ts @@ -35,6 +35,7 @@ export interface MeterProviderOptions { /** Resource associated with metric telemetry */ resource?: IResource; views?: View[]; + readers?: MetricReader[]; } /** @@ -54,6 +55,12 @@ export class MeterProvider implements IMeterProvider { this._sharedState.viewRegistry.addView(view); } } + + if (options?.readers != null && options.readers.length > 0) { + for (const metricReader of options.readers) { + this.addMetricReader(metricReader); + } + } } /** @@ -77,7 +84,13 @@ export class MeterProvider implements IMeterProvider { * Register a {@link MetricReader} to the meter provider. After the * registration, the MetricReader can start metrics collection. * + *

NOTE: {@link MetricReader} instances MUST be added before creating any instruments. + * A {@link MetricReader} instance registered later may receive no or incomplete metric data. + * * @param metricReader the metric reader to be registered. + * + * @deprecated This method will be removed in SDK 2.0. Please use + * {@link MeterProviderOptions.readers} via the {@link MeterProvider} constructor instead */ addMetricReader(metricReader: MetricReader) { const collector = new MetricCollector(this._sharedState, metricReader); diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts index 56ecf03af67..ab466c295a1 100644 --- a/packages/sdk-metrics/test/Instruments.test.ts +++ b/packages/sdk-metrics/test/Instruments.test.ts @@ -751,7 +751,12 @@ describe('Instruments', () => { }); function setup() { - const meterProvider = new MeterProvider({ resource: defaultResource }); + const deltaReader = new TestDeltaMetricReader(); + const cumulativeReader = new TestMetricReader(); + const meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [deltaReader, cumulativeReader], + }); const meter = meterProvider.getMeter( defaultInstrumentationScope.name, defaultInstrumentationScope.version, @@ -759,10 +764,6 @@ function setup() { schemaUrl: defaultInstrumentationScope.schemaUrl, } ); - const deltaReader = new TestDeltaMetricReader(); - meterProvider.addMetricReader(deltaReader); - const cumulativeReader = new TestMetricReader(); - meterProvider.addMetricReader(cumulativeReader); return { meterProvider, diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index be075f0fa61..615fcffab30 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -73,9 +73,11 @@ describe('MeterProvider', () => { }); it('get meter with same identity', async () => { - const meterProvider = new MeterProvider({ resource: defaultResource }); const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); + const meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [reader], + }); // Create meter and instrument, needs observation on instrument, otherwise the scope will not be reported. // name+version pair 1 @@ -131,6 +133,7 @@ describe('MeterProvider', () => { describe('addView', () => { it('with existing instrument should rename', async () => { + const reader = new TestMetricReader(); const meterProvider = new MeterProvider({ resource: defaultResource, // Add view to rename 'non-renamed-instrument' to 'renamed-instrument' @@ -141,11 +144,9 @@ describe('MeterProvider', () => { instrumentName: 'non-renamed-instrument', }), ], + readers: [reader], }); - const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); - // Create meter and instrument. const myMeter = meterProvider.getMeter('meter1', 'v1.0.0'); const counter = myMeter.createCounter('non-renamed-instrument'); @@ -200,6 +201,8 @@ describe('MeterProvider', () => { }); it('with attributeKeys should drop non-listed attributes', async () => { + const reader = new TestMetricReader(); + // Add view to drop all attributes except 'attrib1' const meterProvider = new MeterProvider({ resource: defaultResource, @@ -209,11 +212,9 @@ describe('MeterProvider', () => { instrumentName: 'non-renamed-instrument', }), ], + readers: [reader], }); - const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); - // Create meter and instrument. const myMeter = meterProvider.getMeter('meter1', 'v1.0.0'); const counter = myMeter.createCounter('non-renamed-instrument'); @@ -266,6 +267,8 @@ describe('MeterProvider', () => { }); it('with no meter name should apply view to instruments of all meters', async () => { + const reader = new TestMetricReader(); + // Add view that renames 'test-counter' to 'renamed-instrument' const meterProvider = new MeterProvider({ resource: defaultResource, @@ -275,11 +278,9 @@ describe('MeterProvider', () => { instrumentName: 'test-counter', }), ], + readers: [reader], }); - const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); - // Create two meters. const meter1 = meterProvider.getMeter('meter1', 'v1.0.0'); const meter2 = meterProvider.getMeter('meter2', 'v1.0.0'); @@ -339,6 +340,7 @@ describe('MeterProvider', () => { }); it('with meter name should apply view to only the selected meter', async () => { + const reader = new TestMetricReader(); const meterProvider = new MeterProvider({ resource: defaultResource, views: [ @@ -349,11 +351,9 @@ describe('MeterProvider', () => { meterName: 'meter1', }), ], + readers: [reader], }); - const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); - // Create two meters. const meter1 = meterProvider.getMeter('meter1', 'v1.0.0'); const meter2 = meterProvider.getMeter('meter2', 'v1.0.0'); @@ -413,6 +413,7 @@ describe('MeterProvider', () => { }); it('with different instrument types does not throw', async () => { + const reader = new TestMetricReader(); const meterProvider = new MeterProvider({ resource: defaultResource, // Add Views to rename both instruments (of different types) to the same name. @@ -428,9 +429,8 @@ describe('MeterProvider', () => { meterName: 'meter1', }), ], + readers: [reader], }); - const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); // Create meter and instruments. const meter = meterProvider.getMeter('meter1', 'v1.0.0'); @@ -481,6 +481,8 @@ describe('MeterProvider', () => { const msBoundaries = [0, 1, 2, 3, 4, 5]; const sBoundaries = [10, 50, 250, 1000]; + const reader = new TestMetricReader(); + const meterProvider = new MeterProvider({ resource: defaultResource, views: [ @@ -493,11 +495,9 @@ describe('MeterProvider', () => { aggregation: new ExplicitBucketHistogramAggregation(sBoundaries), }), ], + readers: [reader], }); - 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', { @@ -543,14 +543,14 @@ describe('MeterProvider', () => { describe('shutdown', () => { it('should shutdown all registered metric readers', async () => { - const meterProvider = new MeterProvider({ resource: defaultResource }); const reader1 = new TestMetricReader(); const reader2 = new TestMetricReader(); const reader1ShutdownSpy = sinon.spy(reader1, 'shutdown'); const reader2ShutdownSpy = sinon.spy(reader2, 'shutdown'); - - meterProvider.addMetricReader(reader1); - meterProvider.addMetricReader(reader2); + const meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [reader1, reader2], + }); await meterProvider.shutdown({ timeoutMillis: 1234 }); await meterProvider.shutdown(); @@ -569,14 +569,14 @@ describe('MeterProvider', () => { describe('forceFlush', () => { it('should forceFlush all registered metric readers', async () => { - const meterProvider = new MeterProvider({ resource: defaultResource }); const reader1 = new TestMetricReader(); const reader2 = new TestMetricReader(); const reader1ForceFlushSpy = sinon.spy(reader1, 'forceFlush'); const reader2ForceFlushSpy = sinon.spy(reader2, 'forceFlush'); - - meterProvider.addMetricReader(reader1); - meterProvider.addMetricReader(reader2); + const meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [reader1, reader2], + }); await meterProvider.forceFlush({ timeoutMillis: 1234 }); await meterProvider.forceFlush({ timeoutMillis: 5678 }); diff --git a/packages/sdk-metrics/test/export/ConsoleMetricExporter.test.ts b/packages/sdk-metrics/test/export/ConsoleMetricExporter.test.ts index fe46fa9f71e..6409f6aa04a 100644 --- a/packages/sdk-metrics/test/export/ConsoleMetricExporter.test.ts +++ b/packages/sdk-metrics/test/export/ConsoleMetricExporter.test.ts @@ -50,7 +50,7 @@ describe('ConsoleMetricExporter', () => { let previousConsoleDir: any; let exporter: ConsoleMetricExporter; let meterProvider: MeterProvider; - let meterReader: PeriodicExportingMetricReader; + let metricReader: PeriodicExportingMetricReader; let meter: metrics.Meter; beforeEach(() => { @@ -58,20 +58,22 @@ describe('ConsoleMetricExporter', () => { console.dir = () => {}; exporter = new ConsoleMetricExporter(); - meterProvider = new MeterProvider({ resource: defaultResource }); - meter = meterProvider.getMeter('ConsoleMetricExporter', '1.0.0'); - meterReader = new PeriodicExportingMetricReader({ + metricReader = new PeriodicExportingMetricReader({ exporter: exporter, exportIntervalMillis: 100, exportTimeoutMillis: 100, }); - meterProvider.addMetricReader(meterReader); + meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [metricReader], + }); + meter = meterProvider.getMeter('ConsoleMetricExporter', '1.0.0'); }); afterEach(async () => { console.dir = previousConsoleDir; - await meterReader.shutdown(); + await metricReader.shutdown(); }); it('should export information about metric', async () => { diff --git a/packages/sdk-metrics/test/export/InMemoryMetricExporter.test.ts b/packages/sdk-metrics/test/export/InMemoryMetricExporter.test.ts index 38a3f985869..1e14a8ed5e1 100644 --- a/packages/sdk-metrics/test/export/InMemoryMetricExporter.test.ts +++ b/packages/sdk-metrics/test/export/InMemoryMetricExporter.test.ts @@ -45,24 +45,26 @@ async function waitForNumberOfExports( describe('InMemoryMetricExporter', () => { let exporter: InMemoryMetricExporter; let meterProvider: MeterProvider; - let meterReader: PeriodicExportingMetricReader; + let metricReader: PeriodicExportingMetricReader; let meter: metrics.Meter; beforeEach(() => { exporter = new InMemoryMetricExporter(AggregationTemporality.CUMULATIVE); - meterProvider = new MeterProvider({ resource: defaultResource }); - meter = meterProvider.getMeter('InMemoryMetricExporter', '1.0.0'); - meterReader = new PeriodicExportingMetricReader({ + metricReader = new PeriodicExportingMetricReader({ exporter: exporter, exportIntervalMillis: 100, exportTimeoutMillis: 100, }); - meterProvider.addMetricReader(meterReader); + meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [metricReader], + }); + meter = meterProvider.getMeter('InMemoryMetricExporter', '1.0.0'); }); afterEach(async () => { await exporter.shutdown(); - await meterReader.shutdown(); + await metricReader.shutdown(); }); it('should return failed result code', done => { @@ -85,7 +87,7 @@ describe('InMemoryMetricExporter', () => { }; exporter.export(resourceMetrics, result => { assert.ok(result.code === ExportResultCode.FAILED); - meterReader.shutdown().then(() => { + metricReader.shutdown().then(() => { done(); }); }); @@ -108,7 +110,7 @@ describe('InMemoryMetricExporter', () => { assert.ok(otherMetrics.length === 0); await exporter.shutdown(); - await meterReader.shutdown(); + await metricReader.shutdown(); }); it('should be able to access metric', async () => { @@ -146,6 +148,6 @@ describe('InMemoryMetricExporter', () => { const histogramDataPoint = histogramMetric.dataPoints.shift(); assert.ok(histogramDataPoint); - await meterReader.shutdown(); + await metricReader.shutdown(); }); }); diff --git a/packages/sdk-metrics/test/export/MetricReader.test.ts b/packages/sdk-metrics/test/export/MetricReader.test.ts index c0643a60da9..b08cde5e38d 100644 --- a/packages/sdk-metrics/test/export/MetricReader.test.ts +++ b/packages/sdk-metrics/test/export/MetricReader.test.ts @@ -73,16 +73,18 @@ describe('MetricReader', () => { describe('setMetricProducer', () => { it('The SDK MUST NOT allow a MetricReader instance to be registered on more than one MeterProvider instance', () => { const reader = new TestMetricReader(); - const meterProvider1 = new MeterProvider(); - const meterProvider2 = new MeterProvider(); - - meterProvider1.addMetricReader(reader); assert.throws( - () => meterProvider1.addMetricReader(reader), + () => + new MeterProvider({ + readers: [reader, reader], + }), /MetricReader can not be bound to a MeterProvider again/ ); assert.throws( - () => meterProvider2.addMetricReader(reader), + () => + new MeterProvider({ + readers: [reader], + }), /MetricReader can not be bound to a MeterProvider again/ ); }); @@ -138,7 +140,6 @@ describe('MetricReader', () => { }); it('should collect metrics from the SDK and the additional metricProducers', async () => { - const meterProvider = new MeterProvider({ resource: defaultResource }); const additionalProducer = new TestMetricProducer({ resourceMetrics: { resource: new Resource({ @@ -150,7 +151,10 @@ describe('MetricReader', () => { const reader = new TestMetricReader({ metricProducers: [additionalProducer], }); - meterProvider.addMetricReader(reader); + const meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [reader], + }); // Make a measurement meterProvider @@ -182,14 +186,15 @@ describe('MetricReader', () => { }); it('should merge the errors from the SDK and all metricProducers', async () => { - const meterProvider = new MeterProvider(); const reader = new TestMetricReader({ metricProducers: [ new TestMetricProducer({ errors: ['err1'] }), new TestMetricProducer({ errors: ['err2'] }), ], }); - meterProvider.addMetricReader(reader); + const meterProvider = new MeterProvider({ + readers: [reader], + }); // Provide a callback throwing an error too meterProvider 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 79dcfc434f0..2462ddf5c81 100644 --- a/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts +++ b/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts @@ -46,7 +46,6 @@ describe('cumulative-exponential-histogram', () => { }); const doTest = async (histogramAggregation: Aggregation) => { - const meterProvider = new MeterProvider(); const reader = new TestMetricReader({ aggregationTemporalitySelector() { return AggregationTemporality.CUMULATIVE; @@ -57,8 +56,10 @@ describe('cumulative-exponential-histogram', () => { : Aggregation.Default(); }, }); + const meterProvider = new MeterProvider({ + readers: [reader], + }); - meterProvider.addMetricReader(reader); const meter = meterProvider.getMeter('my-meter'); const hist = meter.createHistogram('testhist'); 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 a1301be0ecc..74913f9476b 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 @@ -23,12 +23,11 @@ import { assertDataPoint, assertMetricData } from '../util'; describe('two-metric-readers-async-instrument', () => { it('both metric readers should collect metrics', async () => { - const meterProvider = new MeterProvider(); const reader1 = new TestDeltaMetricReader(); const reader2 = new TestDeltaMetricReader(); - - meterProvider.addMetricReader(reader1); - meterProvider.addMetricReader(reader2); + const meterProvider = new MeterProvider({ + readers: [reader1, reader2], + }); const meter = meterProvider.getMeter('my-meter'); diff --git a/packages/sdk-metrics/test/state/MeterSharedState.test.ts b/packages/sdk-metrics/test/state/MeterSharedState.test.ts index 55f7fe5386c..9e814a4b439 100644 --- a/packages/sdk-metrics/test/state/MeterSharedState.test.ts +++ b/packages/sdk-metrics/test/state/MeterSharedState.test.ts @@ -48,8 +48,8 @@ describe('MeterSharedState', () => { const meterProvider = new MeterProvider({ resource: defaultResource, views, + readers: readers, }); - readers?.forEach(reader => meterProvider.addMetricReader(reader)); const meter = meterProvider.getMeter('test-meter'); @@ -185,19 +185,17 @@ describe('MeterSharedState', () => { describe('collect', () => { function setupInstruments(views?: View[]) { + const cumulativeReader = new TestMetricReader(); + const deltaReader = new TestDeltaMetricReader(); + const meterProvider = new MeterProvider({ resource: defaultResource, views: views, + readers: [cumulativeReader, deltaReader], }); - const cumulativeReader = new TestMetricReader(); - meterProvider.addMetricReader(cumulativeReader); const cumulativeCollector = cumulativeReader.getMetricCollector(); - - const deltaReader = new TestDeltaMetricReader(); - meterProvider.addMetricReader(deltaReader); const deltaCollector = deltaReader.getMetricCollector(); - const metricCollectors = [cumulativeCollector, deltaCollector]; const meter = meterProvider.getMeter( diff --git a/packages/sdk-metrics/test/state/MetricCollector.test.ts b/packages/sdk-metrics/test/state/MetricCollector.test.ts index 8466a5a95cc..f173a48446d 100644 --- a/packages/sdk-metrics/test/state/MetricCollector.test.ts +++ b/packages/sdk-metrics/test/state/MetricCollector.test.ts @@ -55,10 +55,12 @@ describe('MetricCollector', () => { describe('collect', () => { function setupInstruments() { - const meterProvider = new MeterProvider({ resource: defaultResource }); - const reader = new TestMetricReader(); - meterProvider.addMetricReader(reader); + const meterProvider = new MeterProvider({ + resource: defaultResource, + readers: [reader], + }); + const metricCollector = reader.getMetricCollector(); const meter = meterProvider.getMeter( From 0635ab1c6c21ab7308bf75d399002eedf416b23b Mon Sep 17 00:00:00 2001 From: Zirak Date: Wed, 24 Jan 2024 14:57:13 +0000 Subject: [PATCH 2/3] fix(sdk-trace-base): Export processed spans while exporter failed (#4287) * fix(sdk-trace-base): Export processed spans while exporter failed While the exporter deals with a batch of spans, new spans may come in and wait to be exported. As previously implemented, a successful export would notice these waiting spans, triggering a renewed timer check, but not so for an unsuccessful export. The result was that, prior to this commit, a failing export may end up in a situation where no further spans will be exported. This is due to the behaviour of `_addToBuffer` when the queue is full: Imagine an export which fails after a long timeout (because of, for instance, network troubles). While the connection waits to be timed out, the span queue fills up. Once completely full, no new calls to recheck the timer will be done. On its own, this behaviour is fine. When combined with the patched bug, this leads to a rather confusing case where the exporter never tries exporting. * fix(changelog): add entry --------- Co-authored-by: Marc Pichler --- CHANGELOG.md | 1 + .../src/export/BatchSpanProcessorBase.ts | 2 +- .../export/BatchSpanProcessorBase.test.ts | 51 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe3bc7c0ff..bb6489a66f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :bug: (Bug Fix) * fix(sdk-trace-base): ensure attribute value length limit is enforced on span creation [#4417](https://github.com/open-telemetry/opentelemetry-js/pull/4417) @pichlermarc +* fix(sdk-trace-base): Export processed spans while exporter failed [#4287](https://github.com/open-telemetry/opentelemetry-js/pull/4287) @Zirak ### :books: (Refine Doc) diff --git a/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts b/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts index fef5c80dc0f..f069aac0c7d 100644 --- a/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts +++ b/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts @@ -221,7 +221,7 @@ export abstract class BatchSpanProcessorBase const flush = () => { this._isExporting = true; this._flushOneBatch() - .then(() => { + .finally(() => { this._isExporting = false; if (this._finishedSpans.length > 0) { this._clearTimer(); diff --git a/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts index 83fb3ebe44f..251971385c1 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts @@ -387,6 +387,57 @@ describe('BatchSpanProcessorBase', () => { }); }); + it('should still export when previously failed', async () => { + // The scenario is made of several parts: + // 1. The exporter tries to export some spans + // 2. While it does so, more spans are processed + // 3. The exporter fails + // 4. Spans arriving during step 2 should be exported + + let firstCall = true; + const fillingExportStub = sinon + .stub(exporter, 'export') + .callsFake((spans, cb) => { + // The first time export is called, add some spans to the processor. + // Any other time, call through. We don't simply restore the stub + // so we can count the calls with `sinon.assert` + if (!firstCall) { + return fillingExportStub.wrappedMethod.call(exporter, spans, cb); + } + + // Step 2: During export, add another span + firstCall = false; + processSpan(); + + return fillingExportStub.wrappedMethod.call(exporter, spans, () => { + // Step 3: Mock failure + cb({ + code: ExportResultCode.FAILED, + }); + }); + }); + + const clock = sinon.useFakeTimers(); + + // Step 1: Export a span + processSpan(); + await clock.runAllAsync(); + + clock.restore(); + fillingExportStub.restore(); + + // Step 4: Make sure all spans were processed + assert.equal(exporter['_finishedSpans'].length, 2); + assert.equal(processor['_finishedSpans'].length, 0); + sinon.assert.calledTwice(fillingExportStub); + + function processSpan() { + const span = createSampledSpan('test'); + processor.onStart(span, ROOT_CONTEXT); + processor.onEnd(span); + } + }); + it('should wait for pending resource on flush', async () => { const tracer = new BasicTracerProvider({ resource: new Resource( From 8648313ed83bacc34f56fbec4772413cd83ea463 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 25 Jan 2024 08:20:47 +0100 Subject: [PATCH 3/3] fix(instrumentation)!: pin import-in-the-middle@1.7.1 (#4441) * fix(instrumentation): pin import-in-the-middle@1.7.1 * fix(changelog): add changlog entry * Update experimental/CHANGELOG.md Co-authored-by: Trent Mick * fix(changelog): additional details * fix(changelog): formatting * fix(changelog): lint --------- Co-authored-by: Trent Mick --- experimental/CHANGELOG.md | 6 +++ .../package.json | 2 +- package-lock.json | 48 +++++++++---------- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 21e254222e7..2501ed37ea3 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -6,6 +6,12 @@ All notable changes to experimental packages in this project will be documented ### :boom: Breaking Change +* fix(instrumentation)!: pin import-in-the-middle@1.7.1 [#4441](https://github.com/open-telemetry/opentelemetry-js/pull/4441) + * Fixes a bug where, in some circumstances, ESM instrumentation packages would try to instrument CJS exports on ESM, causing the end-user application to crash. + * This breaking change only affects users that are using the *experimental* `@opentelemetry/instrumentation/hook.mjs` loader hook AND Node.js 18.19 or later: + * This reverts back to an older version of `import-in-the-middle` due to + * This version does not support Node.js 18.19 or later + ### :rocket: (Enhancement) ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-instrumentation/package.json b/experimental/packages/opentelemetry-instrumentation/package.json index 3724bf7ca3d..c94165c1fe4 100644 --- a/experimental/packages/opentelemetry-instrumentation/package.json +++ b/experimental/packages/opentelemetry-instrumentation/package.json @@ -72,7 +72,7 @@ }, "dependencies": { "@types/shimmer": "^1.0.2", - "import-in-the-middle": "^1.7.2", + "import-in-the-middle": "1.7.1", "require-in-the-middle": "^7.1.1", "semver": "^7.5.2", "shimmer": "^1.2.1" diff --git a/package-lock.json b/package-lock.json index 2524eca71ef..237f56e95f1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3108,7 +3108,7 @@ "license": "Apache-2.0", "dependencies": { "@types/shimmer": "^1.0.2", - "import-in-the-middle": "^1.7.2", + "import-in-the-middle": "1.7.1", "require-in-the-middle": "^7.1.1", "semver": "^7.5.2", "shimmer": "^1.2.1" @@ -3948,6 +3948,17 @@ "node": ">=10.13.0" } }, + "experimental/packages/opentelemetry-instrumentation/node_modules/import-in-the-middle": { + "version": "1.7.1", + "resolved": "https://registry.npmjs.org/import-in-the-middle/-/import-in-the-middle-1.7.1.tgz", + "integrity": "sha512-1LrZPDtW+atAxH42S6288qyDFNQ2YCty+2mxEPRtfazH6Z5QwkaBSTS2ods7hnVJioF6rkRfNoA6A/MstpFXLg==", + "dependencies": { + "acorn": "^8.8.2", + "acorn-import-assertions": "^1.9.0", + "cjs-module-lexer": "^1.2.2", + "module-details-from-path": "^1.0.3" + } + }, "experimental/packages/opentelemetry-instrumentation/node_modules/interpret": { "version": "3.1.1", "dev": true, @@ -19486,17 +19497,6 @@ "node": ">=4" } }, - "node_modules/import-in-the-middle": { - "version": "1.7.2", - "resolved": "https://registry.npmjs.org/import-in-the-middle/-/import-in-the-middle-1.7.2.tgz", - "integrity": "sha512-coz7AjRnPyKW36J6JX5Bjz1mcX7MX1H2XsEGseVcnXMdzsAbbAu0HBZhiAem+3SAmuZdi+p8OwoB2qUpTRgjOQ==", - "dependencies": { - "acorn": "^8.8.2", - "acorn-import-assertions": "^1.9.0", - "cjs-module-lexer": "^1.2.2", - "module-details-from-path": "^1.0.3" - } - }, "node_modules/import-local": { "version": "3.1.0", "dev": true, @@ -40348,7 +40348,7 @@ "codecov": "3.8.3", "cpx": "1.5.0", "cross-var": "1.1.0", - "import-in-the-middle": "^1.7.2", + "import-in-the-middle": "1.7.1", "karma": "6.4.2", "karma-chrome-launcher": "3.1.0", "karma-coverage": "2.2.1", @@ -40406,6 +40406,17 @@ "tapable": "^2.2.0" } }, + "import-in-the-middle": { + "version": "1.7.1", + "resolved": "https://registry.npmjs.org/import-in-the-middle/-/import-in-the-middle-1.7.1.tgz", + "integrity": "sha512-1LrZPDtW+atAxH42S6288qyDFNQ2YCty+2mxEPRtfazH6Z5QwkaBSTS2ods7hnVJioF6rkRfNoA6A/MstpFXLg==", + "requires": { + "acorn": "^8.8.2", + "acorn-import-assertions": "^1.9.0", + "cjs-module-lexer": "^1.2.2", + "module-details-from-path": "^1.0.3" + } + }, "interpret": { "version": "3.1.1", "dev": true @@ -49648,17 +49659,6 @@ } } }, - "import-in-the-middle": { - "version": "1.7.2", - "resolved": "https://registry.npmjs.org/import-in-the-middle/-/import-in-the-middle-1.7.2.tgz", - "integrity": "sha512-coz7AjRnPyKW36J6JX5Bjz1mcX7MX1H2XsEGseVcnXMdzsAbbAu0HBZhiAem+3SAmuZdi+p8OwoB2qUpTRgjOQ==", - "requires": { - "acorn": "^8.8.2", - "acorn-import-assertions": "^1.9.0", - "cjs-module-lexer": "^1.2.2", - "module-details-from-path": "^1.0.3" - } - }, "import-local": { "version": "3.1.0", "dev": true,