From 2347435cc64b7295bb17554a31032d8f70a4be2b Mon Sep 17 00:00:00 2001 From: Michael Goin Date: Fri, 31 Jul 2020 15:23:56 -0700 Subject: [PATCH 1/2] fix: updates ValueRecorder to allow negative values --- .../src/metrics/BoundInstrument.ts | 13 --- .../opentelemetry-api/src/metrics/Metric.ts | 21 ----- .../src/metrics/NoopMeter.ts | 15 +--- .../src/BoundInstrument.ts | 22 +---- packages/opentelemetry-metrics/src/Meter.ts | 1 - .../src/ValueRecorderMetric.ts | 6 +- packages/opentelemetry-metrics/src/types.ts | 1 - .../opentelemetry-metrics/test/Meter.test.ts | 86 ++++--------------- 8 files changed, 24 insertions(+), 141 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/BoundInstrument.ts b/packages/opentelemetry-api/src/metrics/BoundInstrument.ts index 4ffbd143c7..0d5554771e 100644 --- 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 index ee7fd29ab4..0022c2a5b6 100644 --- 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 { * 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 index 47df1ab970..f3c8feeddd 100644 --- a/packages/opentelemetry-api/src/metrics/NoopMeter.ts +++ b/packages/opentelemetry-api/src/metrics/NoopMeter.ts @@ -140,19 +140,8 @@ export class NoopCounterMetric extends NoopMetric export class NoopValueRecorderMetric extends NoopMetric 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 index dad6ce01ec..7dedb88164 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -115,34 +115,17 @@ export class BoundUpDownCounter extends BaseBoundInstrument */ 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); } } @@ -150,7 +133,8 @@ export class BoundValueRecorder extends BaseBoundInstrument /** * 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 index e6c3362113..e4826ff70e 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -76,7 +76,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 index e23e3daa72..fa50f9b3a6 100644 --- a/packages/opentelemetry-metrics/src/ValueRecorderMetric.ts +++ b/packages/opentelemetry-metrics/src/ValueRecorderMetric.ts @@ -25,8 +25,6 @@ import { Metric } from './Metric'; /** This is a SDK implementation of Value Recorder Metric. */ export class ValueRecorderMetric extends Metric implements api.ValueRecorder { - protected readonly _absolute: boolean; - constructor( name: string, options: api.MetricOptions, @@ -41,14 +39,12 @@ export class ValueRecorderMetric extends Metric 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 index cc01af9f8d..1eed57a46e 100644 --- a/packages/opentelemetry-metrics/src/types.ts +++ b/packages/opentelemetry-metrics/src/types.ts @@ -49,7 +49,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 index eb6f3d3d0a..1612fa2795 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -476,31 +476,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' @@ -559,10 +534,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(); @@ -578,57 +555,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' From 2c04e1d9275f76166d38dc3033356fabc2b1fd0d Mon Sep 17 00:00:00 2001 From: Michael Goin Date: Thu, 27 Aug 2020 10:59:21 -0700 Subject: [PATCH 2/2] style: lint:fix --- packages/opentelemetry-metrics/src/BoundInstrument.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index 092a75ec66..e84bb72487 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -144,7 +144,8 @@ export class BoundValueRecorder /** * 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,