Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: observers should not expose bind/unbind method #1001

Merged
merged 1 commit into from
May 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions packages/opentelemetry-api/src/metrics/BoundInstrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
9 changes: 4 additions & 5 deletions packages/opentelemetry-api/src/metrics/Meter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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<BoundMeasure>;
createMeasure(name: string, options?: MetricOptions): Measure;

/**
* Creates a new `Counter` metric. Generally, this kind of metric when the
Expand All @@ -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<BoundCounter>;
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<BoundObserver>;
createObserver(name: string, options?: MetricOptions): Observer;
}
46 changes: 27 additions & 19 deletions packages/opentelemetry-api/src/metrics/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -77,7 +78,18 @@ export enum ValueType {
* Metric represents a base class for different types of metric
* pre aggregations.
*/
export interface Metric<T> {
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<T> extends Metric {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: pls add JSDoc comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL :)

/**
* Returns a Instrument associated with specified Labels.
* It is recommended to keep a reference to the Instrument instead of always
Expand All @@ -92,31 +104,16 @@ export interface Metric<T> {
* @param labels key-values pairs that are associated with a specific metric.
*/
unbind(labels: Labels): void;
dyladan marked this conversation as resolved.
Show resolved Hide resolved

/**
* Clears all timeseries from the Metric.
*/
clear(): void;
}

export interface MetricUtils {
export interface Counter extends UnboundMetric<BoundCounter> {
/**
* 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<BoundMeasure> {
/**
* Records the given value to this measure.
*/
Expand All @@ -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.
*/
Expand Down
33 changes: 17 additions & 16 deletions packages/opentelemetry-api/src/metrics/NoopMeter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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<BoundMeasure> {
createMeasure(name: string, options?: MetricOptions): Measure {
return NOOP_MEASURE_METRIC;
}

Expand All @@ -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<BoundCounter> {
createCounter(name: string, options?: MetricOptions): Counter {
return NOOP_COUNTER_METRIC;
}

Expand All @@ -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<BoundObserver> {
createObserver(name: string, options?: MetricOptions): Observer {
return NOOP_OBSERVER_METRIC;
}
}

export class NoopMetric<T> implements Metric<T> {
export class NoopMetric<T> implements UnboundMetric<T> {
private readonly _instrument: T;

constructor(instrument: T) {
Expand Down Expand Up @@ -90,14 +97,14 @@ export class NoopMetric<T> implements Metric<T> {
}

export class NoopCounterMetric extends NoopMetric<BoundCounter>
implements Pick<MetricUtils, 'add'> {
implements Counter {
add(value: number, labels: Labels) {
this.bind(labels).add(value);
}
}

export class NoopMeasureMetric extends NoopMetric<BoundMeasure>
implements Pick<MetricUtils, 'record'> {
implements Measure {
record(
value: number,
labels: Labels,
Expand All @@ -114,8 +121,7 @@ export class NoopMeasureMetric extends NoopMetric<BoundMeasure>
}
}

export class NoopObserverMetric extends NoopMetric<BoundObserver>
implements Pick<MetricUtils, 'setCallback'> {
export class NoopObserverMetric extends NoopMetric<void> implements Observer {
setCallback(callback: (observerResult: ObserverResult) => void): void {}
}

Expand All @@ -135,16 +141,11 @@ 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);

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();
9 changes: 1 addition & 8 deletions packages/opentelemetry-metrics/src/BoundInstrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
constructor(
labels: api.Labels,
disabled: boolean,
Expand All @@ -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);
}
}
15 changes: 3 additions & 12 deletions packages/opentelemetry-metrics/src/Meter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<api.BoundMeasure> {
createMeasure(name: string, options?: api.MetricOptions): api.Measure {
if (!this._isValidName(name)) {
this._logger.warn(
`Invalid metric name ${name}. Defaulting to noop metric implementation.`
Expand All @@ -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<api.BoundCounter> {
createCounter(name: string, options?: api.MetricOptions): api.Counter {
if (!this._isValidName(name)) {
this._logger.warn(
`Invalid metric name ${name}. Defaulting to noop metric implementation.`
Expand All @@ -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<api.BoundObserver> {
createObserver(name: string, options?: api.MetricOptions): api.Observer {
if (!this._isValidName(name)) {
this._logger.warn(
`Invalid metric name ${name}. Defaulting to noop metric implementation.`
Expand Down
10 changes: 4 additions & 6 deletions packages/opentelemetry-metrics/src/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { hashLabels } from './Utils';

/** This is a SDK implementation of {@link Metric} interface. */
export abstract class Metric<T extends BaseBoundInstrument>
implements api.Metric<T> {
implements api.UnboundMetric<T> {
protected readonly _monotonic: boolean;
protected readonly _disabled: boolean;
protected readonly _valueType: api.ValueType;
Expand Down Expand Up @@ -106,8 +106,7 @@ export abstract class Metric<T extends BaseBoundInstrument>
}

/** This is a SDK implementation of Counter Metric. */
export class CounterMetric extends Metric<BoundCounter>
implements Pick<api.MetricUtils, 'add'> {
export class CounterMetric extends Metric<BoundCounter> implements api.Counter {
constructor(
name: string,
options: MetricOptions,
Expand Down Expand Up @@ -139,8 +138,7 @@ export class CounterMetric extends Metric<BoundCounter>
}
}

export class MeasureMetric extends Metric<BoundMeasure>
implements Pick<api.MetricUtils, 'record'> {
export class MeasureMetric extends Metric<BoundMeasure> implements api.Measure {
protected readonly _absolute: boolean;

constructor(
Expand Down Expand Up @@ -172,7 +170,7 @@ export class MeasureMetric extends Metric<BoundMeasure>

/** This is a SDK implementation of Observer Metric. */
export class ObserverMetric extends Metric<BoundObserver>
implements Pick<api.MetricUtils, 'setCallback'> {
implements api.Observer {
private _observerResult = new ObserverResult();

constructor(
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-metrics/test/Batcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('Batcher', () => {
let meter: Meter;
let fooCounter: api.BoundCounter;
let barCounter: api.BoundCounter;
let counter: api.Metric<api.BoundCounter>;
let counter: api.Counter;
beforeEach(() => {
meter = new MeterProvider({
logger: new NoopLogger(),
Expand Down