fix(sdk-metrics-base): remove aggregator.toMetricData dependency on A…

…ggregationTemporality (open-telemetry#2676) Co-authored-by: Valentin Marchaud <[email protected]>
rauno56 · Jan 14, 2022 · 7739ce9 · 7739ce9
1 parent 669e3d9
commit 7739ce9
Show file tree

Hide file tree

Showing 10 changed files with 46 additions and 199 deletions.
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Drop.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Drop.ts
@@ -17,7 +17,6 @@
 import { HrTime } from '@opentelemetry/api';
 import { InstrumentationLibrary } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
-import { AggregationTemporality } from '../export/AggregationTemporality';
 import { MetricData } from '../export/MetricData';
 import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
@@ -48,10 +47,8 @@ export class DropAggregator implements Aggregator<undefined> {
     _instrumentationLibrary: InstrumentationLibrary,
     _instrumentDescriptor: InstrumentDescriptor,
     _accumulationByAttributes: AccumulationRecord<undefined>[],
-    _temporality: AggregationTemporality,
-    _sdkStartTime: HrTime,
-    _lastCollectionTime: HrTime,
-    _collectionTime: HrTime): Maybe<MetricData> {
+    _startTime: HrTime,
+    _endTime: HrTime): Maybe<MetricData> {
     return undefined;
   }
 }
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts
@@ -25,7 +25,6 @@ import { HistogramMetricData, PointDataType } from '../export/MetricData';
 import { Resource } from '@opentelemetry/resources';
 import { InstrumentationLibrary } from '@opentelemetry/core';
 import { HrTime } from '@opentelemetry/api';
-import { AggregationTemporality } from '../export/AggregationTemporality';
 import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
 
@@ -143,10 +142,8 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
     instrumentationLibrary: InstrumentationLibrary,
     metricDescriptor: InstrumentDescriptor,
     accumulationByAttributes: AccumulationRecord<HistogramAccumulation>[],
-    temporality: AggregationTemporality,
-    sdkStartTime: HrTime,
-    lastCollectionTime: HrTime,
-    collectionTime: HrTime): Maybe<HistogramMetricData> {
+    startTime: HrTime,
+    endTime: HrTime): Maybe<HistogramMetricData> {
     return {
       resource,
       instrumentationLibrary,
@@ -155,8 +152,8 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
       pointData: accumulationByAttributes.map(([attributes, accumulation]) => {
         return {
           attributes,
-          startTime: temporality === AggregationTemporality.CUMULATIVE ? sdkStartTime : lastCollectionTime,
-          endTime: collectionTime,
+          startTime,
+          endTime,
           point: accumulation.toPoint(),
         }
       })

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/LastValue.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/LastValue.ts
@@ -18,7 +18,6 @@ import { LastValue, AggregatorKind, Aggregator, Accumulation, AccumulationRecord
 import { HrTime } from '@opentelemetry/api';
 import { hrTime, hrTimeToMicroseconds, InstrumentationLibrary } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
-import { AggregationTemporality } from '../export/AggregationTemporality';
 import { PointDataType, SingularMetricData } from '../export/MetricData';
 import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
@@ -72,10 +71,8 @@ export class LastValueAggregator implements Aggregator<LastValueAccumulation> {
     instrumentationLibrary: InstrumentationLibrary,
     instrumentDescriptor: InstrumentDescriptor,
     accumulationByAttributes: AccumulationRecord<LastValueAccumulation>[],
-    temporality: AggregationTemporality,
-    sdkStartTime: HrTime,
-    lastCollectionTime: HrTime,
-    collectionTime: HrTime): Maybe<SingularMetricData> {
+    startTime: HrTime,
+    endTime: HrTime): Maybe<SingularMetricData> {
     return {
       resource,
       instrumentationLibrary,
@@ -84,8 +81,8 @@ export class LastValueAggregator implements Aggregator<LastValueAccumulation> {
       pointData: accumulationByAttributes.map(([attributes, accumulation]) => {
         return {
           attributes,
-          startTime: temporality === AggregationTemporality.CUMULATIVE ? sdkStartTime : lastCollectionTime,
-          endTime: collectionTime,
+          startTime,
+          endTime,
           point: accumulation.toPoint(),
         }
       })

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Sum.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Sum.ts
@@ -19,7 +19,6 @@ import { HrTime } from '@opentelemetry/api';
 import { InstrumentationLibrary } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
 import { PointDataType, SingularMetricData } from '../export/MetricData';
-import { AggregationTemporality } from '../export/AggregationTemporality';
 import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
 
@@ -62,10 +61,8 @@ export class SumAggregator implements Aggregator<SumAccumulation> {
     instrumentationLibrary: InstrumentationLibrary,
     instrumentDescriptor: InstrumentDescriptor,
     accumulationByAttributes: AccumulationRecord<SumAccumulation>[],
-    temporality: AggregationTemporality,
-    sdkStartTime: HrTime,
-    lastCollectionTime: HrTime,
-    collectionTime: HrTime): Maybe<SingularMetricData> {
+    startTime: HrTime,
+    endTime: HrTime): Maybe<SingularMetricData> {
     return {
       resource,
       instrumentationLibrary,
@@ -74,8 +71,8 @@ export class SumAggregator implements Aggregator<SumAccumulation> {
       pointData: accumulationByAttributes.map(([attributes, accumulation]) => {
         return {
           attributes,
-          startTime: temporality === AggregationTemporality.CUMULATIVE ? sdkStartTime : lastCollectionTime,
-          endTime: collectionTime,
+          startTime,
+          endTime,
           point: accumulation.toPoint(),
         }
       })

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/types.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/types.ts
@@ -18,7 +18,6 @@ import { HrTime } from '@opentelemetry/api';
 import { Attributes } from '@opentelemetry/api-metrics';
 import { InstrumentationLibrary } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
-import { AggregationTemporality } from '../export/AggregationTemporality';
 import { MetricData } from '../export/MetricData';
 import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
@@ -113,18 +112,14 @@ export interface Aggregator<T> {
    * @param instrumentationLibrary the library that instrumented the metric
    * @param instrumentDescriptor the metric instrument descriptor.
    * @param accumulationByAttributes the array of attributes and accumulation pairs.
-   * @param temporality the temporality of the accumulation.
-   * @param sdkStartTime the start time of the sdk.
-   * @param lastCollectionTime the last collection time of the instrument.
-   * @param collectionTime the active collection time of the instrument.
+   * @param startTime the start time of the metric data.
+   * @param endTime the end time of the metric data.
    * @return the {@link MetricData} that this {@link Aggregator} will produce.
    */
   toMetricData(resource: Resource,
     instrumentationLibrary: InstrumentationLibrary,
     instrumentDescriptor: InstrumentDescriptor,
     accumulationByAttributes: AccumulationRecord<T>[],
-    temporality: AggregationTemporality,
-    sdkStartTime: HrTime,
-    lastCollectionTime: HrTime,
-    collectionTime: HrTime): Maybe<MetricData>;
+    startTime: HrTime,
+    endTime: HrTime): Maybe<MetricData>;
 }
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts
@@ -107,18 +107,16 @@ export class TemporalMetricProcessor<T> {
       collectionTime,
     });
 
-    // Metric data time span is determined in Aggregator.toMetricData with aggregation temporality:
+    // Metric data time span is determined as:
     // 1. Cumulative Aggregation time span: (sdkStartTime, collectionTime]
     // 2. Delta Aggregation time span: (lastCollectionTime, collectionTime]
     return this._aggregator.toMetricData(
       resource,
       instrumentationLibrary,
       instrumentDescriptor,
       AttributesMapToAccumulationRecords(result),
-      aggregationTemporality,
-      sdkStartTime,
-      lastCollectionTime,
-      collectionTime);
+      /* startTime */ aggregationTemporality === AggregationTemporality.CUMULATIVE ? sdkStartTime : lastCollectionTime,
+      /* endTime */ collectionTime);
   }
 
   private _stashAccumulations(collectors: MetricCollectorHandle[], currentAccumulation: AttributeHashMap<T>) {

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Drop.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Drop.test.ts
@@ -17,7 +17,6 @@
 import { HrTime } from '@opentelemetry/api';
 import * as assert from 'assert';
 import { DropAggregator } from '../../src/aggregator';
-import { AggregationTemporality } from '../../src/export/AggregationTemporality';
 import { defaultInstrumentationLibrary, defaultInstrumentDescriptor, defaultResource } from '../util';
 
 describe('DropAggregator', () => {
@@ -51,19 +50,16 @@ describe('DropAggregator', () => {
     it('no exceptions', () => {
       const aggregator = new DropAggregator();
 
-      const sdkStartTime: HrTime = [0, 0];
-      const lastCollectionTime: HrTime = [1, 1];
-      const collectionTime: HrTime = [2, 2];
+      const startTime: HrTime = [0, 0];
+      const endTime: HrTime = [1, 1];
 
       assert.strictEqual(aggregator.toMetricData(
         defaultResource,
         defaultInstrumentationLibrary,
         defaultInstrumentDescriptor,
         [[{}, undefined]],
-        AggregationTemporality.DELTA,
-        sdkStartTime,
-        lastCollectionTime,
-        collectionTime,
+        startTime,
+        endTime,
       ), undefined);
     });
   });

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Histogram.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Histogram.test.ts
@@ -17,7 +17,6 @@
 import { HrTime } from '@opentelemetry/api';
 import * as assert from 'assert';
 import { HistogramAccumulation, HistogramAggregator } from '../../src/aggregator';
-import { AggregationTemporality } from '../../src/export/AggregationTemporality';
 import { MetricData, PointDataType } from '../../src/export/MetricData';
 import { commonValues, defaultInstrumentationLibrary, defaultInstrumentDescriptor, defaultResource } from '../util';
 
@@ -82,16 +81,15 @@ describe('HistogramAggregator', () => {
   });
 
   describe('toMetricData', () => {
-    it('transform with AggregationTemporality.DELTA', () => {
+    it('transform without exception', () => {
       const aggregator = new HistogramAggregator([1, 10, 100]);
 
       const accumulation = aggregator.createAccumulation();
       accumulation.record(0);
       accumulation.record(1);
 
-      const sdkStartTime: HrTime = [0, 0];
-      const lastCollectionTime: HrTime = [1, 1];
-      const collectionTime: HrTime = [2, 2];
+      const startTime: HrTime = [0, 0];
+      const endTime: HrTime = [1, 1];
 
       const expected: MetricData = {
         resource: defaultResource,
@@ -101,8 +99,8 @@ describe('HistogramAggregator', () => {
         pointData: [
           {
             attributes: {},
-            startTime: lastCollectionTime,
-            endTime: collectionTime,
+            startTime,
+            endTime,
             point: {
               buckets: {
                 boundaries: [1, 10, 100],
@@ -119,54 +117,8 @@ describe('HistogramAggregator', () => {
         defaultInstrumentationLibrary,
         defaultInstrumentDescriptor,
         [[{}, accumulation]],
-        AggregationTemporality.DELTA,
-        sdkStartTime,
-        lastCollectionTime,
-        collectionTime,
-      ), expected);
-    });
-
-    it('transform with AggregationTemporality.CUMULATIVE', () => {
-      const aggregator = new HistogramAggregator([1, 10, 100]);
-
-      const accumulation = aggregator.createAccumulation();
-      accumulation.record(0);
-      accumulation.record(1);
-
-      const sdkStartTime: HrTime = [0, 0];
-      const lastCollectionTime: HrTime = [1, 1];
-      const collectionTime: HrTime = [2, 2];
-
-      const expected: MetricData = {
-        resource: defaultResource,
-        instrumentationLibrary: defaultInstrumentationLibrary,
-        instrumentDescriptor: defaultInstrumentDescriptor,
-        pointDataType: PointDataType.HISTOGRAM,
-        pointData: [
-          {
-            attributes: {},
-            startTime: sdkStartTime,
-            endTime: collectionTime,
-            point: {
-              buckets: {
-                boundaries: [1, 10, 100],
-                counts: [1, 1, 0, 0],
-              },
-              count: 2,
-              sum: 1,
-            },
-          },
-        ],
-      };
-      assert.deepStrictEqual(aggregator.toMetricData(
-        defaultResource,
-        defaultInstrumentationLibrary,
-        defaultInstrumentDescriptor,
-        [[{}, accumulation]],
-        AggregationTemporality.CUMULATIVE,
-        sdkStartTime,
-        lastCollectionTime,
-        collectionTime,
+        startTime,
+        endTime,
       ), expected);
     });
   });

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/LastValue.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/LastValue.test.ts
@@ -17,7 +17,6 @@
 import { HrTime } from '@opentelemetry/api';
 import * as assert from 'assert';
 import { LastValueAccumulation, LastValueAggregator } from '../../src/aggregator';
-import { AggregationTemporality } from '../../src/export/AggregationTemporality';
 import { MetricData, PointDataType } from '../../src/export/MetricData';
 import { commonValues, defaultInstrumentationLibrary, defaultInstrumentDescriptor, defaultResource, sleep } from '../util';
 
@@ -88,7 +87,7 @@ describe('LastValueAggregator', () => {
   });
 
   describe('toMetricData', () => {
-    it('transform with AggregationTemporality.DELTA', () => {
+    it('transform without exception', () => {
       const aggregator = new LastValueAggregator();
 
       const accumulation = aggregator.createAccumulation();
@@ -97,9 +96,8 @@ describe('LastValueAggregator', () => {
       accumulation.record(1);
       accumulation.record(4);
 
-      const sdkStartTime: HrTime = [0, 0];
-      const lastCollectionTime: HrTime = [1, 1];
-      const collectionTime: HrTime = [2, 2];
+      const startTime: HrTime = [0, 0];
+      const endTime: HrTime = [1, 1];
 
       const expected: MetricData = {
         resource: defaultResource,
@@ -109,8 +107,8 @@ describe('LastValueAggregator', () => {
         pointData: [
           {
             attributes: {},
-            startTime: lastCollectionTime,
-            endTime: collectionTime,
+            startTime,
+            endTime,
             point: 4,
           },
         ],
@@ -120,48 +118,8 @@ describe('LastValueAggregator', () => {
         defaultInstrumentationLibrary,
         defaultInstrumentDescriptor,
         [[{}, accumulation]],
-        AggregationTemporality.DELTA,
-        sdkStartTime,
-        lastCollectionTime,
-        collectionTime,
-      ), expected);
-    });
-
-    it('transform with AggregationTemporality.CUMULATIVE', () => {
-      const aggregator = new LastValueAggregator();
-
-      const accumulation = aggregator.createAccumulation();
-      accumulation.record(1);
-      accumulation.record(2);
-      accumulation.record(1);
-
-      const sdkStartTime: HrTime = [0, 0];
-      const lastCollectionTime: HrTime = [1, 1];
-      const collectionTime: HrTime = [2, 2];
-
-      const expected: MetricData = {
-        resource: defaultResource,
-        instrumentationLibrary: defaultInstrumentationLibrary,
-        instrumentDescriptor: defaultInstrumentDescriptor,
-        pointDataType: PointDataType.SINGULAR,
-        pointData: [
-          {
-            attributes: {},
-            startTime: sdkStartTime,
-            endTime: collectionTime,
-            point: 1,
-          },
-        ],
-      };
-      assert.deepStrictEqual(aggregator.toMetricData(
-        defaultResource,
-        defaultInstrumentationLibrary,
-        defaultInstrumentDescriptor,
-        [[{}, accumulation]],
-        AggregationTemporality.CUMULATIVE,
-        sdkStartTime,
-        lastCollectionTime,
-        collectionTime,
+        startTime,
+        endTime,
       ), expected);
     });
   });