From b3d7030a79b024f3c12947206483e05b19056a10 Mon Sep 17 00:00:00 2001 From: Michele Azzolari Date: Sat, 7 Dec 2024 10:40:15 +0100 Subject: [PATCH 1/5] test(sdk-metrics): tests: add missing 'await' when calling 'meterProvider.shutdown()' --- packages/sdk-metrics/test/MeterProvider.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index cc534adcb1d..453945200d4 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -126,9 +126,9 @@ describe('MeterProvider', () => { assert.strictEqual(meter1, meter2); }); - it('get a noop meter on shutdown', () => { + it('get a noop meter on shutdown', async () => { const meterProvider = new MeterProvider(); - meterProvider.shutdown(); + await meterProvider.shutdown(); const meter = meterProvider.getMeter('meter1', '1.0.0'); // returned tracer should be no-op, not instance of Meter (from SDK) assert.ok(!(meter instanceof Meter)); From 8bce3f5239a2901bc39fdfd3e6f29a031ba2e615 Mon Sep 17 00:00:00 2001 From: Michele Azzolari Date: Sat, 7 Dec 2024 10:48:41 +0100 Subject: [PATCH 2/5] chore(sdk-metrics): align shutdown implementation to the function description, calling forceFlush() this also aligns the behavior with the other signals providers: calling sdk.shutdown() will now flush all pending signals --- packages/sdk-metrics/src/MeterProvider.ts | 7 +++++++ packages/sdk-metrics/test/MeterProvider.test.ts | 7 ++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/sdk-metrics/src/MeterProvider.ts b/packages/sdk-metrics/src/MeterProvider.ts index 962d59b4290..ab727abb103 100644 --- a/packages/sdk-metrics/src/MeterProvider.ts +++ b/packages/sdk-metrics/src/MeterProvider.ts @@ -133,6 +133,13 @@ export class MeterProvider implements IMeterProvider { return; } + try { + await this.forceFlush(); + } catch (e) { + // log the error and continue with shutdown + diag.warn('error flushing data during shutdown', e); + } + this._shutdown = true; await Promise.all( diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index 453945200d4..eff71824fc9 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -835,10 +835,11 @@ describe('MeterProvider', () => { timeoutMillis: 5678, }); - await meterProvider.shutdown(); + await meterProvider.shutdown(); // <-- this will call forceFlush() internally await meterProvider.forceFlush(); - assert.strictEqual(reader1ForceFlushSpy.callCount, 2); - assert.strictEqual(reader2ForceFlushSpy.callCount, 2); + assert.strictEqual(reader1ForceFlushSpy.callCount, 3); + assert.strictEqual(reader2ForceFlushSpy.callCount, 3); + }); }); }); From d67269f9b09f5e4f8f393e377237c385716218e6 Mon Sep 17 00:00:00 2001 From: Michele Azzolari Date: Mon, 9 Dec 2024 15:29:13 +0100 Subject: [PATCH 3/5] fixup! chore(sdk-metrics): align shutdown implementation to the function description, calling forceFlush() --- packages/sdk-metrics/src/MeterProvider.ts | 9 +-------- .../src/export/PeriodicExportingMetricReader.ts | 2 +- packages/sdk-metrics/test/MeterProvider.test.ts | 7 +++---- .../test/export/PeriodicExportingMetricReader.test.ts | 2 -- 4 files changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/sdk-metrics/src/MeterProvider.ts b/packages/sdk-metrics/src/MeterProvider.ts index ab727abb103..6d5cd56fd03 100644 --- a/packages/sdk-metrics/src/MeterProvider.ts +++ b/packages/sdk-metrics/src/MeterProvider.ts @@ -122,7 +122,7 @@ export class MeterProvider implements IMeterProvider { } /** - * Flush all buffered data and shut down the MeterProvider and all registered + * Shut down the MeterProvider and all registered * MetricReaders. * * Returns a promise which is resolved when all flushes are complete. @@ -133,13 +133,6 @@ export class MeterProvider implements IMeterProvider { return; } - try { - await this.forceFlush(); - } catch (e) { - // log the error and continue with shutdown - diag.warn('error flushing data during shutdown', e); - } - this._shutdown = true; await Promise.all( diff --git a/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts b/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts index 646c832aa41..6a9096652d7 100644 --- a/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts +++ b/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts @@ -161,7 +161,7 @@ export class PeriodicExportingMetricReader extends MetricReader { if (this._interval) { clearInterval(this._interval); } - + await this.onForceFlush(); await this._exporter.shutdown(); } } diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index eff71824fc9..453945200d4 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -835,11 +835,10 @@ describe('MeterProvider', () => { timeoutMillis: 5678, }); - await meterProvider.shutdown(); // <-- this will call forceFlush() internally + await meterProvider.shutdown(); await meterProvider.forceFlush(); - assert.strictEqual(reader1ForceFlushSpy.callCount, 3); - assert.strictEqual(reader2ForceFlushSpy.callCount, 3); - + assert.strictEqual(reader1ForceFlushSpy.callCount, 2); + assert.strictEqual(reader2ForceFlushSpy.callCount, 2); }); }); }); diff --git a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts index 9210f4622f3..1fe248cab84 100644 --- a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts +++ b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts @@ -77,9 +77,7 @@ class TestMetricExporter implements PushMetricExporter { async shutdown(): Promise { if (this._shutdown) return; - const flushPromise = this.forceFlush(); this._shutdown = true; - await flushPromise; } async forceFlush(): Promise { From 4ef3b38e877eba9f411ab6ee38b9115a2a81db9d Mon Sep 17 00:00:00 2001 From: Michele Azzolari Date: Mon, 9 Dec 2024 18:50:16 +0100 Subject: [PATCH 4/5] test(opentelemetry-exporter-metrics-otlp-http): call 'meterProvider.shutdown()' only after 'meterProvider.forceFlush()' resolves --- .../test/node/OTLPMetricExporter.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/node/OTLPMetricExporter.test.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/node/OTLPMetricExporter.test.ts index 87569f327a3..04d3228dee5 100644 --- a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/node/OTLPMetricExporter.test.ts +++ b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/node/OTLPMetricExporter.test.ts @@ -214,8 +214,9 @@ describe('OTLPMetricExporter', () => { meterProvider.getMeter('test-meter').createCounter('test-counter').add(1); // act - meterProvider.forceFlush(); - meterProvider.shutdown(); + meterProvider.forceFlush().then(() => { + return meterProvider.shutdown(); + }); }); }); }); From a2d7810a5cf35125d9528806bd4537ad7370e480 Mon Sep 17 00:00:00 2001 From: Michele Azzolari Date: Tue, 10 Dec 2024 12:23:43 +0100 Subject: [PATCH 5/5] Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f378c59c12..81905d18fb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :rocket: (Enhancement) +* feat(sdk-metrics): PeriodicExportingMetricReader now flushes pending tasks at shutdown [#5242](https://github.com/open-telemetry/opentelemetry-js/pull/5242) + ### :bug: (Bug Fix) * fix(sdk-trace-base): do not load OTEL_ env vars on module load, but when needed [#5224](https://github.com/open-telemetry/opentelemetry-js/pull/5224)