diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts index a582ac6203..23f836c039 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts @@ -22,7 +22,7 @@ import { MetricReader, DataPoint, DataPointType, - HistogramAggregation, + ExplicitBucketHistogramAggregation, SumAggregation, Histogram, } from '@opentelemetry/sdk-metrics-base-wip'; @@ -109,7 +109,7 @@ describe('PrometheusSerializer', () => { const reader = new TestMetricReader(); const meterProvider = new MeterProvider(); meterProvider.addMetricReader(reader); - meterProvider.addView({ aggregation: new HistogramAggregation([1, 10, 100]) }); + meterProvider.addView({ aggregation: new ExplicitBucketHistogramAggregation([1, 10, 100]) }); const meter = meterProvider.getMeter('test'); const histogram = meter.createHistogram('test'); @@ -208,12 +208,12 @@ describe('PrometheusSerializer', () => { }); }); - describe('with HistogramAggregator', () => { + describe('with ExplicitBucketHistogramAggregation', () => { async function testSerializer(serializer: PrometheusSerializer) { const reader = new TestMetricReader(); const meterProvider = new MeterProvider(); meterProvider.addMetricReader(reader); - meterProvider.addView({ aggregation: new HistogramAggregation([1, 10, 100]) }); + meterProvider.addView({ aggregation: new ExplicitBucketHistogramAggregation([1, 10, 100]) }); const meter = meterProvider.getMeter('test'); const histogram = meter.createHistogram('test', { @@ -235,7 +235,7 @@ describe('PrometheusSerializer', () => { return result; } - it('serialize metric record with HistogramAggregator aggregator, cumulative', async () => { + it('serialize metric record with ExplicitHistogramAggregation aggregator, cumulative', async () => { const serializer = new PrometheusSerializer(); const result = await testSerializer(serializer); assert.strictEqual( diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts index 90d4b63ea3..f3f1782d31 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts @@ -27,10 +27,12 @@ import { InstrumentDescriptor } from '../InstrumentDescriptor'; import { Maybe } from '../utils'; function createNewEmptyCheckpoint(boundaries: number[]): Histogram { + const counts = boundaries.map(() => 0); + counts.push(0); return { buckets: { boundaries, - counts: boundaries.map(() => 0).concat([0]), + counts, }, sum: 0, count: 0, @@ -68,16 +70,11 @@ export class HistogramAccumulation implements Accumulation { */ export class HistogramAggregator implements Aggregator { public kind: AggregatorKind.HISTOGRAM = AggregatorKind.HISTOGRAM; - private readonly _boundaries: number[]; - constructor(boundaries: number[]) { - if (boundaries === undefined || boundaries.length === 0) { - throw new Error('HistogramAggregator should be created with boundaries.'); - } - // we need to an ordered set to be able to correctly compute count for each - // boundary since we'll iterate on each in order. - this._boundaries = boundaries.sort((a, b) => a - b); - } + /** + * @param _boundaries upper bounds of recorded values. + */ + constructor(private readonly _boundaries: number[]) {} createAccumulation() { return new HistogramAccumulation(this._boundaries); diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts index b9d11e2990..ce3afa5fa2 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts @@ -83,14 +83,9 @@ export class LastValueAggregation extends Aggregation { * The default histogram aggregation. */ export class HistogramAggregation extends Aggregation { - private DEFAULT_INSTANCE: HistogramAggregator; - constructor(boundaries: number[] = [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]) { - super(); - this.DEFAULT_INSTANCE = new HistogramAggregator(boundaries); - } - + private static DEFAULT_INSTANCE = new HistogramAggregator([0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]); createAggregator(_instrument: InstrumentDescriptor) { - return this.DEFAULT_INSTANCE; + return HistogramAggregation.DEFAULT_INSTANCE; } } @@ -98,11 +93,27 @@ export class HistogramAggregation extends Aggregation { * The explicit bucket histogram aggregation. */ export class ExplicitBucketHistogramAggregation extends Aggregation { + private _boundaries: number[]; /** - * @param _boundaries the bucket boundaries of the histogram aggregation + * @param boundaries the bucket boundaries of the histogram aggregation */ - constructor(private _boundaries: number[]) { + constructor(boundaries: number[]) { super(); + if (boundaries === undefined || boundaries.length === 0) { + throw new Error('HistogramAggregator should be created with boundaries.'); + } + // Copy the boundaries array for modification. + boundaries = boundaries.concat(); + // We need to an ordered set to be able to correctly compute count for each + // boundary since we'll iterate on each in order. + boundaries = boundaries.sort((a, b) => a - b); + // Remove all Infinity from the boundaries. + const minusInfinityIndex = boundaries.lastIndexOf(-Infinity); + let infinityIndex: number | undefined = boundaries.indexOf(Infinity); + if (infinityIndex === -1) { + infinityIndex = undefined; + } + this._boundaries = boundaries.slice(minusInfinityIndex + 1, infinityIndex); } createAggregator(_instrument: InstrumentDescriptor) { diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts index 6ce420a511..c5e959d854 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts @@ -83,19 +83,47 @@ describe('DefaultAggregation', () => { }); }); +describe('HistogramAggregator', () => { + describe('createAggregator', () => { + it('should create histogram aggregators with boundaries', () => { + const aggregator = new HistogramAggregation().createAggregator(defaultInstrumentDescriptor); + assert(aggregator instanceof HistogramAggregator); + assert.deepStrictEqual(aggregator['_boundaries'], [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]); + }); + }); +}); + describe('ExplicitBucketHistogramAggregation', () => { it('construct without exceptions', () => { - const aggregation = new ExplicitBucketHistogramAggregation([1, 10, 100]); + const cases = [ + [1, 10, 100], + [1, 10, 100, Infinity], + [-Infinity, 1, 10, 100], + ]; + for (const boundaries of cases) { + const aggregation = new ExplicitBucketHistogramAggregation(boundaries); + assert(aggregation instanceof ExplicitBucketHistogramAggregation); + assert.deepStrictEqual(aggregation['_boundaries'], [1, 10, 100]); + } + }); + + it('constructor should not modify inputs', () => { + const boundaries = [100, 10, 1]; + const aggregation = new ExplicitBucketHistogramAggregation(boundaries); assert(aggregation instanceof ExplicitBucketHistogramAggregation); + assert.deepStrictEqual(aggregation['_boundaries'], [1, 10, 100]); + assert.deepStrictEqual(boundaries, [100, 10, 1]); }); describe('createAggregator', () => { it('should create histogram aggregators with boundaries', () => { - const aggregator1 = new ExplicitBucketHistogramAggregation([1, 10, 100]).createAggregator(defaultInstrumentDescriptor); + const aggregator1 = new ExplicitBucketHistogramAggregation([100, 10, 1]).createAggregator(defaultInstrumentDescriptor); assert(aggregator1 instanceof HistogramAggregator); assert.deepStrictEqual(aggregator1['_boundaries'], [1, 10, 100]); - const aggregator2 = new ExplicitBucketHistogramAggregation([10, 100, 1000]).createAggregator(defaultInstrumentDescriptor); + const aggregator2 = new ExplicitBucketHistogramAggregation( + [-Infinity, -Infinity, 10, 100, 1000, Infinity, Infinity] + ).createAggregator(defaultInstrumentDescriptor); assert(aggregator2 instanceof HistogramAggregator); assert.deepStrictEqual(aggregator2['_boundaries'], [10, 100, 1000]); });