Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk-metrics) Don't Export from PeriodicExportingMetricReader with No Metrics #5288

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,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. []() @jacksonweber
JacksonWeber marked this conversation as resolved.
Show resolved Hide resolved

### :books: (Refine Doc)

### :house: (Internal)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,13 @@ export class PeriodicExportingMetricReader extends MetricReader {
}
}

const result = await internal._export(this._exporter, resourceMetrics);
if (result.code !== ExportResultCode.SUCCESS) {
throw new Error(
`PeriodicExportingMetricReader: metrics export failed (error ${result.error})`
);
if (resourceMetrics.scopeMetrics.length > 0) {
const result = await internal._export(this._exporter, resourceMetrics);
if (result.code !== ExportResultCode.SUCCESS) {
throw new Error(
`PeriodicExportingMetricReader: metrics export failed (error ${result.error})`
);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,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';
Expand All @@ -34,7 +38,7 @@ import {
setGlobalErrorHandler,
} from '@opentelemetry/core';
import { assertRejects } from '../test-utils';
import { emptyResourceMetrics, TestMetricProducer } from './TestMetricProducer';
import { TestMetricProducer } from './TestMetricProducer';
import {
assertAggregationSelector,
assertAggregationTemporalitySelector,
Expand All @@ -43,6 +47,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;

Expand Down Expand Up @@ -126,6 +131,51 @@ 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: [
{
startTime: process.hrtime(),
endTime: process.hrtime(),
attributes: {},
value: 1,
},
],
descriptor: {
name: '',
description: '',
unit: '',
type: InstrumentType.COUNTER,
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();
Expand Down Expand Up @@ -203,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();
Expand All @@ -224,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();
Expand All @@ -245,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();
Expand All @@ -267,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();
Expand All @@ -295,7 +356,9 @@ describe('PeriodicExportingMetricReader', () => {
exportTimeoutMillis: 80,
});

reader.setMetricProducer(new TestMetricProducer());
reader.setMetricProducer(
new TestMetricProducer({ resourceMetrics: resourceMetrics, errors: [] })
);
await reader.forceFlush();
exporterMock.verify();

Expand All @@ -321,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 = {
Expand Down
Loading