From 18b33a0a1d40703cc42ae5c91025b047d8f32ad9 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 13 Jan 2025 17:23:45 +0100 Subject: [PATCH] [backport/v1.x] fix(sdk-metrics): don't export from PeriodicExportingMetricReader with no metrics (#5288) (#5340) Co-authored-by: Jackson Weber <47067795+JacksonWeber@users.noreply.github.com> Co-authored-by: Hector Hernandez <39923391+hectorhdzg@users.noreply.github.com> --- CHANGELOG.md | 2 + .../export/PeriodicExportingMetricReader.ts | 4 + .../PeriodicExportingMetricReader.test.ts | 112 ++++++++++++++---- 3 files changed, 94 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e572234db4..bc4e749ae19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,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. [#5288](https://github.com/open-telemetry/opentelemetry-js/pull/5288) @jacksonweber + ### :books: (Refine Doc) ### :house: (Internal) diff --git a/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts b/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts index 6a9096652d7..6968bb3e55b 100644 --- a/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts +++ b/packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts @@ -135,6 +135,10 @@ export class PeriodicExportingMetricReader extends MetricReader { } } + if (resourceMetrics.scopeMetrics.length === 0) { + return; + } + const result = await internal._export(this._exporter, resourceMetrics); if (result.code !== ExportResultCode.SUCCESS) { throw new Error( diff --git a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts index 1fe248cab84..e6eee47e63a 100644 --- a/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts +++ b/packages/sdk-metrics/test/export/PeriodicExportingMetricReader.test.ts @@ -23,7 +23,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'; @@ -33,7 +37,7 @@ import { setGlobalErrorHandler, } from '@opentelemetry/core'; import { assertRejects } from '../test-utils'; -import { emptyResourceMetrics, TestMetricProducer } from './TestMetricProducer'; +import { TestMetricProducer } from './TestMetricProducer'; import { assertAggregationSelector, assertAggregationTemporalitySelector, @@ -42,6 +46,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; @@ -125,6 +130,52 @@ 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: [ + { + // Sample hr time datapoints. + startTime: [12345, 678901234], + endTime: [12345, 678901234], + attributes: {}, + value: 1, + }, + ], + descriptor: { + name: '', + description: '', + type: InstrumentType.GAUGE, + unit: '', + 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(); @@ -202,17 +253,31 @@ 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(); }); }); + 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(); @@ -223,13 +288,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(); @@ -244,13 +308,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(); @@ -266,13 +329,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(); @@ -294,7 +356,9 @@ describe('PeriodicExportingMetricReader', () => { exportTimeoutMillis: 80, }); - reader.setMetricProducer(new TestMetricProducer()); + reader.setMetricProducer( + new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] }) + ); await reader.forceFlush(); exporterMock.verify(); @@ -320,7 +384,7 @@ describe('PeriodicExportingMetricReader', () => { asyncAttributesPending: true, // ensure we try to await async attributes waitForAsyncAttributes: waitForAsyncAttributesStub, // resolve when awaited }, - scopeMetrics: [], + scopeMetrics: scopeMetrics, }; const mockCollectionResult: CollectionResult = {