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: metric observer #828

Merged
merged 6 commits into from Mar 5, 2020
Merged

Conversation

@obecny
Copy link
Member

obecny commented Mar 2, 2020

Which problem is this PR solving?

Short description of the changes

  • It adds observer metric, added example and updated Prometheus to be able to see the results
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 2, 2020

Codecov Report

Merging #828 into master will increase coverage by 0.01%.
The diff coverage is 92.56%.

@@            Coverage Diff            @@
##           master    #828      +/-   ##
=========================================
+ Coverage   94.19%   94.2%   +0.01%     
=========================================
  Files         249     250       +1     
  Lines       10857   10948      +91     
  Branches     1058    1058              
=========================================
+ Hits        10227   10314      +87     
- Misses        630     634       +4
Impacted Files Coverage Δ
packages/opentelemetry-api/src/metrics/Metric.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 96.36% <100%> (+1.24%) ⬆️
...pentelemetry-exporter-prometheus/src/prometheus.ts 92.07% <100%> (+2.07%) ⬆️
packages/opentelemetry-metrics/src/export/types.ts 100% <100%> (ø) ⬆️
...ckages/opentelemetry-metrics/src/export/Batcher.ts 100% <100%> (ø) ⬆️
...emetry-exporter-prometheus/test/prometheus.test.ts 98.7% <100%> (+0.13%) ⬆️
...ckages/opentelemetry-metrics/src/ObserverResult.ts 100% <100%> (ø)
packages/opentelemetry-metrics/test/Meter.test.ts 100% <100%> (ø) ⬆️
...ges/opentelemetry-metrics/src/export/Aggregator.ts 100% <100%> (ø) ⬆️
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts 68.62% <50%> (-3.47%) ⬇️
... and 18 more
Copy link
Member

mayurkale22 left a comment

Some first pass comments, overall nice work!

examples/metrics/package.json Outdated Show resolved Hide resolved
packages/opentelemetry-api/src/metrics/Meter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-api/src/metrics/Metric.ts Outdated Show resolved Hide resolved
packages/opentelemetry-api/src/metrics/NoopMeter.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
@dyladan dyladan added this to the Beta milestone Mar 4, 2020
Copy link
Contributor

dyladan left a comment

Seems generally good to me. I'd like to get this merged soon if possible.

"description": "Example of using @opentelemetry/metrics",
"main": "index.js",
"scripts": {
"start:observer": "node metrics/observer.js"

This comment has been minimized.

Copy link
@dyladan

dyladan Mar 4, 2020

Contributor

Some reason this is start:observer instead of just start?

This comment has been minimized.

Copy link
@obecny

obecny Mar 5, 2020

Author Member

I have created metrics example to be a place for other metrics too, so next step when working on documentation and examples should be to add start:counter, start:measure

@dyladan
dyladan approved these changes Mar 5, 2020
@mayurkale22 mayurkale22 merged commit d16c691 into open-telemetry:master Mar 5, 2020
7 checks passed
7 checks passed
ci/circleci: lint_&_docs Your tests passed on CircleCI!
Details
ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node12 Your tests passed on CircleCI!
Details
ci/circleci: node12-browsers Your tests passed on CircleCI!
Details
ci/circleci: node13 Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/linuxfoundation mayurkale22 authorized
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.