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): allow single bucket histograms #4456

5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,13 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :rocket: (Enhancement)

* feat(sdk-metrics): allow single bucket histograms [#4456](https://github.com/open-telemetry/opentelemetry-js/pull/4456) @pichlermarc

### :bug: (Bug Fix)

* fix(sdk-metrics): allow single bucket histograms [#4456](https://github.com/open-telemetry/opentelemetry-js/pull/4456) @pichlermarc
* fixes a bug where `Meter.createHistogram()` with the advice `explicitBucketBoundaries: []` would throw

### :books: (Refine Doc)

### :house: (Internal)
Expand Down
6 changes: 4 additions & 2 deletions packages/sdk-metrics/src/view/Aggregation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,10 @@ export class ExplicitBucketHistogramAggregation extends Aggregation {
private readonly _recordMinMax = true
) {
super();
if (boundaries === undefined || boundaries.length === 0) {
throw new Error('HistogramAggregator should be created with boundaries.');
if (boundaries == null) {
throw new Error(
'ExplicitBucketHistogramAggregation should be created with explicit boundaries, if a single bucket histogram is required, please pass an empty array'
);
}
// Copy the boundaries array for modification.
boundaries = boundaries.concat();
Expand Down
14 changes: 14 additions & 0 deletions packages/sdk-metrics/test/Instruments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,20 @@ describe('Instruments', () => {
});
});

it('should allow metric advice with empty explicit boundaries', function () {
const meter = new MeterProvider({
readers: [new TestMetricReader()],
}).getMeter('meter');

assert.doesNotThrow(() => {
meter.createHistogram('histogram', {
advice: {
explicitBucketBoundaries: [],
},
});
});
});

it('should collect min and max', async () => {
const { meter, deltaReader, cumulativeReader } = setup();
const histogram = meter.createHistogram('test', {
Expand Down
92 changes: 92 additions & 0 deletions packages/sdk-metrics/test/aggregator/Histogram.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,27 @@ describe('HistogramAggregator', () => {
sum: -65,
});
});

it('with single bucket', function () {
const aggregator = new HistogramAggregator([0], true);
pichlermarc marked this conversation as resolved.
Show resolved Hide resolved
const prev = aggregator.createAccumulation([0, 0]);
prev.record(0);
prev.record(1);

const delta = aggregator.createAccumulation([1, 1]);
delta.record(2);
delta.record(11);

const expected = aggregator.createAccumulation([0, 0]);
// replay actions on prev
expected.record(0);
expected.record(1);
// replay actions on delta
expected.record(2);
expected.record(11);

assert.deepStrictEqual(aggregator.merge(prev, delta), expected);
pichlermarc marked this conversation as resolved.
Show resolved Hide resolved
});
});

describe('diff', () => {
Expand Down Expand Up @@ -112,6 +133,35 @@ describe('HistogramAggregator', () => {

assert.deepStrictEqual(aggregator.diff(prev, curr), expected);
});

it('with single bucket', function () {
const aggregator = new HistogramAggregator([], true);
const prev = aggregator.createAccumulation([0, 0]);
prev.record(0);
prev.record(1);

const curr = aggregator.createAccumulation([1, 1]);
// replay actions on prev
curr.record(0);
curr.record(1);
// perform new actions
curr.record(2);
curr.record(11);

const expected = new HistogramAccumulation([1, 1], [], true, {
buckets: {
boundaries: [],
counts: [2],
},
count: 2,
sum: 13,
hasMinMax: false,
min: Infinity,
max: -Infinity,
});

assert.deepStrictEqual(aggregator.diff(prev, curr), expected);
});
});

describe('toMetricData', () => {
Expand Down Expand Up @@ -199,6 +249,48 @@ describe('HistogramAggregator', () => {
);
});

it('should transform to expected data with empty boundaries', () => {
const aggregator = new HistogramAggregator([], false);

const startTime: HrTime = [0, 0];
const endTime: HrTime = [1, 1];
const accumulation = aggregator.createAccumulation(startTime);
accumulation.record(0);
accumulation.record(1);

const expected: MetricData = {
descriptor: defaultInstrumentDescriptor,
aggregationTemporality: AggregationTemporality.CUMULATIVE,
dataPointType: DataPointType.HISTOGRAM,
dataPoints: [
{
attributes: {},
startTime,
endTime,
value: {
buckets: {
boundaries: [],
counts: [2],
},
count: 2,
sum: 1,
min: undefined,
max: undefined,
},
},
],
};
assert.deepStrictEqual(
aggregator.toMetricData(
defaultInstrumentDescriptor,
AggregationTemporality.CUMULATIVE,
[[{}, accumulation]],
endTime
),
expected
);
});

function testSum(instrumentType: InstrumentType, expectSum: boolean) {
const aggregator = new HistogramAggregator([1, 10, 100], true);

Expand Down
19 changes: 19 additions & 0 deletions packages/sdk-metrics/test/view/Aggregation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,25 @@ describe('ExplicitBucketHistogramAggregation', () => {
}
});

it('construct with empty boundaries', function () {
const boundaries: number[] = [];
const aggregation = new ExplicitBucketHistogramAggregation(boundaries);
assert.ok(aggregation instanceof ExplicitBucketHistogramAggregation);
assert.deepStrictEqual(aggregation['_boundaries'], []);
});

it('construct with undefined boundaries should throw', function () {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore simulate how a JS user could pass undefined
assert.throws(() => new ExplicitBucketHistogramAggregation(undefined));
});

it('construct with null boundaries should throw', function () {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore simulate how a JS user could pass null
assert.throws(() => new ExplicitBucketHistogramAggregation(null));
});

it('constructor should not modify inputs', () => {
const boundaries = [100, 10, 1];
const aggregation = new ExplicitBucketHistogramAggregation(boundaries);
Expand Down