From bfc2fb1e59e342a7c8d0b4864cb99f81b609c77a 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 | 9 ++-- .../opentelemetry-api/src/metrics/Metric.ts | 46 +++++++++++-------- .../src/metrics/NoopMeter.ts | 33 ++++++------- .../src/BoundInstrument.ts | 9 +--- packages/opentelemetry-metrics/src/Meter.ts | 15 ++---- packages/opentelemetry-metrics/src/Metric.ts | 10 ++-- .../test/Batcher.test.ts | 2 +- 8 files changed, 57 insertions(+), 79 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/BoundInstrument.ts b/packages/opentelemetry-api/src/metrics/BoundInstrument.ts index 87defee0dc..07302bd22f 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 5231629c91..0b43708c4d 100644 --- a/packages/opentelemetry-api/src/metrics/Meter.ts +++ b/packages/opentelemetry-api/src/metrics/Meter.ts @@ -14,8 +14,7 @@ * limitations under the License. */ -import { Metric, MetricOptions } from './Metric'; -import { BoundCounter, BoundMeasure, BoundObserver } from './BoundInstrument'; +import { MetricOptions, Counter, Measure, Observer } from './Metric'; /** * An interface to allow the recording metrics. @@ -30,7 +29,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): Measure; /** * Creates a new `Counter` metric. Generally, this kind of metric when the @@ -39,12 +38,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): Counter; /** * 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): Observer; } diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index d57896a1d3..4b02812723 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,18 @@ export enum ValueType { * Metric represents a base class for different types of metric * pre aggregations. */ -export interface Metric { +export interface Metric { + /** + * Clears all bound instruments from the Metric. + */ + clear(): void; +} + +/** + * UnboundMetric represents a base class for different types of metric + * pre aggregations without label value bound yet. + */ +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 +104,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 Counter 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 Measure extends UnboundMetric { /** * Records the given value to this measure. */ @@ -136,6 +133,17 @@ export interface MetricUtils { ): void; } +/** Base interface for the Observer metrics. */ +export interface Observer 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 bfc2e70a93..dcd6a5dea1 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, + Counter, + Measure, + Observer, +} 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): Measure { 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): Counter { 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): Observer { 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 Counter { add(value: number, labels: Labels) { this.bind(labels).add(value); } } export class NoopMeasureMetric extends NoopMetric - implements Pick { + implements Measure { record( value: number, labels: Labels, @@ -114,8 +121,7 @@ export class NoopMeasureMetric extends NoopMetric } } -export class NoopObserverMetric extends NoopMetric - implements Pick { +export class NoopObserverMetric extends NoopMetric implements Observer { setCallback(callback: (observerResult: ObserverResult) => void): void {} } @@ -135,10 +141,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 +148,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 94338a29eb..d451bcb601 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -16,7 +16,6 @@ import * as api 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 api.BoundObserver { +export class BoundObserver extends BaseBoundInstrument { constructor( labels: api.Labels, disabled: boolean, @@ -146,9 +144,4 @@ export class BoundObserver extends BaseBoundInstrument ) { super(labels, logger, monotonic, disabled, valueType, aggregator); } - - setCallback(callback: (observerResult: api.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 425af55c20..ee2d5a5427 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -56,10 +56,7 @@ export class Meter implements api.Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createMeasure( - name: string, - options?: api.MetricOptions - ): api.Metric { + createMeasure(name: string, options?: api.MetricOptions): api.Measure { if (!this._isValidName(name)) { this._logger.warn( `Invalid metric name ${name}. Defaulting to noop metric implementation.` @@ -86,10 +83,7 @@ export class Meter implements api.Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createCounter( - name: string, - options?: api.MetricOptions - ): api.Metric { + createCounter(name: string, options?: api.MetricOptions): api.Counter { if (!this._isValidName(name)) { this._logger.warn( `Invalid metric name ${name}. Defaulting to noop metric implementation.` @@ -113,10 +107,7 @@ export class Meter implements api.Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createObserver( - name: string, - options?: api.MetricOptions - ): api.Metric { + createObserver(name: string, options?: api.MetricOptions): api.Observer { 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 0fce307d6a..ce9d89df52 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; @@ -106,8 +106,7 @@ export abstract class Metric } /** This is a SDK implementation of Counter Metric. */ -export class CounterMetric extends Metric - implements Pick { +export class CounterMetric extends Metric implements api.Counter { constructor( name: string, options: MetricOptions, @@ -139,8 +138,7 @@ export class CounterMetric extends Metric } } -export class MeasureMetric extends Metric - implements Pick { +export class MeasureMetric extends Metric implements api.Measure { protected readonly _absolute: boolean; constructor( @@ -172,7 +170,7 @@ export class MeasureMetric extends Metric /** This is a SDK implementation of Observer Metric. */ export class ObserverMetric extends Metric - implements Pick { + implements api.Observer { private _observerResult = new ObserverResult(); constructor( diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts index 5c2e00b3fb..72c68608bb 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: api.BoundCounter; let barCounter: api.BoundCounter; - let counter: api.Metric; + let counter: api.Counter; beforeEach(() => { meter = new MeterProvider({ logger: new NoopLogger(),