From e2e291c5837a10fa6e15b6274a3022680a4cdfd7 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Thu, 29 Jun 2023 02:31:08 -0400 Subject: [PATCH] fix(sdk-metrics): preserve startTime for cumulative ExponentialHistograms (#3934) Co-authored-by: Marc Pichler --- CHANGELOG.md | 1 + .../src/aggregator/ExponentialHistogram.ts | 33 +++---- .../aggregator/ExponentialHistogram.test.ts | 9 ++ .../cumulative-exponential-histogram.test.ts | 90 +++++++++++++++++++ 4 files changed, 117 insertions(+), 16 deletions(-) create mode 100644 packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts 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', () => { diff --git a/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts b/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts new file mode 100644 index 0000000000..79dcfc434f --- /dev/null +++ b/packages/sdk-metrics/test/regression/cumulative-exponential-histogram.test.ts @@ -0,0 +1,90 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { + Aggregation, + AggregationTemporality, + InstrumentType, + MeterProvider, + MetricReader, +} from '../../src'; +import { TestMetricReader } from '../export/TestMetricReader'; + +describe('cumulative-exponential-histogram', () => { + let clock: sinon.SinonFakeTimers; + + beforeEach(() => { + clock = sinon.useFakeTimers(); + }); + afterEach(() => { + sinon.restore(); + }); + + it('Cumulative Histogram should have the same startTime every collection', async () => { + // Works fine and passes + await doTest(Aggregation.Histogram()); + }); + + it('Cumulative ExponentialHistogram should have the same startTime every collection', async () => { + // Fails + await doTest(Aggregation.ExponentialHistogram()); + }); + + const doTest = async (histogramAggregation: Aggregation) => { + const meterProvider = new MeterProvider(); + const reader = new TestMetricReader({ + aggregationTemporalitySelector() { + return AggregationTemporality.CUMULATIVE; + }, + aggregationSelector(type) { + return type === InstrumentType.HISTOGRAM + ? histogramAggregation + : Aggregation.Default(); + }, + }); + + meterProvider.addMetricReader(reader); + const meter = meterProvider.getMeter('my-meter'); + const hist = meter.createHistogram('testhist'); + + hist.record(1); + + const resourceMetrics1 = await collectNoErrors(reader); + const dataPoint1 = + resourceMetrics1.scopeMetrics[0].metrics[0].dataPoints[0]; + + clock.tick(1000); + hist.record(2); + + const resourceMetrics2 = await collectNoErrors(reader); + const dataPoint2 = + resourceMetrics2.scopeMetrics[0].metrics[0].dataPoints[0]; + + assert.deepStrictEqual( + dataPoint1.startTime, + dataPoint2.startTime, + 'The start time should be the same across cumulative collections' + ); + }; + + const collectNoErrors = async (reader: MetricReader) => { + const { resourceMetrics, errors } = await reader.collect(); + assert.strictEqual(errors.length, 0); + return resourceMetrics; + }; +});