open-telemetry
/
opentelemetry-js
mirror of https://github.com/open-telemetry/opentelemetry-js.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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

This commit is contained in:
Marc Pichler 2023-09-28 14:17:06 +02:00 committed by GitHub
parent f0ceabc57c
commit 52f428a9f1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 95 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
CHANGELOG.md
View File

@ -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)

22
packages/sdk-metrics/src/state/MeterSharedState.ts
View File

@ -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[];
}

32
packages/sdk-metrics/src/state/MetricCollector.ts
View File

@ -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,
};
}

9
packages/sdk-metrics/src/state/TemporalMetricProcessor.ts
View File

@ -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
);
}

16
packages/sdk-metrics/test/Instruments.test.ts
View File

@ -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);
});
});

32
packages/sdk-metrics/test/MeterProvider.test.ts
View File

@ -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();

3
packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts
View File

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

54
packages/sdk-metrics/test/state/MetricCollector.test.ts
View File

@ -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);
});
});
});

3
packages/sdk-metrics/test/state/SyncMetricStorage.test.ts
View File

@ -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]);

9
packages/sdk-metrics/test/state/TemporalMetricProcessor.test.ts
View File

@ -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.