diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fed4e5904..35b3d94f1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ 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) @@ -18,6 +19,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * 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) diff --git a/packages/sdk-metrics/src/view/Aggregation.ts b/packages/sdk-metrics/src/view/Aggregation.ts index 6edf50c495..2f74063ca2 100644 --- a/packages/sdk-metrics/src/view/Aggregation.ts +++ b/packages/sdk-metrics/src/view/Aggregation.ts @@ -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(); diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts index 4b35451831..71c69913a3 100644 --- a/packages/sdk-metrics/test/Instruments.test.ts +++ b/packages/sdk-metrics/test/Instruments.test.ts @@ -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', { diff --git a/packages/sdk-metrics/test/aggregator/Histogram.test.ts b/packages/sdk-metrics/test/aggregator/Histogram.test.ts index b0daeac402..894e30e4af 100644 --- a/packages/sdk-metrics/test/aggregator/Histogram.test.ts +++ b/packages/sdk-metrics/test/aggregator/Histogram.test.ts @@ -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', () => { @@ -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', () => { @@ -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); diff --git a/packages/sdk-metrics/test/view/Aggregation.test.ts b/packages/sdk-metrics/test/view/Aggregation.test.ts index 0da2c09bee..51595aa590 100644 --- a/packages/sdk-metrics/test/view/Aggregation.test.ts +++ b/packages/sdk-metrics/test/view/Aggregation.test.ts @@ -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);