From 0b4c2c6909fa28dc993b8e8522b433fba25d5b18 Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 30 Apr 2020 16:27:19 +0800 Subject: [PATCH] fix: observers should not expose bind/unbind method --- .../src/metrics/BoundInstrument.ts | 12 ------ .../opentelemetry-api/src/metrics/Meter.ts | 14 ++++--- .../opentelemetry-api/src/metrics/Metric.ts | 42 ++++++++++--------- .../src/metrics/NoopMeter.ts | 34 ++++++++------- .../src/BoundInstrument.ts | 9 +--- packages/opentelemetry-metrics/src/Meter.ts | 6 +-- packages/opentelemetry-metrics/src/Metric.ts | 8 ++-- .../test/Batcher.test.ts | 2 +- 8 files changed, 59 insertions(+), 68 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/BoundInstrument.ts b/packages/opentelemetry-api/src/metrics/BoundInstrument.ts index 87defee0dcf..07302bd22fb 100644 --- a/packages/opentelemetry-api/src/metrics/BoundInstrument.ts +++ b/packages/opentelemetry-api/src/metrics/BoundInstrument.ts @@ -16,7 +16,6 @@ import { CorrelationContext } from '../correlation_context/CorrelationContext'; import { SpanContext } from '../trace/span_context'; -import { ObserverResult } from './ObserverResult'; /** An Instrument for Counter Metric. */ export interface BoundCounter { @@ -45,14 +44,3 @@ export interface BoundMeasure { spanContext: SpanContext ): void; } - -/** Base interface for the Observer metrics. */ -export interface BoundObserver { - /** - * Sets callback for the observer. The callback is called once and then it - * sets observers for values. The observers are called periodically to - * retrieve the value. - * @param callback - */ - setCallback(callback: (observerResult: ObserverResult) => void): void; -} diff --git a/packages/opentelemetry-api/src/metrics/Meter.ts b/packages/opentelemetry-api/src/metrics/Meter.ts index 5231629c910..47fe8fa16ef 100644 --- a/packages/opentelemetry-api/src/metrics/Meter.ts +++ b/packages/opentelemetry-api/src/metrics/Meter.ts @@ -14,8 +14,12 @@ * limitations under the License. */ -import { Metric, MetricOptions } from './Metric'; -import { BoundCounter, BoundMeasure, BoundObserver } from './BoundInstrument'; +import { + MetricOptions, + UnboundCounterMetric, + UnboundMeasureMetric, + ObserverMetric, +} from './Metric'; /** * An interface to allow the recording metrics. @@ -30,7 +34,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createMeasure(name: string, options?: MetricOptions): Metric; + createMeasure(name: string, options?: MetricOptions): UnboundMeasureMetric; /** * Creates a new `Counter` metric. Generally, this kind of metric when the @@ -39,12 +43,12 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createCounter(name: string, options?: MetricOptions): Metric; + createCounter(name: string, options?: MetricOptions): UnboundCounterMetric; /** * Creates a new `Observer` metric. * @param name the name of the metric. * @param [options] the metric options. */ - createObserver(name: string, options?: MetricOptions): Metric; + createObserver(name: string, options?: MetricOptions): ObserverMetric; } diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index d57896a1d32..937fb1da0c8 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -17,6 +17,7 @@ import { CorrelationContext } from '../correlation_context/CorrelationContext'; import { SpanContext } from '../trace/span_context'; import { ObserverResult } from './ObserverResult'; +import { BoundCounter, BoundMeasure } from './BoundInstrument'; /** * Options needed for metric creation @@ -77,7 +78,14 @@ export enum ValueType { * Metric represents a base class for different types of metric * pre aggregations. */ -export interface Metric { +export interface Metric { + /** + * Clears all timeseries from the Metric. + */ + clear(): void; +} + +export interface UnboundMetric extends Metric { /** * Returns a Instrument associated with specified Labels. * It is recommended to keep a reference to the Instrument instead of always @@ -92,31 +100,16 @@ export interface Metric { * @param labels key-values pairs that are associated with a specific metric. */ unbind(labels: Labels): void; - - /** - * Clears all timeseries from the Metric. - */ - clear(): void; } -export interface MetricUtils { +export interface UnboundCounterMetric extends UnboundMetric { /** * Adds the given value to the current value. Values cannot be negative. */ add(value: number, labels: Labels): void; +} - /** - * Sets a callback where user can observe value for certain labels - * @param callback a function that will be called once to set observers - * for values - */ - setCallback(callback: (observerResult: ObserverResult) => void): void; - - /** - * Sets the given value. Values can be negative. - */ - set(value: number, labels: Labels): void; - +export interface UnboundMeasureMetric extends UnboundMetric { /** * Records the given value to this measure. */ @@ -136,6 +129,17 @@ export interface MetricUtils { ): void; } +/** Base interface for the Observer metrics. */ +export interface ObserverMetric extends Metric { + /** + * Sets a callback where user can observe value for certain labels. The + * observers are called periodically to retrieve the value. + * @param callback a function that will be called once to set observers + * for values + */ + setCallback(callback: (observerResult: ObserverResult) => void): void; +} + /** * key-value pairs passed by the user. */ diff --git a/packages/opentelemetry-api/src/metrics/NoopMeter.ts b/packages/opentelemetry-api/src/metrics/NoopMeter.ts index bfc2e70a930..4e4a316df6e 100644 --- a/packages/opentelemetry-api/src/metrics/NoopMeter.ts +++ b/packages/opentelemetry-api/src/metrics/NoopMeter.ts @@ -15,8 +15,15 @@ */ import { Meter } from './Meter'; -import { MetricOptions, Metric, Labels, MetricUtils } from './Metric'; -import { BoundMeasure, BoundCounter, BoundObserver } from './BoundInstrument'; +import { + MetricOptions, + UnboundMetric, + Labels, + UnboundCounterMetric, + UnboundMeasureMetric, + ObserverMetric, +} from './Metric'; +import { BoundMeasure, BoundCounter } from './BoundInstrument'; import { CorrelationContext } from '../correlation_context/CorrelationContext'; import { SpanContext } from '../trace/span_context'; import { ObserverResult } from './ObserverResult'; @@ -33,7 +40,7 @@ export class NoopMeter implements Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createMeasure(name: string, options?: MetricOptions): Metric { + createMeasure(name: string, options?: MetricOptions): UnboundMeasureMetric { return NOOP_MEASURE_METRIC; } @@ -42,7 +49,7 @@ export class NoopMeter implements Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createCounter(name: string, options?: MetricOptions): Metric { + createCounter(name: string, options?: MetricOptions): UnboundCounterMetric { return NOOP_COUNTER_METRIC; } @@ -51,12 +58,12 @@ export class NoopMeter implements Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createObserver(name: string, options?: MetricOptions): Metric { + createObserver(name: string, options?: MetricOptions): ObserverMetric { return NOOP_OBSERVER_METRIC; } } -export class NoopMetric implements Metric { +export class NoopMetric implements UnboundMetric { private readonly _instrument: T; constructor(instrument: T) { @@ -90,14 +97,14 @@ export class NoopMetric implements Metric { } export class NoopCounterMetric extends NoopMetric - implements Pick { + implements UnboundCounterMetric { add(value: number, labels: Labels) { this.bind(labels).add(value); } } export class NoopMeasureMetric extends NoopMetric - implements Pick { + implements UnboundMeasureMetric { record( value: number, labels: Labels, @@ -114,8 +121,8 @@ export class NoopMeasureMetric extends NoopMetric } } -export class NoopObserverMetric extends NoopMetric - implements Pick { +export class NoopObserverMetric extends NoopMetric + implements ObserverMetric { setCallback(callback: (observerResult: ObserverResult) => void): void {} } @@ -135,10 +142,6 @@ export class NoopBoundMeasure implements BoundMeasure { } } -export class NoopBoundObserver implements BoundObserver { - setCallback(callback: (observerResult: ObserverResult) => void): void {} -} - export const NOOP_METER = new NoopMeter(); export const NOOP_BOUND_COUNTER = new NoopBoundCounter(); export const NOOP_COUNTER_METRIC = new NoopCounterMetric(NOOP_BOUND_COUNTER); @@ -146,5 +149,4 @@ export const NOOP_COUNTER_METRIC = new NoopCounterMetric(NOOP_BOUND_COUNTER); export const NOOP_BOUND_MEASURE = new NoopBoundMeasure(); export const NOOP_MEASURE_METRIC = new NoopMeasureMetric(NOOP_BOUND_MEASURE); -export const NOOP_BOUND_OBSERVER = new NoopBoundObserver(); -export const NOOP_OBSERVER_METRIC = new NoopObserverMetric(NOOP_BOUND_OBSERVER); +export const NOOP_OBSERVER_METRIC = new NoopObserverMetric(); diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index e7db73dd401..c8e20a08f45 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -16,7 +16,6 @@ import * as types from '@opentelemetry/api'; import { Aggregator } from './export/types'; -import { ObserverResult } from './ObserverResult'; /** * This class represent the base to BoundInstrument, which is responsible for generating @@ -134,8 +133,7 @@ export class BoundMeasure extends BaseBoundInstrument /** * BoundObserver is an implementation of the {@link BoundObserver} interface. */ -export class BoundObserver extends BaseBoundInstrument - implements types.BoundObserver { +export class BoundObserver extends BaseBoundInstrument { constructor( labels: types.Labels, disabled: boolean, @@ -146,9 +144,4 @@ export class BoundObserver extends BaseBoundInstrument ) { super(labels, logger, monotonic, disabled, valueType, aggregator); } - - setCallback(callback: (observerResult: types.ObserverResult) => void): void { - const observerResult = new ObserverResult(); - callback(observerResult); - } } diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index 091a96b435d..aad4d95ee0e 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -59,7 +59,7 @@ export class Meter implements types.Meter { createMeasure( name: string, options?: types.MetricOptions - ): types.Metric { + ): types.UnboundMeasureMetric { if (!this._isValidName(name)) { this._logger.warn( `Invalid metric name ${name}. Defaulting to noop metric implementation.` @@ -89,7 +89,7 @@ export class Meter implements types.Meter { createCounter( name: string, options?: types.MetricOptions - ): types.Metric { + ): types.UnboundCounterMetric { if (!this._isValidName(name)) { this._logger.warn( `Invalid metric name ${name}. Defaulting to noop metric implementation.` @@ -116,7 +116,7 @@ export class Meter implements types.Meter { createObserver( name: string, options?: types.MetricOptions - ): types.Metric { + ): types.ObserverMetric { if (!this._isValidName(name)) { this._logger.warn( `Invalid metric name ${name}. Defaulting to noop metric implementation.` diff --git a/packages/opentelemetry-metrics/src/Metric.ts b/packages/opentelemetry-metrics/src/Metric.ts index cd666a210e1..a1103fc2baa 100644 --- a/packages/opentelemetry-metrics/src/Metric.ts +++ b/packages/opentelemetry-metrics/src/Metric.ts @@ -30,7 +30,7 @@ import { hashLabels } from './Utils'; /** This is a SDK implementation of {@link Metric} interface. */ export abstract class Metric - implements api.Metric { + implements api.UnboundMetric { protected readonly _monotonic: boolean; protected readonly _disabled: boolean; protected readonly _valueType: api.ValueType; @@ -107,7 +107,7 @@ export abstract class Metric /** This is a SDK implementation of Counter Metric. */ export class CounterMetric extends Metric - implements Pick { + implements api.UnboundCounterMetric { constructor( name: string, options: MetricOptions, @@ -140,7 +140,7 @@ export class CounterMetric extends Metric } export class MeasureMetric extends Metric - implements Pick { + implements api.UnboundMeasureMetric { protected readonly _absolute: boolean; constructor( @@ -172,7 +172,7 @@ export class MeasureMetric extends Metric /** This is a SDK implementation of Observer Metric. */ export class ObserverMetric extends Metric - implements Pick { + implements api.ObserverMetric { private _observerResult = new ObserverResult(); constructor( diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts index c09998bd32d..7dce7513033 100644 --- a/packages/opentelemetry-metrics/test/Batcher.test.ts +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -24,7 +24,7 @@ describe('Batcher', () => { let meter: Meter; let fooCounter: types.BoundCounter; let barCounter: types.BoundCounter; - let counter: types.Metric; + let counter: types.UnboundMetric; beforeEach(() => { meter = new MeterProvider({ logger: new NoopLogger(),