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

feat: direct calling of metric instruments #507

Merged
merged 11 commits into from
Nov 12, 2019
31 changes: 24 additions & 7 deletions packages/opentelemetry-core/src/metrics/NoopMeter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
Meter,
Metric,
MetricOptions,
MetricUtils,
MeasureHandle,
SpanContext,
LabelSet,
Expand Down Expand Up @@ -110,6 +111,26 @@ export class NoopMetric<T> implements Metric<T> {
}
}

export class NoopCounterMetric extends NoopMetric<CounterHandle>
implements Pick<MetricUtils, 'add'> {
add(value: number, labelSet: LabelSet) {
this.getHandle(labelSet).add(value);
}
}

export class NoopGaugeMetric extends NoopMetric<GaugeHandle>
implements Pick<MetricUtils, 'set'> {
set(value: number, labelSet: LabelSet) {
this.getHandle(labelSet).set(value);
}
}

export class NoopMeasureMetric extends NoopMetric<MeasureHandle> {
record(value: number, labelSet: LabelSet) {
this.getHandle(labelSet).record(value);
}
}

export class NoopCounterHandle implements CounterHandle {
add(value: number): void {
return;
Expand All @@ -133,16 +154,12 @@ export class NoopMeasureHandle implements MeasureHandle {
}

export const NOOP_GAUGE_HANDLE = new NoopGaugeHandle();
export const NOOP_GAUGE_METRIC = new NoopMetric<GaugeHandle>(NOOP_GAUGE_HANDLE);
export const NOOP_GAUGE_METRIC = new NoopGaugeMetric(NOOP_GAUGE_HANDLE);

export const NOOP_COUNTER_HANDLE = new NoopCounterHandle();
export const NOOP_COUNTER_METRIC = new NoopMetric<CounterHandle>(
NOOP_COUNTER_HANDLE
);
export const NOOP_COUNTER_METRIC = new NoopCounterMetric(NOOP_COUNTER_HANDLE);

export const NOOP_MEASURE_HANDLE = new NoopMeasureHandle();
export const NOOP_MEASURE_METRIC = new NoopMetric<MeasureHandle>(
NOOP_MEASURE_HANDLE
);
export const NOOP_MEASURE_METRIC = new NoopMeasureMetric(NOOP_MEASURE_HANDLE);

export const NOOP_LABEL_SET = {} as LabelSet;
9 changes: 0 additions & 9 deletions packages/opentelemetry-metrics/src/LabelSet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,3 @@ export class LabelSet implements types.LabelSet {
this.labels = labels;
}
}

/**
* Type guard to remove nulls from arrays
*
* @param value value to be checked for null equality
*/
export function notNull<T>(value: T | null): value is T {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is in Utils.ts, accidentally added here, just clear it up.

return value !== null;
}
24 changes: 22 additions & 2 deletions packages/opentelemetry-metrics/src/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ export abstract class Metric<T extends BaseHandle> implements types.Metric<T> {
}

/** This is a SDK implementation of Counter Metric. */
export class CounterMetric extends Metric<CounterHandle> {
export class CounterMetric extends Metric<CounterHandle>
implements Pick<types.MetricUtils, 'add'> {
constructor(
name: string,
options: MetricOptions,
Expand All @@ -145,10 +146,20 @@ export class CounterMetric extends Metric<CounterHandle> {
this._onUpdate
);
}

/**
* Adds the given value to the current value. Values cannot be negative.
* @param value the value to add.
* @param labelSet the canonicalized LabelSet used to associate with this metric's handle.
*/
add(value: number, labelSet: types.LabelSet) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should include this on the top level API in types package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I thought about it, do you think it makes sense to add a update(value, labelSet) in types/Metric you linked to? thanks~

Copy link
Member

Choose a reason for hiding this comment

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

Maybe record(value, labelSet), if we can't use add(value, labelSet) and set(value, labelSet). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it be a little bit confused with measure's record function? WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm makes sense. I am ok with current approach for now. Let's wait for others to comment on this.

Copy link
Member

Choose a reason for hiding this comment

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

You could add something like this to the Metric type:

export interface Metric<T> {
  ...
  
  add: T extends CounterHandle ? CounterHandle["add"] : never;
  set: T extends GaugeHandle ? GaugeHandle["set"] : never;
  record: T extends MeasureHandle ? MeasureHandle["record"] : never;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dyladan Thanks for your advice. I didn't make your suggestion work.. but it helps me find out Pick interface that enables what we need here. Please take a look and let me know if it works. 😁

this.getHandle(labelSet).add(value);
}
}

/** This is a SDK implementation of Gauge Metric. */
export class GaugeMetric extends Metric<GaugeHandle> {
export class GaugeMetric extends Metric<GaugeHandle>
implements Pick<types.MetricUtils, 'set'> {
constructor(
name: string,
options: MetricOptions,
Expand All @@ -172,4 +183,13 @@ export class GaugeMetric extends Metric<GaugeHandle> {
this._onUpdate
);
}

/**
* Sets the given value. Values can be negative.
* @param value the new value.
* @param labelSet the canonicalized LabelSet used to associate with this metric's handle.
*/
set(value: number, labelSet: types.LabelSet) {
this.getHandle(labelSet).set(value);
}
}
16 changes: 16 additions & 0 deletions packages/opentelemetry-metrics/test/Meter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ describe('Meter', () => {
assert.ok(counter instanceof Metric);
});

it('should be able to call add() directly on counter', () => {
const counter = meter.createCounter('name') as CounterMetric;
counter.add(10, labelSet);
assert.strictEqual(counter.getHandle(labelSet)['_data'], 10);
counter.add(10, labelSet);
assert.strictEqual(counter.getHandle(labelSet)['_data'], 20);
});

describe('.getHandle()', () => {
it('should create a counter handle', () => {
const counter = meter.createCounter('name') as CounterMetric;
Expand Down Expand Up @@ -229,6 +237,14 @@ describe('Meter', () => {
assert.ok(gauge instanceof Metric);
});

it('should be able to call set() directly on gauge', () => {
const gauge = meter.createGauge('name') as GaugeMetric;
gauge.set(10, labelSet);
assert.strictEqual(gauge.getHandle(labelSet)['_data'], 10);
gauge.set(250, labelSet);
assert.strictEqual(gauge.getHandle(labelSet)['_data'], 250);
});

describe('.getHandle()', () => {
it('should create a gauge handle', () => {
const gauge = meter.createGauge('name') as GaugeMetric;
Expand Down
17 changes: 17 additions & 0 deletions packages/opentelemetry-types/src/metrics/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,23 @@ export interface Metric<T> {
setCallback(fn: () => void): void;
}

export interface MetricUtils {
/**
* Adds the given value to the current value. Values cannot be negative.
*/
add(value: number, labelSet: LabelSet): void;
/**
* Sets the given value. Values can be negative.
*/

set(value: number, labelSet: LabelSet): void;
/**
* Records the given value to this measure.
*/

record(value: number, labelSet: LabelSet): void;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the overload versions that take distributed context and span context?

See here:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, fixed it, thanks.

}

/**
* key-value pairs passed by the user.
*/
Expand Down