Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix(sdk-metrics): allow single bucket histograms
  • Loading branch information
pichlermarc committed Feb 1, 2024
commit 183a1d20fc3b5ce2914bd0e97890ff9d81704b83
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 === undefined) {
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);
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);
});
});

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
7 changes: 7 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,13 @@ 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('constructor should not modify inputs', () => {
const boundaries = [100, 10, 1];
const aggregation = new ExplicitBucketHistogramAggregation(boundaries);
Expand Down