fix(sdk-metrics): do not report empty scopes and metrics (#4135)

CHANGELOG.md

@@ -14,6 +14,9 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
 ### :bug: (Bug Fix)
 
 * fix(sdk-metrics): allow instrument names to contain '/' [#4155](https://github.com/open-telemetry/opentelemetry-js/pull/4155)
+* fix(sdk-metrics): do not report empty scopes and metrics [#4135](https://github.com/open-telemetry/opentelemetry-js/pull/4135) @pichlermarc
+  * Instruments that were created, but did not have measurements will not be exported anymore
+  * Meters (Scopes) that were created, but did not have any instruments with measurements under them will not be exported anymore.
 * fix(exporter-zipkin): round duration to the nearest int in annotations to be compliant with zipkin protocol [#4167](https://github.com/open-telemetry/opentelemetry-js/pull/4167) @FelipeEmerim
 
 ### :books: (Refine Doc)

packages/sdk-metrics/src/state/MeterSharedState.ts

@@ -78,7 +78,7 @@ export class MeterSharedState {
     collector: MetricCollectorHandle,
     collectionTime: HrTime,
     options?: MetricCollectOptions
-  ): Promise<ScopeMetricsResult> {
+  ): Promise<ScopeMetricsResult | null> {
     /**
      * 1. Call all observable callbacks first.
      * 2. Collect metric result for the collector.
@@ -87,9 +87,14 @@ export class MeterSharedState {
       collectionTime,
       options?.timeoutMillis
     );
-    const metricDataList = Array.from(
-      this.metricStorageRegistry.getStorages(collector)
-    )
+    const storages = this.metricStorageRegistry.getStorages(collector);
+
+    // prevent more allocations if there are no storages.
+    if (storages.length === 0) {
+      return null;
+    }
+
+    const metricDataList = storages
       .map(metricStorage => {
         return metricStorage.collect(
           collector,
@@ -99,10 +104,15 @@ export class MeterSharedState {
       })
       .filter(isNotNullish);
 
+    // skip this scope if no data was collected (storage created, but no data observed)
+    if (metricDataList.length === 0) {
+      return { errors };
+    }
+
     return {
       scopeMetrics: {
         scope: this._instrumentationScope,
-        metrics: metricDataList.filter(isNotNullish),
+        metrics: metricDataList,
       },
       errors,
     };
@@ -173,7 +183,7 @@ export class MeterSharedState {
 }
 
 interface ScopeMetricsResult {
-  scopeMetrics: ScopeMetrics;
+  scopeMetrics?: ScopeMetrics;
   errors: unknown[];
 }
 

packages/sdk-metrics/src/state/MetricCollector.ts

@@ -16,12 +16,11 @@
 
 import { millisToHrTime } from '@opentelemetry/core';
 import { AggregationTemporalitySelector } from '../export/AggregationSelector';
-import { CollectionResult } from '../export/MetricData';
+import { CollectionResult, ScopeMetrics } from '../export/MetricData';
 import { MetricProducer, MetricCollectOptions } from '../export/MetricProducer';
 import { MetricReader } from '../export/MetricReader';
 import { InstrumentType } from '../InstrumentDescriptor';
 import { ForceFlushOptions, ShutdownOptions } from '../types';
-import { FlatMap } from '../utils';
 import { MeterProviderSharedState } from './MeterProviderSharedState';
 
 /**
@@ -37,19 +36,36 @@ export class MetricCollector implements MetricProducer {
 
   async collect(options?: MetricCollectOptions): Promise<CollectionResult> {
     const collectionTime = millisToHrTime(Date.now());
+    const scopeMetrics: ScopeMetrics[] = [];
+    const errors: unknown[] = [];
+
     const meterCollectionPromises = Array.from(
       this._sharedState.meterSharedStates.values()
-    ).map(meterSharedState =>
-      meterSharedState.collect(this, collectionTime, options)
-    );
-    const result = await Promise.all(meterCollectionPromises);
+    ).map(async meterSharedState => {
+      const current = await meterSharedState.collect(
+        this,
+        collectionTime,
+        options
+      );
+
+      // only add scope metrics if available
+      if (current?.scopeMetrics != null) {
+        scopeMetrics.push(current.scopeMetrics);
+      }
+
+      // only add errors if available
+      if (current?.errors != null) {
+        errors.push(...current.errors);
+      }
+    });
+    await Promise.all(meterCollectionPromises);
 
     return {
       resourceMetrics: {
         resource: this._sharedState.resource,
-        scopeMetrics: result.map(it => it.scopeMetrics),
+        scopeMetrics: scopeMetrics,
       },
-      errors: FlatMap(result, it => it.errors),
+      errors: errors,
     };
   }
 

packages/sdk-metrics/src/state/TemporalMetricProcessor.ts

@@ -133,10 +133,17 @@ export class TemporalMetricProcessor<T extends Maybe<Accumulation>> {
       aggregationTemporality,
     });
 
+    const accumulationRecords = AttributesMapToAccumulationRecords(result);
+
+    // do not convert to metric data if there is nothing to convert.
+    if (accumulationRecords.length === 0) {
+      return undefined;
+    }
+
     return this._aggregator.toMetricData(
       instrumentDescriptor,
       aggregationTemporality,
-      AttributesMapToAccumulationRecords(result),
+      accumulationRecords,
       /* endTime */ collectionTime
     );
   }

packages/sdk-metrics/test/Instruments.test.ts

@@ -434,10 +434,10 @@ describe('Instruments', () => {
       });
 
       histogram.record(-1, { foo: 'bar' });
-      await validateExport(deltaReader, {
-        dataPointType: DataPointType.HISTOGRAM,
-        dataPoints: [],
-      });
+      const result = await deltaReader.collect();
+
+      // nothing observed
+      assert.equal(result.resourceMetrics.scopeMetrics.length, 0);
     });
 
     it('should record DOUBLE values', async () => {
@@ -499,10 +499,10 @@ describe('Instruments', () => {
       });
 
       histogram.record(-0.5, { foo: 'bar' });
-      await validateExport(deltaReader, {
-        dataPointType: DataPointType.HISTOGRAM,
-        dataPoints: [],
-      });
+      const result = await deltaReader.collect();
+
+      // nothing observed
+      assert.equal(result.resourceMetrics.scopeMetrics.length, 0);
     });
   });
 

packages/sdk-metrics/test/MeterProvider.test.ts

@@ -77,23 +77,29 @@ describe('MeterProvider', () => {
       const reader = new TestMetricReader();
       meterProvider.addMetricReader(reader);
 
-      // Create meter and instrument.
+      // Create meter and instrument, needs observation on instrument, otherwise the scope will not be reported.
       // name+version pair 1
-      meterProvider.getMeter('meter1', 'v1.0.0');
-      meterProvider.getMeter('meter1', 'v1.0.0');
+      meterProvider.getMeter('meter1', 'v1.0.0').createCounter('test').add(1);
+      meterProvider.getMeter('meter1', 'v1.0.0').createCounter('test').add(1);
       // name+version pair 2
-      meterProvider.getMeter('meter2', 'v1.0.0');
-      meterProvider.getMeter('meter2', 'v1.0.0');
+      meterProvider.getMeter('meter2', 'v1.0.0').createCounter('test').add(1);
+      meterProvider.getMeter('meter2', 'v1.0.0').createCounter('test').add(1);
       // name+version pair 3
-      meterProvider.getMeter('meter1', 'v1.0.1');
-      meterProvider.getMeter('meter1', 'v1.0.1');
+      meterProvider.getMeter('meter1', 'v1.0.1').createCounter('test').add(1);
+      meterProvider.getMeter('meter1', 'v1.0.1').createCounter('test').add(1);
       // name+version+schemaUrl pair 4
-      meterProvider.getMeter('meter1', 'v1.0.1', {
-        schemaUrl: 'https://opentelemetry.io/schemas/1.4.0',
-      });
-      meterProvider.getMeter('meter1', 'v1.0.1', {
-        schemaUrl: 'https://opentelemetry.io/schemas/1.4.0',
-      });
+      meterProvider
+        .getMeter('meter1', 'v1.0.1', {
+          schemaUrl: 'https://opentelemetry.io/schemas/1.4.0',
+        })
+        .createCounter('test')
+        .add(1);
+      meterProvider
+        .getMeter('meter1', 'v1.0.1', {
+          schemaUrl: 'https://opentelemetry.io/schemas/1.4.0',
+        })
+        .createCounter('test')
+        .add(1);
 
       // Perform collection.
       const { resourceMetrics, errors } = await reader.collect();

packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts

@@ -110,8 +110,7 @@ describe('AsyncMetricStorage', () => {
             collectionTime
           );
 
-          assertMetricData(metric, DataPointType.SUM);
-          assert.strictEqual(metric.dataPoints.length, 0);
+          assert.equal(metric, undefined);
         }
 
         delegate.setDelegate(observableResult => {

packages/sdk-metrics/test/state/MetricCollector.test.ts

@@ -141,7 +141,8 @@ describe('MetricCollector', () => {
       assert.strictEqual(errors.length, 0);
       const { scopeMetrics } = resourceMetrics;
       const { metrics } = scopeMetrics[0];
-      assert.strictEqual(metrics.length, 3);
+      // Should not export observableCounter3, as it was never observed
+      assert.strictEqual(metrics.length, 2);
 
       /** checking batch[0] */
       const metricData1 = metrics[0];
@@ -160,13 +161,6 @@ describe('MetricCollector', () => {
       assert.strictEqual(metricData2.dataPoints.length, 2);
       assertDataPoint(metricData2.dataPoints[0], {}, 3);
       assertDataPoint(metricData2.dataPoints[1], { foo: 'bar' }, 4);
-
-      /** checking batch[2] */
-      const metricData3 = metrics[2];
-      assertMetricData(metricData3, DataPointType.SUM, {
-        name: 'observable3',
-      });
-      assert.strictEqual(metricData3.dataPoints.length, 0);
     });
 
     it('should collect observer metrics with timeout', async () => {
@@ -205,19 +199,15 @@ describe('MetricCollector', () => {
         assert(errors[0] instanceof TimeoutError);
         const { scopeMetrics } = resourceMetrics;
         const { metrics } = scopeMetrics[0];
-        assert.strictEqual(metrics.length, 2);
 
-        /** observer1 */
-        assertMetricData(metrics[0], DataPointType.SUM, {
-          name: 'observer1',
-        });
-        assert.strictEqual(metrics[0].dataPoints.length, 0);
+        // Only observer2 is exported, observer1 never reported a measurement
+        assert.strictEqual(metrics.length, 1);
 
         /** observer2 */
-        assertMetricData(metrics[1], DataPointType.SUM, {
+        assertMetricData(metrics[0], DataPointType.SUM, {
           name: 'observer2',
         });
-        assert.strictEqual(metrics[1].dataPoints.length, 1);
+        assert.strictEqual(metrics[0].dataPoints.length, 1);
       }
 
       /** now the observer1 is back to normal */
@@ -272,19 +262,13 @@ describe('MetricCollector', () => {
       assert.strictEqual(`${errors[0]}`, 'Error: foobar');
       const { scopeMetrics } = resourceMetrics;
       const { metrics } = scopeMetrics[0];
-      assert.strictEqual(metrics.length, 2);
 
-      /** counter1 data points are collected */
+      /** only counter1 data points are collected */
+      assert.strictEqual(metrics.length, 1);
       assertMetricData(metrics[0], DataPointType.SUM, {
         name: 'counter1',
       });
       assert.strictEqual(metrics[0].dataPoints.length, 1);
-
-      /** observer1 data points are not collected */
-      assertMetricData(metrics[1], DataPointType.SUM, {
-        name: 'observer1',
-      });
-      assert.strictEqual(metrics[1].dataPoints.length, 0);
     });
 
     it('should collect batch observer metrics with timeout', async () => {
@@ -327,19 +311,13 @@ describe('MetricCollector', () => {
         assert(errors[0] instanceof TimeoutError);
         const { scopeMetrics } = resourceMetrics;
         const { metrics } = scopeMetrics[0];
-        assert.strictEqual(metrics.length, 2);
 
-        /** observer1 */
+        /** only observer2 is present; observer1's promise never settled*/
+        assert.strictEqual(metrics.length, 1);
         assertMetricData(metrics[0], DataPointType.SUM, {
-          name: 'observer1',
-        });
-        assert.strictEqual(metrics[0].dataPoints.length, 0);
-
-        /** observer2 */
-        assertMetricData(metrics[1], DataPointType.SUM, {
           name: 'observer2',
         });
-        assert.strictEqual(metrics[1].dataPoints.length, 1);
+        assert.strictEqual(metrics[0].dataPoints.length, 1);
       }
 
       /** now the observer1 is back to normal */
@@ -398,19 +376,13 @@ describe('MetricCollector', () => {
       assert.strictEqual(`${errors[0]}`, 'Error: foobar');
       const { scopeMetrics } = resourceMetrics;
       const { metrics } = scopeMetrics[0];
-      assert.strictEqual(metrics.length, 2);
 
-      /** counter1 data points are collected */
+      /** counter1 data points are collected; observer1's callback did throw, so data points are not collected */
+      assert.strictEqual(metrics.length, 1);
       assertMetricData(metrics[0], DataPointType.SUM, {
         name: 'counter1',
       });
       assert.strictEqual(metrics[0].dataPoints.length, 1);
-
-      /** observer1 data points are not collected */
-      assertMetricData(metrics[1], DataPointType.SUM, {
-        name: 'observer1',
-      });
-      assert.strictEqual(metrics[1].dataPoints.length, 0);
     });
   });
 });

packages/sdk-metrics/test/state/SyncMetricStorage.test.ts

@@ -88,8 +88,7 @@ describe('SyncMetricStorage', () => {
             [4, 4]
           );
 
-          assertMetricData(metric, DataPointType.SUM);
-          assert.strictEqual(metric.dataPoints.length, 0);
+          assert.strictEqual(metric, undefined);
         }
 
         metricStorage.record(1, {}, api.context.active(), [5, 5]);

packages/sdk-metrics/test/state/TemporalMetricProcessor.test.ts

@@ -107,13 +107,8 @@ describe('TemporalMetricProcessor', () => {
             [5, 5]
           );
 
-          assertMetricData(
-            metric,
-            DataPointType.SUM,
-            defaultInstrumentDescriptor,
-            AggregationTemporality.DELTA
-          );
-          assert.strictEqual(metric.dataPoints.length, 0);
+          // nothing recorded -> nothing collected
+          assert.equal(metric, undefined);
         }
 
         // selectAggregationTemporality should be called only once.