Skip to content

Commit

Permalink
fix(sdk-metrics): allow single bucket histograms (#4456)
Browse files Browse the repository at this point in the history
* fix(sdk-metrics): allow single bucket histograms

* test(sdk-metrics): undefined and null inputs for bucket boundaries

* fixup! test(sdk-metrics): undefined and null inputs for bucket boundaries
  • Loading branch information
pichlermarc authored Feb 6, 2024
1 parent f6712fd commit 63845ec
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 2 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,16 @@ 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
* feat(instrumentation): Make `init()` method public [#4418](https://github.com/open-telemetry/opentelemetry-js/pull/4418)

### :bug: (Bug Fix)

* fix(sdk-metrics): handle zero bucket counts in exponential histogram merge [#4459](https://github.com/open-telemetry/opentelemetry-js/pull/4459) @mwear
* fix(sdk-metrics): ignore `NaN` value recordings in Histograms [#4455](https://github.com/open-telemetry/opentelemetry-js/pull/4455) @pichlermarc
* fixes a bug where recording `NaN` on a histogram would result in the sum of bucket count values not matching the overall count
* 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)

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 @@ -407,6 +407,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
95 changes: 95 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,30 @@ describe('HistogramAggregator', () => {
sum: -65,
});
});

it('with single bucket', function () {
const aggregator = new HistogramAggregator([], 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 = new HistogramAccumulation([0, 0], [], true, {
buckets: {
boundaries: [],
counts: [4],
},
count: 4,
sum: 14,
hasMinMax: true,
min: 0,
max: 11,
});
assert.deepStrictEqual(aggregator.merge(prev, delta), expected);
});
});

describe('diff', () => {
Expand Down Expand Up @@ -112,6 +136,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 +252,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

0 comments on commit 63845ec

Please sign in to comment.