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.

Sign up for GitHub

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
Merged

feat: direct calling of metric instruments #507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
48e3faa
feat: direct calling of metric instruments
xiao-lix Nov 7, 2019
1758fd0
fix: add interface in types->metric
xiao-lix Nov 8, 2019
ced8cdf
fix: linting
xiao-lix Nov 8, 2019
48349c6
fix: linting & docs
xiao-lix Nov 8, 2019
5442f7d
fix: add overloads for record in measure metric
xiao-lix Nov 9, 2019
c6ccc14
fix: linting
xiao-lix Nov 11, 2019
fa78b80
Merge branch 'master' into xiao/direct-call
xiao-lix Nov 11, 2019
1445c07
fix: linting
xiao-lix Nov 11, 2019
4dbcbe0
Merge branch 'master' into xiao/direct-call
mayurkale22 Nov 12, 2019
9a19964
fix: linting
xiao-lix Nov 12, 2019
3c1e6ec
Merge branch 'master' into xiao/direct-call
mayurkale22 Nov 12, 2019
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
9 changes: 0 additions & 9 deletions packages/opentelemetry-metrics/src/LabelSet.ts
View file
Open in desktop
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;
}
18 changes: 18 additions & 0 deletions packages/opentelemetry-metrics/src/Metric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,15 @@ 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. */
Expand Down Expand Up @@ -172,4 +181,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
View file
Open in desktop
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