From 470fc6279b531b4388cd8965b5c3cb20e748ea08 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Tue, 4 Feb 2020 17:09:39 -0800 Subject: [PATCH] skeleton(metrics): createMeasure (#563) * feat: implement createMeasure * fix: linting * fix: disable broken tests * fix: labelSet log statement * fix: absolute always set to true * fix: linting * fix: add jsdoc, adjust optionals * fix: failing tests * fix: linting * refactor: rename test handle var Co-authored-by: Mayur Kale --- .../opentelemetry-api/src/metrics/Metric.ts | 9 +- .../src/BoundInstrument.ts | 36 ++++++- packages/opentelemetry-metrics/src/Meter.ts | 20 +++- packages/opentelemetry-metrics/src/Metric.ts | 36 +++++++ .../opentelemetry-metrics/src/export/types.ts | 8 ++ packages/opentelemetry-metrics/src/types.ts | 5 +- .../opentelemetry-metrics/test/Meter.test.ts | 100 ++++++++++++++++++ 7 files changed, 206 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index dffdbe4241..0c6ca61a70 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -49,11 +49,16 @@ export interface MetricOptions { disabled?: boolean; /** - * Monotonic allows this metric to accept negative values. If `true` only - * non-negative values are expected. + * Asserts that this metric may only increase (e.g. time spent). */ monotonic?: 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} diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index a30ed8426a..b72176fae1 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -133,12 +133,44 @@ export class BoundGauge extends BaseBoundInstrument */ export class BoundMeasure extends BaseBoundInstrument implements types.BoundMeasure { + constructor( + labelSet: types.LabelSet, + private readonly _disabled: boolean, + private readonly _absolute: boolean, + private readonly _valueType: types.ValueType, + private readonly _logger: types.Logger, + private readonly _onUpdate: Function + ) { + super(labelSet); + } + record( value: number, distContext?: types.DistributedContext, spanContext?: types.SpanContext ): void { - // @todo: implement this method. - return; + if (this._disabled) return; + + if (this._absolute && value < 0) { + this._logger.error( + `Absolute measure cannot contain negative values for ${Object.values( + this._labelSet.labels + )}}` + ); + return; + } + + if (this._valueType === types.ValueType.INT && !Number.isInteger(value)) { + this._logger.warn( + `INT measure cannot accept a floating-point value for ${Object.values( + this._labelSet.labels + )}; truncating the value.` + ); + value = Math.trunc(value); + } + + //@todo: implement this._data logic + + this._onUpdate(); } } diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index 758bc13292..9d3122b5de 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -17,7 +17,7 @@ import * as types from '@opentelemetry/api'; import { ConsoleLogger } from '@opentelemetry/core'; import { BaseBoundInstrument } from './BoundInstrument'; -import { Metric, CounterMetric, GaugeMetric } from './Metric'; +import { Metric, CounterMetric, GaugeMetric, MeasureMetric } from './Metric'; import { MetricOptions, DEFAULT_METRIC_OPTIONS, @@ -61,8 +61,20 @@ export class Meter implements types.Meter { ); return types.NOOP_MEASURE_METRIC; } - // @todo: implement this method - throw new Error('not implemented yet'); + const opt: MetricOptions = { + // Measures are defined as absolute by default + absolute: true, + monotonic: false, // not applicable to measure, set to false + logger: this._logger, + ...DEFAULT_METRIC_OPTIONS, + ...options, + }; + + const measure = new MeasureMetric(name, opt, () => { + this._exportOneMetric(name); + }); + this._registerMetric(name, measure); + return measure; } /** @@ -85,6 +97,7 @@ export class Meter implements types.Meter { const opt: MetricOptions = { // Counters are defined as monotonic by default monotonic: true, + absolute: false, // not applicable to counter, set to false logger: this._logger, ...DEFAULT_METRIC_OPTIONS, ...options, @@ -117,6 +130,7 @@ export class Meter implements types.Meter { const opt: MetricOptions = { // Gauges are defined as non-monotonic by default monotonic: false, + absolute: false, // not applicable for gauges, set to false logger: this._logger, ...DEFAULT_METRIC_OPTIONS, ...options, diff --git a/packages/opentelemetry-metrics/src/Metric.ts b/packages/opentelemetry-metrics/src/Metric.ts index 4f66dcd8ae..f9a010dfd8 100644 --- a/packages/opentelemetry-metrics/src/Metric.ts +++ b/packages/opentelemetry-metrics/src/Metric.ts @@ -20,6 +20,7 @@ import { BoundCounter, BoundGauge, BaseBoundInstrument, + BoundMeasure, } from './BoundInstrument'; import { MetricOptions } from './types'; import { @@ -199,3 +200,38 @@ export class GaugeMetric extends Metric this.bind(labelSet).set(value); } } + +export class MeasureMetric extends Metric + implements Pick { + protected readonly _absolute: boolean; + + constructor( + name: string, + options: MetricOptions, + private readonly _onUpdate: Function + ) { + super( + name, + options, + options.valueType === types.ValueType.DOUBLE + ? MetricDescriptorType.MEASURE_DOUBLE + : MetricDescriptorType.MEASURE_INT64 + ); + + this._absolute = options.absolute !== undefined ? options.absolute : true; // Absolute default is true + } + protected _makeInstrument(labelSet: types.LabelSet): BoundMeasure { + return new BoundMeasure( + labelSet, + this._disabled, + this._absolute, + this._valueType, + this._logger, + this._onUpdate + ); + } + + record(value: number, labelSet: types.LabelSet) { + this.bind(labelSet).record(value); + } +} diff --git a/packages/opentelemetry-metrics/src/export/types.ts b/packages/opentelemetry-metrics/src/export/types.ts index 22fd4527fe..559b9658d8 100644 --- a/packages/opentelemetry-metrics/src/export/types.ts +++ b/packages/opentelemetry-metrics/src/export/types.ts @@ -122,6 +122,14 @@ export enum MetricDescriptorType { * window. This is not recommended, since it cannot be aggregated. */ SUMMARY, + /** + * Integer measure. The value can be positive or negative. + */ + MEASURE_INT64, + /** + * Floating point measure. The value can be positive or negative. + */ + MEASURE_DOUBLE, } /** diff --git a/packages/opentelemetry-metrics/src/types.ts b/packages/opentelemetry-metrics/src/types.ts index e65d35a29b..cb53e67dad 100644 --- a/packages/opentelemetry-metrics/src/types.ts +++ b/packages/opentelemetry-metrics/src/types.ts @@ -34,12 +34,15 @@ export interface MetricOptions { /** The map of constant labels for the Metric. */ constantLabels?: Map; - /** Indicates the metric is a verbose metric that is disabled by default */ + /** Indicates the metric is a verbose metric that is disabled by default. */ disabled: boolean; /** Monotonic metrics may only increase. */ monotonic: boolean; + /** (Measure only) Asserts that this metric will only accept non-negative values. */ + absolute: boolean; + /** User provided logger. */ logger: Logger; diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 822a83d24f..a155cc7ed9 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -21,6 +21,7 @@ import { CounterMetric, GaugeMetric, MetricDescriptorType, + MeasureMetric, } from '../src'; import * as types from '@opentelemetry/api'; import { LabelSet } from '../src/LabelSet'; @@ -359,6 +360,39 @@ describe('Meter', () => { }); describe('#measure', () => { + it('should create a measure', () => { + const measure = meter.createMeasure('name') as MeasureMetric; + assert.ok(measure instanceof Metric); + }); + + it('should create a measure with options', () => { + const measure = meter.createMeasure('name', { + description: 'desc', + unit: '1', + disabled: false, + }); + assert.ok(measure instanceof Metric); + }); + + it('should be absolute by default', () => { + const measure = meter.createMeasure('name', { + description: 'desc', + unit: '1', + disabled: false, + }); + assert.strictEqual((measure as MeasureMetric)['_absolute'], true); + }); + + it('should be able to set absolute to false', () => { + const measure = meter.createMeasure('name', { + description: 'desc', + unit: '1', + disabled: false, + absolute: false, + }); + assert.strictEqual((measure as MeasureMetric)['_absolute'], false); + }); + describe('names', () => { it('should return no op metric if name is an empty string', () => { const gauge = meter.createMeasure(''); @@ -377,6 +411,72 @@ describe('Meter', () => { assert.ok(gauge instanceof types.NoopMetric); }); }); + + describe('.bind()', () => { + it('should create a measure instrument', () => { + const measure = meter.createMeasure('name') as MeasureMetric; + const boundMeasure = measure.bind(labelSet); + assert.doesNotThrow(() => boundMeasure.record(10)); + }); + + it('should return the timeseries', () => { + // @todo: implement once record is implemented + }); + + it('should not accept negative values by default', () => { + // @todo: implement once record is implemented + }); + + it('should not set the instrument data when disabled', () => { + const measure = meter.createMeasure('name', { + disabled: true, + }) as MeasureMetric; + const boundMeasure = measure.bind(labelSet); + boundMeasure.record(10); + assert.strictEqual(boundMeasure['_data'], 0); + }); + + it('should accept negative (and positive) values when monotonic is set to false', () => { + // @todo: implement once record is implemented + }); + + it('should return same instrument on same label values', () => { + const measure = meter.createMeasure('name') as MeasureMetric; + const boundMeasure1 = measure.bind(labelSet); + boundMeasure1.record(10); + const boundMeasure2 = measure.bind(labelSet); + boundMeasure2.record(100); + // @todo: re-add once record is implemented + // assert.strictEqual(boundMeasure1['_data'], 100); + assert.strictEqual(boundMeasure1, boundMeasure2); + }); + }); + + describe('.unbind()', () => { + it('should remove the measure instrument', () => { + const measure = meter.createMeasure('name') as MeasureMetric; + const boundMeasure = measure.bind(labelSet); + assert.strictEqual(measure['_instruments'].size, 1); + measure.unbind(labelSet); + assert.strictEqual(measure['_instruments'].size, 0); + const boundMeasure2 = measure.bind(labelSet); + assert.strictEqual(measure['_instruments'].size, 1); + assert.notStrictEqual(boundMeasure, boundMeasure2); + }); + + it('should not fail when removing non existing instrument', () => { + const measure = meter.createMeasure('name'); + measure.unbind(new LabelSet('nonexistant', {})); + }); + + it('should clear all instruments', () => { + const measure = meter.createMeasure('name') as MeasureMetric; + measure.bind(labelSet); + assert.strictEqual(measure['_instruments'].size, 1); + measure.clear(); + assert.strictEqual(measure['_instruments'].size, 0); + }); + }); }); describe('#getMetrics', () => {