fix(sdk-metrics): preserve startTime for cumulative ExponentialHistog…

…rams (#3934)

Co-authored-by: Marc Pichler <[email protected]>

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)
 

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);
   }
 
   /**

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', () => {

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;
+  };
+});