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

Host Metrics #227

Merged
merged 18 commits into from Oct 28, 2020
Merged

Host Metrics #227

merged 18 commits into from Oct 28, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Oct 21, 2020

Runtime Metrics are added on top of Host Metrics so PR is here to be able to see the differences easily:
obecny#1

@obecny obecny requested a review from a team as a code owner October 21, 2020 19:02
@obecny obecny self-assigned this Oct 21, 2020
@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #227 into master will decrease coverage by 0.01%.
The diff coverage is 94.81%.

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage   95.23%   95.21%   -0.02%     
==========================================
  Files          92      100       +8     
  Lines        4767     4979     +212     
  Branches      492      517      +25     
==========================================
+ Hits         4540     4741     +201     
- Misses        227      238      +11     
Impacted Files Coverage Δ
packages/opentelemetry-host-metrics/.eslintrc.js 0.00% <0.00%> (ø)
...ackages/opentelemetry-host-metrics/src/stats/si.ts 88.88% <88.88%> (ø)
packages/opentelemetry-host-metrics/src/metric.ts 94.80% <94.80%> (ø)
...ges/opentelemetry-host-metrics/test/metric.test.ts 95.50% <95.50%> (ø)
packages/opentelemetry-host-metrics/src/enum.ts 100.00% <100.00%> (ø)
...ges/opentelemetry-host-metrics/src/stats/common.ts 100.00% <100.00%> (ø)
packages/opentelemetry-host-metrics/src/util.ts 100.00% <100.00%> (ø)
packages/opentelemetry-host-metrics/src/version.ts 100.00% <100.00%> (ø)
... and 6 more

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.

First pass nothing looks bad. The structure of the RCAMetrics class is a little confusing to me, but after a little reading it seems fine.

packages/opentelemetry-rca-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-rca-metrics/src/metric.ts Outdated Show resolved Hide resolved
@obecny obecny requested a review from dyladan October 28, 2020 00:15
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.

Seems fine to me. The implementation of metric.ts seems more complicated than I would expect and I would be worried implementing something this complex on top of our current metrics implementation because the sdk is so incomplete and the spec is changing so much. But in general nothing seems wrong.

packages/opentelemetry-rca-metrics/README.md Outdated Show resolved Hide resolved
@obecny obecny changed the title RCA Metrics Operation Metrics Oct 28, 2020
@obecny obecny changed the title Operation Metrics Host Metrics Oct 28, 2020
@obecny obecny requested a review from mwear October 28, 2020 17:49
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM. One question about the split between host and runtime metrics. Will it be possible to collect runtime metrics without collecting host metrics? I think this is use case that we'd like to support. The reason is that the OTel collector can generate host metrics, so some people will only want to generate runtime metrics from their applications.

@@ -0,0 +1,61 @@
#OpenTelemetry RCA Metrics for Node.js
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#OpenTelemetry RCA Metrics for Node.js
#OpenTelemetry Host Metrics for Node.js

Copy link
Member Author

Choose a reason for hiding this comment

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

yes the runtime metrics do not generate host metrics.

@obecny obecny added the enhancement New feature or request label Oct 28, 2020
@obecny obecny merged commit 088a4d5 into open-telemetry:master Oct 28, 2020
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.

None yet

3 participants