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(metrics): add registerMetric and getMetrics #437

Merged
merged 15 commits into from
Nov 1, 2019

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Oct 17, 2019

Which problem is this PR solving?

  • Updates Implement Metrics SDK #402

  • Continuation of Metrics SDK feat: implement metrics sdk #415.

  • Added methods (registerMetric and getMetrics) to collect recorded values in the format of ReadableMetric, which can be used by metric exporters to transform and send to the backend.

  • Pending stuff for next PRs:

    • Add option to register metric exporter.
    • Send data to registered exporter via meter.getMetrics()
    • Implement MeasureHandle.

TODO:

  • Decide which MetricDescriptorType to assign in case of Gauge (two options are MetricDescriptorType.GAUGE_INT64 and MetricDescriptorType.GAUGE_DOUBLE) and in case of counter (two options are MetricDescriptorType.COUNTER_INT64 and MetricDescriptorType.COUNTER_DOUBLE)

(update 1: 10/30)
Implemented option 3 as per SIG discussion: https://docs.google.com/document/d/1g0O7TZ4KH82iwbJmEdBh3qWONnHbap3ajjD2_HWA0mY/edit

/cc @danielkhan @bg451

@mayurkale22 mayurkale22 changed the title feat(metrics): add registerMetric and getMetric feat(metrics): add registerMetric and getMetrics Oct 17, 2019
@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #437 into master will increase coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   94.83%   95.09%   +0.25%     
==========================================
  Files         131      132       +1     
  Lines        6528     6851     +323     
  Branches      560      572      +12     
==========================================
+ Hits         6191     6515     +324     
+ Misses        337      336       -1
Impacted Files Coverage Δ
packages/opentelemetry-metrics/src/export/types.ts 100% <ø> (ø)
packages/opentelemetry-metrics/src/Meter.ts 97.43% <100%> (+1.43%) ⬆️
packages/opentelemetry-metrics/src/Handle.ts 95.23% <100%> (+1.29%) ⬆️
packages/opentelemetry-metrics/test/Meter.test.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-metrics/src/types.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 88.46% <100%> (+6.1%) ⬆️
...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%> (ø) ⬆️
packages/opentelemetry-tracing/test/Span.test.ts 100% <0%> (ø) ⬆️
... and 6 more

@mayurkale22
Copy link
Member Author

@rghetia @c24t as discussed, could you please review this PR in-terms of structure and semantics?

@mayurkale22 mayurkale22 force-pushed the registerMetric branch 2 times, most recently from 9bef4bf to 782162e Compare October 23, 2019 18:17
Copy link
Contributor

@danielkhan danielkhan left a comment

Choose a reason for hiding this comment

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

LGTM so far and I think we should move ahead to iterate towards end-to-end implementation

@mayurkale22
Copy link
Member Author

LGTM so far and I think we should move ahead to iterate towards end-to-end implementation

What do you think about this TODO -> https://github.com/open-telemetry/opentelemetry-js/pull/437/files#r336268261

@dyladan
Copy link
Member

dyladan commented Oct 31, 2019

It looks like getTimeSeries, which is implemented on all Handle classes, is missing from the Handle interfaces in types. Is this intentional?

@mayurkale22
Copy link
Member Author

It looks like getTimeSeries, which is implemented on all Handle classes, is missing from the Handle interfaces in types. Is this intentional?

Yes, this is intentional. getTimeSeries is our SDK related functionality, we do not enforce these things in the public types/API. More info is available here.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM. Left very minor style suggestions.

packages/opentelemetry-metrics/src/Handle.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/Handle.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/Meter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/Meter.ts Outdated Show resolved Hide resolved
@mayurkale22 mayurkale22 merged commit 173cab2 into open-telemetry:master Nov 1, 2019
@mayurkale22 mayurkale22 deleted the registerMetric branch November 1, 2019 19:24
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants