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

Conversation

xiao-lix
Copy link
Contributor

@xiao-lix xiao-lix commented Nov 7, 2019

Which problem is this PR solving?

Short description of the changes

  • Implement direct calling convention of metric instruments

*
* @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.

@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #507 into master will increase coverage by 0.17%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##           master     #507      +/-   ##
==========================================
+ Coverage   90.24%   90.42%   +0.17%     
==========================================
  Files         142      144       +2     
  Lines        7064     7309     +245     
  Branches      645      650       +5     
==========================================
+ Hits         6375     6609     +234     
- Misses        689      700      +11
Impacted Files Coverage Δ
packages/opentelemetry-metrics/src/LabelSet.ts 100% <ø> (+16.66%) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 89.09% <100%> (+0.85%) ⬆️
packages/opentelemetry-metrics/test/Meter.test.ts 100% <100%> (ø) ⬆️
...ckages/opentelemetry-core/src/metrics/NoopMeter.ts 80.76% <37.5%> (-19.24%) ⬇️
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0%> (-9.53%) ⬇️
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/config.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/utility.ts 100% <0%> (ø) ⬆️
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100% <0%> (ø) ⬆️
...telemetry-plugin-grpc/test/utils/assertionUtils.ts 100% <0%> (ø) ⬆️
... and 12 more

* @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. 😁

* 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.

@OlivierAlbertini OlivierAlbertini added the enhancement New feature or request label Nov 9, 2019
@mayurkale22
Copy link
Member

@open-telemetry/javascript-approvers Please review and approve if looks good.

@mayurkale22 mayurkale22 merged commit 668c3aa into open-telemetry:master Nov 12, 2019
@xiao-lix xiao-lix deleted the xiao/direct-call branch November 15, 2019 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement direct calling convention of metric instruments
6 participants