From 8988bc4784bcab1190662fe30dc525c4df099463 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 11 Sep 2023 13:24:25 +0200 Subject: [PATCH 1/6] fix(sdk-metrics): do not report empty scopes and metrics --- .../sdk-metrics/src/state/MeterSharedState.ts | 22 +++++--- .../sdk-metrics/src/state/MetricCollector.ts | 32 ++++++++--- .../src/state/TemporalMetricProcessor.ts | 9 +++- packages/sdk-metrics/test/Instruments.test.ts | 16 +++--- .../sdk-metrics/test/MeterProvider.test.ts | 32 ++++++----- .../test/state/AsyncMetricStorage.test.ts | 3 +- .../test/state/MetricCollector.test.ts | 54 ++++++------------- .../test/state/SyncMetricStorage.test.ts | 3 +- .../state/TemporalMetricProcessor.test.ts | 9 +--- 9 files changed, 94 insertions(+), 86 deletions(-) diff --git a/packages/sdk-metrics/src/state/MeterSharedState.ts b/packages/sdk-metrics/src/state/MeterSharedState.ts index 099a21c0d77..4189d6bb6c6 100644 --- a/packages/sdk-metrics/src/state/MeterSharedState.ts +++ b/packages/sdk-metrics/src/state/MeterSharedState.ts @@ -78,7 +78,7 @@ export class MeterSharedState { collector: MetricCollectorHandle, collectionTime: HrTime, options?: MetricCollectOptions - ): Promise { + ): Promise { /** * 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[]; } diff --git a/packages/sdk-metrics/src/state/MetricCollector.ts b/packages/sdk-metrics/src/state/MetricCollector.ts index 3f17a0d5b9a..f1f1dacdb13 100644 --- a/packages/sdk-metrics/src/state/MetricCollector.ts +++ b/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 { 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, }; } diff --git a/packages/sdk-metrics/src/state/TemporalMetricProcessor.ts b/packages/sdk-metrics/src/state/TemporalMetricProcessor.ts index 2b9c5dbbaa9..bb5559e70e6 100644 --- a/packages/sdk-metrics/src/state/TemporalMetricProcessor.ts +++ b/packages/sdk-metrics/src/state/TemporalMetricProcessor.ts @@ -133,10 +133,17 @@ export class TemporalMetricProcessor> { 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 ); } diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts index 80f834f30cb..cfb62550398 100644 --- a/packages/sdk-metrics/test/Instruments.test.ts +++ b/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); }); }); diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index a9f8361b81c..be075f0fa61 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/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(); diff --git a/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts b/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts index 9fe742ca021..03eca242360 100644 --- a/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts +++ b/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 => { diff --git a/packages/sdk-metrics/test/state/MetricCollector.test.ts b/packages/sdk-metrics/test/state/MetricCollector.test.ts index 3c70b357599..d3cb131f06c 100644 --- a/packages/sdk-metrics/test/state/MetricCollector.test.ts +++ b/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]; @@ -161,12 +162,9 @@ describe('MetricCollector', () => { assertDataPoint(metricData2.dataPoints[0], {}, 3); assertDataPoint(metricData2.dataPoints[1], { foo: 'bar' }, 4); - /** checking batch[2] */ + /** checking batch[2], should not exist as it was never observed */ const metricData3 = metrics[2]; - assertMetricData(metricData3, DataPointType.SUM, { - name: 'observable3', - }); - assert.strictEqual(metricData3.dataPoints.length, 0); + assert.strictEqual(metricData3, undefined); }); it('should collect observer metrics with timeout', async () => { @@ -205,19 +203,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 +266,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 +315,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 +380,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); }); }); }); diff --git a/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts b/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts index 8e568be19e2..072cc9d16dc 100644 --- a/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts +++ b/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]); diff --git a/packages/sdk-metrics/test/state/TemporalMetricProcessor.test.ts b/packages/sdk-metrics/test/state/TemporalMetricProcessor.test.ts index 8d0f165cd03..26a10aea572 100644 --- a/packages/sdk-metrics/test/state/TemporalMetricProcessor.test.ts +++ b/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. From 726ef7b564996fb107e6164e7b227a70c161c8a4 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 11 Sep 2023 13:34:59 +0200 Subject: [PATCH 2/6] fix(sdk-metrics): remove nonsense assertion --- packages/sdk-metrics/test/state/MetricCollector.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/sdk-metrics/test/state/MetricCollector.test.ts b/packages/sdk-metrics/test/state/MetricCollector.test.ts index d3cb131f06c..8466a5a95cc 100644 --- a/packages/sdk-metrics/test/state/MetricCollector.test.ts +++ b/packages/sdk-metrics/test/state/MetricCollector.test.ts @@ -161,10 +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], should not exist as it was never observed */ - const metricData3 = metrics[2]; - assert.strictEqual(metricData3, undefined); }); it('should collect observer metrics with timeout', async () => { From 3ad63b82cd39cc7c832c31c1192b4bbc25b3831e Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 11 Sep 2023 13:37:18 +0200 Subject: [PATCH 3/6] fix(changelog): add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47f735d6adf..047deca7366 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * fix(exporter-zipkin): rounding duration to the nearest int to be compliant with zipkin protocol [#4064](https://github.com/open-telemetry/opentelemetry-js/pull/4064) @n0cloud * fix(sdk-metrics): metric names should be case-insensitive +* fix(sdk-metrics): do not report empty scopes and metrics [#4135](https://github.com/open-telemetry/opentelemetry-js/pull/4135) @pichlermarc ### :books: (Refine Doc) From 8ab18befd8264f5b0611c1c39bf52509d8177c97 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 11 Sep 2023 13:40:20 +0200 Subject: [PATCH 4/6] fix(changelog): add more info to changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 047deca7366..afc84aee6f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * fix(exporter-zipkin): rounding duration to the nearest int to be compliant with zipkin protocol [#4064](https://github.com/open-telemetry/opentelemetry-js/pull/4064) @n0cloud * fix(sdk-metrics): metric names should be case-insensitive * 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. ### :books: (Refine Doc) From 11051ceb71b57cb057ee58bd79d77a304eff2f9a Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 11 Sep 2023 13:51:34 +0200 Subject: [PATCH 5/6] fix: lint --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afc84aee6f4..a079e6c865d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * fix(exporter-zipkin): rounding duration to the nearest int to be compliant with zipkin protocol [#4064](https://github.com/open-telemetry/opentelemetry-js/pull/4064) @n0cloud * fix(sdk-metrics): metric names should be case-insensitive -* fix(sdk-metrics): do not report empty scopes and metrics [#4135](https://github.com/open-telemetry/opentelemetry-js/pull/4135) @pichlermarc +* 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. From 40a9cedb6039ac9860d632dbcd304d3ecdac9808 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Fri, 15 Sep 2023 07:21:58 +0200 Subject: [PATCH 6/6] fix: move changelog entry --- CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c4db70168e..1fb116a47fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :bug: (Bug Fix) +* 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. + ### :books: (Refine Doc) ### :house: (Internal) @@ -36,9 +40,6 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * fix(exporter-zipkin): rounding duration to the nearest int to be compliant with zipkin protocol [#4064](https://github.com/open-telemetry/opentelemetry-js/pull/4064) @n0cloud * fix(sdk-metrics): metric names should be case-insensitive -* 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. ### :books: (Refine Doc)