[backport/v1.x] fix(sdk-metrics): don't export from PeriodicExporting…

…MetricReader with no metrics (#5288) (#5340) Co-authored-by: Jackson Weber <[email protected]> Co-authored-by: Hector Hernandez <[email protected]>
open-telemetry · Jan 13, 2025 · 18b33a0 · 18b33a0
1 parent 3acae30
commit 18b33a0
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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<void>(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 = {