diff --git a/CHANGELOG.md b/CHANGELOG.md index ee112f73dd..d65dae2bdc 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/packages/sdk-metrics/src/state/MeterSharedState.ts b/packages/sdk-metrics/src/state/MeterSharedState.ts index 099a21c0d7..4189d6bb6c 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 3f17a0d5b9..f1f1dacdb1 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 2b9c5dbbaa..bb5559e70e 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 80f834f30c..cfb6255039 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 a9f8361b81..be075f0fa6 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 9fe742ca02..03eca24236 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 3c70b35759..8466a5a95c 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]; @@ -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); }); }); }); diff --git a/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts b/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts index 8e568be19e..072cc9d16d 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 8d0f165cd0..26a10aea57 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.