From 4cfaa29dbc76950ed11f0adcf3dc240afbc92229 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Thu, 19 Dec 2024 12:20:03 -0800 Subject: [PATCH 1/8] Add check to ensure that some metrics are being exported and update tests. --- .../export/PeriodicExportingMetricReader.ts | 12 ++- .../PeriodicExportingMetricReader.test.ts | 96 ++++++++++++++----- 2 files changed, 79 insertions(+), 29 deletions(-) diff --git a/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts b/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts index 6a9096652d..567dde78db 100644 --- a/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts +++ b/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts @@ -135,11 +135,13 @@ export class PeriodicExportingMetricReader extends MetricReader { } } - const result = await internal._export(this._exporter, resourceMetrics); - if (result.code !== ExportResultCode.SUCCESS) { - throw new Error( - `PeriodicExportingMetricReader: metrics export failed (error ${result.error})` - ); + if (resourceMetrics.scopeMetrics.length > 0) { + const result = await internal._export(this._exporter, resourceMetrics); + if (result.code !== ExportResultCode.SUCCESS) { + throw new Error( + `PeriodicExportingMetricReader: metrics export failed (error ${result.error})` + ); + } } } diff --git a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts index 3294697874..29d12506ce 100644 --- a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts +++ b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts @@ -24,7 +24,11 @@ import { MetricProducer, PushMetricExporter, } from '../../src'; -import { ResourceMetrics } from '../../src/export/MetricData'; +import { + DataPointType, + ResourceMetrics, + ScopeMetrics, +} from '../../src/export/MetricData'; import * as assert from 'assert'; import * as sinon from 'sinon'; import { TimeoutError } from '../../src/utils'; @@ -34,7 +38,7 @@ import { setGlobalErrorHandler, } from '@opentelemetry/core'; import { assertRejects } from '../test-utils'; -import { emptyResourceMetrics, TestMetricProducer } from './TestMetricProducer'; +import { TestMetricProducer } from './TestMetricProducer'; import { assertAggregationSelector, assertAggregationTemporalitySelector, @@ -43,6 +47,7 @@ import { DEFAULT_AGGREGATION_SELECTOR, DEFAULT_AGGREGATION_TEMPORALITY_SELECTOR, } from '../../src/export/AggregationSelector'; +import { ValueType } from '@opentelemetry/api'; const MAX_32_BIT_INT = 2 ** 31 - 1; @@ -126,6 +131,51 @@ describe('PeriodicExportingMetricReader', () => { sinon.restore(); }); + const waitForAsyncAttributesStub = sinon.stub().returns( + new Promise(resolve => + setTimeout(() => { + resolve(); + }, 10) + ) + ); + const scopeMetrics: ScopeMetrics[] = [ + { + scope: { + name: 'test', + }, + metrics: [ + { + dataPointType: DataPointType.GAUGE, + dataPoints: [ + { + startTime: process.hrtime(), + endTime: process.hrtime(), + attributes: {}, + value: 1, + }, + ], + descriptor: { + name: '', + description: '', + unit: '', + type: InstrumentType.COUNTER, + valueType: ValueType.INT, + }, + aggregationTemporality: AggregationTemporality.CUMULATIVE, + }, + ], + }, + ]; + const resourceMetrics: ResourceMetrics = { + resource: { + attributes: {}, + merge: sinon.stub(), + asyncAttributesPending: true, // ensure we try to await async attributes + waitForAsyncAttributes: waitForAsyncAttributesStub, // resolve when awaited + }, + scopeMetrics: scopeMetrics, + }; + describe('constructor', () => { it('should construct PeriodicExportingMetricReader without exceptions', () => { const exporter = new TestDeltaMetricExporter(); @@ -203,13 +253,12 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 20, }); - reader.setMetricProducer(new TestMetricProducer()); + reader.setMetricProducer( + new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] }) + ); const result = await exporter.waitForNumberOfExports(2); - assert.deepStrictEqual(result, [ - emptyResourceMetrics, - emptyResourceMetrics, - ]); + assert.deepStrictEqual(result, [resourceMetrics, resourceMetrics]); await reader.shutdown(); }); }); @@ -224,13 +273,12 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 20, }); - reader.setMetricProducer(new TestMetricProducer()); + reader.setMetricProducer( + new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] }) + ); const result = await exporter.waitForNumberOfExports(2); - assert.deepStrictEqual(result, [ - emptyResourceMetrics, - emptyResourceMetrics, - ]); + assert.deepStrictEqual(result, [resourceMetrics, resourceMetrics]); exporter.throwExport = false; await reader.shutdown(); @@ -245,13 +293,12 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 20, }); - reader.setMetricProducer(new TestMetricProducer()); + reader.setMetricProducer( + new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] }) + ); const result = await exporter.waitForNumberOfExports(2); - assert.deepStrictEqual(result, [ - emptyResourceMetrics, - emptyResourceMetrics, - ]); + assert.deepStrictEqual(result, [resourceMetrics, resourceMetrics]); exporter.rejectExport = false; await reader.shutdown(); @@ -267,13 +314,12 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 20, }); - reader.setMetricProducer(new TestMetricProducer()); + reader.setMetricProducer( + new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] }) + ); const result = await exporter.waitForNumberOfExports(2); - assert.deepStrictEqual(result, [ - emptyResourceMetrics, - emptyResourceMetrics, - ]); + assert.deepStrictEqual(result, [resourceMetrics, resourceMetrics]); exporter.throwExport = false; await reader.shutdown(); @@ -295,7 +341,9 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 80, }); - reader.setMetricProducer(new TestMetricProducer()); + reader.setMetricProducer( + new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] }) + ); await reader.forceFlush(); exporterMock.verify(); @@ -321,7 +369,7 @@ describe('PeriodicExportingMetricReader', () => { asyncAttributesPending: true, // ensure we try to await async attributes waitForAsyncAttributes: waitForAsyncAttributesStub, // resolve when awaited }, - scopeMetrics: [], + scopeMetrics: scopeMetrics, }; const mockCollectionResult: CollectionResult = { From 5f97f1f21fabb32e5ad2ddfea492d2650fb6e327 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Thu, 19 Dec 2024 13:13:34 -0800 Subject: [PATCH 2/8] Add test and update changelog. --- CHANGELOG.md | 2 ++ .../PeriodicExportingMetricReader.test.ts | 17 +++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d2af8ed12..a117eb00e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :bug: (Bug Fix) +* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. []() @jacksonweber + ### :books: (Refine Doc) ### :house: (Internal) diff --git a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts index 29d12506ce..8ba6ab054a 100644 --- a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts +++ b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts @@ -263,6 +263,23 @@ describe('PeriodicExportingMetricReader', () => { }); }); + it('should not export without populated scope metrics', async () => { + const exporter = new TestMetricExporter(); + const reader = new PeriodicExportingMetricReader({ + exporter: exporter, + exportIntervalMillis: 30, + exportTimeoutMillis: 20, + }); + + reader.setMetricProducer( + new TestMetricProducer() + ); + const result = await exporter.forceFlush(); + + assert.deepStrictEqual(result, undefined); + await reader.shutdown(); + }); + describe('periodic export', () => { it('should keep running on export errors', async () => { const exporter = new TestMetricExporter(); From 8bdffdc698c3e9dfe1539d1e8680f06737a75c39 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Fri, 20 Dec 2024 10:36:53 -0800 Subject: [PATCH 3/8] Update PeriodicExportingMetricReader.test.ts --- .../test/export/PeriodicExportingMetricReader.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts index 8ba6ab054a..862e5344a2 100644 --- a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts +++ b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts @@ -271,9 +271,7 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 20, }); - reader.setMetricProducer( - new TestMetricProducer() - ); + reader.setMetricProducer(new TestMetricProducer()); const result = await exporter.forceFlush(); assert.deepStrictEqual(result, undefined); From 389fffb8a7f1e48fc92a92a25c71816a90ec6f4d Mon Sep 17 00:00:00 2001 From: Jackson Weber <47067795+JacksonWeber@users.noreply.github.com> Date: Fri, 20 Dec 2024 13:54:53 -0800 Subject: [PATCH 4/8] Update CHANGELOG.md Co-authored-by: Hector Hernandez <39923391+hectorhdzg@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a117eb00e9..73e2004f8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :bug: (Bug Fix) -* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. []() @jacksonweber +* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288]() @jacksonweber ### :books: (Refine Doc) From 37d196304990d2dfa5608cb8df6980bfb65e6c86 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Fri, 20 Dec 2024 13:55:43 -0800 Subject: [PATCH 5/8] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73e2004f8d..83ae8f07a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :bug: (Bug Fix) -* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288]() @jacksonweber +* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288](https://github.com/open-telemetry/opentelemetry-js/pull/5288) @jacksonweber ### :books: (Refine Doc) From 8f0c5c32e970df282064cef86e8d76e233be0796 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Fri, 20 Dec 2024 15:51:06 -0800 Subject: [PATCH 6/8] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83ae8f07a1..e1714bcd03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :bug: (Bug Fix) -* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288](https://github.com/open-telemetry/opentelemetry-js/pull/5288) @jacksonweber +* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288](https://github.com/open-telemetry/opentelemetry-js/pull/5288) @jacksonweber ### :books: (Refine Doc) From cd6c29fcadaec448080ae443d6f18aa745d47406 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Fri, 20 Dec 2024 15:51:23 -0800 Subject: [PATCH 7/8] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1714bcd03..83ae8f07a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se ### :bug: (Bug Fix) -* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288](https://github.com/open-telemetry/opentelemetry-js/pull/5288) @jacksonweber +* fix(sdk-metrics): do not export from `PeriodicExportingMetricReader` when there are no metrics to export. [#5288](https://github.com/open-telemetry/opentelemetry-js/pull/5288) @jacksonweber ### :books: (Refine Doc) From d639b6f2f69f7014861efddf381844af41b305c1 Mon Sep 17 00:00:00 2001 From: Jackson Weber Date: Tue, 24 Dec 2024 16:02:52 -0800 Subject: [PATCH 8/8] (fix): Remove node environment specific functions. --- .../test/export/PeriodicExportingMetricReader.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts index 862e5344a2..e3b622810e 100644 --- a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts +++ b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts @@ -148,8 +148,9 @@ describe('PeriodicExportingMetricReader', () => { dataPointType: DataPointType.GAUGE, dataPoints: [ { - startTime: process.hrtime(), - endTime: process.hrtime(), + // Sample hr time datapoints. + startTime: [12345, 678901234], + endTime: [12345, 678901234], attributes: {}, value: 1, },