From 3735506d24f885057462663f1f22a4345f67a28b Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Wed, 21 Jun 2023 20:50:49 +0000 Subject: [PATCH] fix(sdk-metrics): preserve startTime for cumulative ExponentialHistograms --- CHANGELOG.md | 1 + .../src/aggregator/ExponentialHistogram.ts | 33 ++++++++++--------- .../aggregator/ExponentialHistogram.test.ts | 9 +++++ 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 819fab8972..a162eae0a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :bug: (Bug Fix) * fix(sdk-metrics): Update default Histogram's boundary to match OTEL's spec [#3893](https://github.com/open-telemetry/opentelemetry-js/pull/3893/) @chigia001 +* fix(sdk-metrics): preserve startTime for cumulative ExponentialHistograms [#3934](https://github.com/open-telemetry/opentelemetry-js/pull/3934/) @aabmass ### :books: (Refine Doc) diff --git a/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts b/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts index 5a70a478a8..51c71ee211 100644 --- a/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts +++ b/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts @@ -213,32 +213,33 @@ export class ExponentialHistogramAccumulation implements Accumulation { } /** - * merge combines data from other into self - * @param {ExponentialHistogramAccumulation} other + * merge combines data from previous value into self + * @param {ExponentialHistogramAccumulation} previous */ - merge(other: ExponentialHistogramAccumulation) { + merge(previous: ExponentialHistogramAccumulation) { if (this._count === 0) { - this._min = other.min; - this._max = other.max; - } else if (other.count !== 0) { - if (other.min < this.min) { - this._min = other.min; + this._min = previous.min; + this._max = previous.max; + } else if (previous.count !== 0) { + if (previous.min < this.min) { + this._min = previous.min; } - if (other.max > this.max) { - this._max = other.max; + if (previous.max > this.max) { + this._max = previous.max; } } - this._sum += other.sum; - this._count += other.count; - this._zeroCount += other.zeroCount; + this.startTime = previous.startTime; + this._sum += previous.sum; + this._count += previous.count; + this._zeroCount += previous.zeroCount; - const minScale = this._minScale(other); + const minScale = this._minScale(previous); this._downscale(this.scale - minScale); - this._mergeBuckets(this.positive, other, other.positive, minScale); - this._mergeBuckets(this.negative, other, other.negative, minScale); + this._mergeBuckets(this.positive, previous, previous.positive, minScale); + this._mergeBuckets(this.negative, previous, previous.negative, minScale); } /** diff --git a/packages/sdk-metrics/test/aggregator/ExponentialHistogram.test.ts b/packages/sdk-metrics/test/aggregator/ExponentialHistogram.test.ts index 150eb13e91..2bcbc43307 100644 --- a/packages/sdk-metrics/test/aggregator/ExponentialHistogram.test.ts +++ b/packages/sdk-metrics/test/aggregator/ExponentialHistogram.test.ts @@ -574,6 +574,15 @@ describe('ExponentialHistogramAggregation', () => { assert.deepStrictEqual(acc0.toPointValue(), acc0Snapshot); assert.deepStrictEqual(acc1.toPointValue(), acc1Snapshot); }); + + it("keeps the previous point's startTime", () => { + const agg = new ExponentialHistogramAggregator(4, true); + const acc0 = agg.createAccumulation([0, 0]); + const acc1 = agg.createAccumulation([3, 0]); + + const result = agg.merge(acc0, acc1); + assert.strictEqual(result.startTime, acc0.startTime); + }); }); describe('diff', () => {