fix: updates ValueRecorder to allow negative values (#1373)

open-telemetry · Aug 31, 2020 · cb6555a · cb6555a
1 parent b88b95b
commit cb6555a
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 141 deletions.
diff --git a/packages/opentelemetry-api/src/metrics/BoundInstrument.ts b/packages/opentelemetry-api/src/metrics/BoundInstrument.ts
@@ -14,9 +14,6 @@
  * limitations under the License.
  */
 
-import { CorrelationContext } from '../correlation_context/CorrelationContext';
-import { SpanContext } from '../trace/span_context';
-
 /** An Instrument for Counter Metric. */
 export interface BoundCounter {
   /**
@@ -31,18 +28,8 @@ export interface BoundValueRecorder {
   /**
    * Records the given value to this value recorder.
    * @param value to record.
-   * @param correlationContext the correlationContext associated with the
-   *     values.
-   * @param spanContext the {@link SpanContext} that identifies the {@link Span}
-   *     which the values are associated with.
    */
   record(value: number): void;
-  record(value: number, correlationContext: CorrelationContext): void;
-  record(
-    value: number,
-    correlationContext: CorrelationContext,
-    spanContext: SpanContext
-  ): void;
 }
 
 /** An Instrument for Base Observer */

diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts
@@ -14,8 +14,6 @@
  * limitations under the License.
  */
 
-import { CorrelationContext } from '../correlation_context/CorrelationContext';
-import { SpanContext } from '../trace/span_context';
 import {
   BoundBaseObserver,
   BoundCounter,
@@ -51,12 +49,6 @@ export interface MetricOptions {
    */
   disabled?: boolean;
 
-  /**
-   * (Measure only, default true) Asserts that this metric will only accept
-   * non-negative values (e.g. disk usage).
-   */
-  absolute?: boolean;
-
   /**
    * Indicates the type of the recorded value.
    * @default {@link ValueType.DOUBLE}
@@ -148,19 +140,6 @@ export interface ValueRecorder extends UnboundMetric<BoundValueRecorder> {
    * Records the given value to this value recorder.
    */
   record(value: number, labels?: Labels): void;
-
-  record(
-    value: number,
-    labels: Labels,
-    correlationContext: CorrelationContext
-  ): void;
-
-  record(
-    value: number,
-    labels: Labels,
-    correlationContext: CorrelationContext,
-    spanContext: SpanContext
-  ): void;
 }
 
 /** Base interface for the Observer metrics. */

diff --git a/packages/opentelemetry-api/src/metrics/NoopMeter.ts b/packages/opentelemetry-api/src/metrics/NoopMeter.ts
@@ -142,19 +142,8 @@ export class NoopCounterMetric
 export class NoopValueRecorderMetric
   extends NoopMetric<BoundValueRecorder>
   implements ValueRecorder {
-  record(
-    value: number,
-    labels: Labels,
-    correlationContext?: CorrelationContext,
-    spanContext?: SpanContext
-  ) {
-    if (typeof correlationContext === 'undefined') {
-      this.bind(labels).record(value);
-    } else if (typeof spanContext === 'undefined') {
-      this.bind(labels).record(value, correlationContext);
-    } else {
-      this.bind(labels).record(value, correlationContext, spanContext);
-    }
+  record(value: number, labels: Labels) {
+    this.bind(labels).record(value);
   }
 }
 

diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts
@@ -126,42 +126,27 @@ export class BoundUpDownCounter
 export class BoundValueRecorder
   extends BaseBoundInstrument
   implements api.BoundValueRecorder {
-  private readonly _absolute: boolean;
-
   constructor(
     labels: api.Labels,
     disabled: boolean,
-    absolute: boolean,
     valueType: api.ValueType,
     logger: api.Logger,
     aggregator: Aggregator
   ) {
     super(labels, logger, disabled, valueType, aggregator);
-    this._absolute = absolute;
   }
 
-  record(
-    value: number,
-    correlationContext?: api.CorrelationContext,
-    spanContext?: api.SpanContext
-  ): void {
-    if (this._absolute && value < 0) {
-      this._logger.error(
-        `Absolute ValueRecorder cannot contain negative values for $${Object.values(
-          this._labels
-        )}`
-      );
-      return;
-    }
-
+  record(value: number): void {
     this.update(value);
   }
 }
 
 /**
  * BoundObserver is an implementation of the {@link BoundObserver} interface.
  */
-export class BoundObserver extends BaseBoundInstrument {
+export class BoundObserver
+  extends BaseBoundInstrument
+  implements api.BoundBaseObserver {
   constructor(
     labels: api.Labels,
     disabled: boolean,

diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts
@@ -77,7 +77,6 @@ export class Meter implements api.Meter {
     const opt: api.MetricOptions = {
       logger: this._logger,
       ...DEFAULT_METRIC_OPTIONS,
-      absolute: true, // value recorders are defined as absolute by default
       ...options,
     };
 

diff --git a/packages/opentelemetry-metrics/src/ValueRecorderMetric.ts b/packages/opentelemetry-metrics/src/ValueRecorderMetric.ts
@@ -26,8 +26,6 @@ import { Metric } from './Metric';
 export class ValueRecorderMetric
   extends Metric<BoundValueRecorder>
   implements api.ValueRecorder {
-  protected readonly _absolute: boolean;
-
   constructor(
     name: string,
     options: api.MetricOptions,
@@ -42,14 +40,12 @@ export class ValueRecorderMetric
       resource,
       instrumentationLibrary
     );
-
-    this._absolute = options.absolute !== undefined ? options.absolute : true; // Absolute default is true
   }
+
   protected _makeInstrument(labels: api.Labels): BoundValueRecorder {
     return new BoundValueRecorder(
       labels,
       this._disabled,
-      this._absolute,
       this._valueType,
       this._logger,
       this._batcher.aggregatorFor(this._descriptor)

diff --git a/packages/opentelemetry-metrics/src/types.ts b/packages/opentelemetry-metrics/src/types.ts
@@ -53,7 +53,6 @@ export const DEFAULT_CONFIG = {
 /** The default metric creation options value. */
 export const DEFAULT_METRIC_OPTIONS = {
   disabled: false,
-  absolute: false,
   description: '',
   unit: '1',
   valueType: api.ValueType.DOUBLE,

diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts
@@ -555,31 +555,6 @@ describe('Meter', () => {
       assert.ok(valueRecorder instanceof Metric);
     });
 
-    it('should be absolute by default', () => {
-      const valueRecorder = meter.createValueRecorder('name', {
-        description: 'desc',
-        unit: '1',
-        disabled: false,
-      });
-      assert.strictEqual(
-        (valueRecorder as ValueRecorderMetric)['_absolute'],
-        true
-      );
-    });
-
-    it('should be able to set absolute to false', () => {
-      const valueRecorder = meter.createValueRecorder('name', {
-        description: 'desc',
-        unit: '1',
-        disabled: false,
-        absolute: false,
-      });
-      assert.strictEqual(
-        (valueRecorder as ValueRecorderMetric)['_absolute'],
-        false
-      );
-    });
-
     it('should pipe through resource', async () => {
       const valueRecorder = meter.createValueRecorder(
         'name'
@@ -638,10 +613,12 @@ describe('Meter', () => {
         assert.doesNotThrow(() => boundValueRecorder.record(10));
       });
 
-      it('should not accept negative values by default', async () => {
-        const valueRecorder = meter.createValueRecorder('name');
+      it('should not set the instrument data when disabled', async () => {
+        const valueRecorder = meter.createValueRecorder('name', {
+          disabled: true,
+        }) as ValueRecorderMetric;
         const boundValueRecorder = valueRecorder.bind(labels);
-        boundValueRecorder.record(-10);
+        boundValueRecorder.record(10);
 
         await meter.collect();
         const [record1] = meter.getBatcher().checkPointSet();
@@ -657,57 +634,30 @@ describe('Meter', () => {
         );
       });
 
-      it('should not set the instrument data when disabled', async () => {
-        const valueRecorder = meter.createValueRecorder('name', {
-          disabled: true,
-        }) as ValueRecorderMetric;
+      it('should accept negative (and positive) values', async () => {
+        const valueRecorder = meter.createValueRecorder('name');
         const boundValueRecorder = valueRecorder.bind(labels);
-        boundValueRecorder.record(10);
+        boundValueRecorder.record(-10);
+        boundValueRecorder.record(50);
 
         await meter.collect();
         const [record1] = meter.getBatcher().checkPointSet();
         assert.deepStrictEqual(
           record1.aggregator.toPoint().value as Distribution,
           {
-            count: 0,
-            last: 0,
-            max: -Infinity,
-            min: Infinity,
-            sum: 0,
+            count: 2,
+            last: 50,
+            max: 50,
+            min: -10,
+            sum: 40,
           }
         );
+        assert.ok(
+          hrTimeToNanoseconds(record1.aggregator.toPoint().timestamp) >
+            hrTimeToNanoseconds(performanceTimeOrigin)
+        );
       });
 
-      it(
-        'should accept negative (and positive) values when absolute is set' +
-          ' to false',
-        async () => {
-          const valueRecorder = meter.createValueRecorder('name', {
-            absolute: false,
-          });
-          const boundValueRecorder = valueRecorder.bind(labels);
-          boundValueRecorder.record(-10);
-          boundValueRecorder.record(50);
-
-          await meter.collect();
-          const [record1] = meter.getBatcher().checkPointSet();
-          assert.deepStrictEqual(
-            record1.aggregator.toPoint().value as Distribution,
-            {
-              count: 2,
-              last: 50,
-              max: 50,
-              min: -10,
-              sum: 40,
-            }
-          );
-          assert.ok(
-            hrTimeToNanoseconds(record1.aggregator.toPoint().timestamp) >
-              hrTimeToNanoseconds(performanceTimeOrigin)
-          );
-        }
-      );
-
       it('should return same instrument on same label values', async () => {
         const valueRecorder = meter.createValueRecorder(
           'name'