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: adding new metric: up down sum observer #1272

Merged
merged 17 commits into from
Jul 10, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Jul 1, 2020

Which problem is this PR solving?

Short description of the changes

  • It adds new metric UpDownSumObserver

@obecny obecny self-assigned this Jul 1, 2020
@obecny obecny added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 1, 2020
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #1272 into master will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1272      +/-   ##
==========================================
+ Coverage   93.03%   93.24%   +0.20%     
==========================================
  Files         120      137      +17     
  Lines        2931     3878     +947     
  Branches      590      790     +200     
==========================================
+ Hits         2727     3616     +889     
- Misses        204      262      +58     
Impacted Files Coverage Δ
packages/opentelemetry-api/src/metrics/Metric.ts 100.00% <ø> (ø)
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts 67.30% <100.00%> (+0.64%) ⬆️
...es/opentelemetry-metrics/src/BaseObserverMetric.ts 100.00% <100.00%> (ø)
packages/opentelemetry-metrics/src/Meter.ts 95.45% <100.00%> (+3.14%) ⬆️
...entelemetry-metrics/src/UpDownSumObserverMetric.ts 100.00% <100.00%> (ø)
...s/opentelemetry-metrics/src/ValueObserverMetric.ts 100.00% <100.00%> (ø)
...ry-plugin-xml-http-request/src/enums/EventNames.ts 100.00% <0.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 80.68% <0.00%> (ø)
...kages/opentelemetry-web/src/StackContextManager.ts 97.05% <0.00%> (ø)
packages/opentelemetry-plugin-fetch/src/fetch.ts 96.52% <0.00%> (ø)
... and 14 more

packages/opentelemetry-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/Meter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/Meter.ts Show resolved Hide resolved
@mayurkale22
Copy link
Member

What is the plan to export this new Metric type to Prometheus exporter?

@obecny
Copy link
Member Author

obecny commented Jul 2, 2020

What is the plan to export this new Metric type to Prometheus exporter?

This is currently already exported as point has value which is number and this number will be exported

@obecny obecny requested a review from mayurkale22 July 2, 2020 18:35
packages/opentelemetry-metrics/src/Meter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/BaseObserverMetric.ts Outdated Show resolved Hide resolved
@obecny obecny requested a review from dyladan July 2, 2020 20:10
Copy link
Contributor

@lykkin lykkin left a comment

Choose a reason for hiding this comment

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

Code looks good to me, a few comments left in the docs.

packages/opentelemetry-metrics/README.md Show resolved Hide resolved
packages/opentelemetry-metrics/README.md Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/README.md Show resolved Hide resolved
Copy link
Contributor

@lykkin lykkin left a comment

Choose a reason for hiding this comment

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

🎉 🦝

Copy link
Contributor

@lykkin lykkin left a comment

Choose a reason for hiding this comment

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

Couple more things I noticed.

packages/opentelemetry-metrics/src/Meter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/test/Meter.test.ts Outdated Show resolved Hide resolved
@obecny obecny requested a review from mayurkale22 July 3, 2020 12:47
@@ -30,7 +30,7 @@ const NOOP_CALLBACK = () => {};
*/
export abstract class BaseObserverMetric extends Metric<BoundObserver>
implements api.BaseObserver {
protected _callback: (observerResult: api.ObserverResult) => void;
protected _callback: (observerResult: api.ObserverResult) => unknown;
Copy link
Member

Choose a reason for hiding this comment

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

not sure why you changed the void to a unknown ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because this might or not return something

@obecny obecny requested a review from vmarchaud July 7, 2020 15:31
@mayurkale22
Copy link
Member

@open-telemetry/javascript-approvers ^^

});
}

// sync callback in case you don't need to wait for value
Copy link
Member

Choose a reason for hiding this comment

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

👍
Great to provide both examples.

@mayurkale22 mayurkale22 merged commit 297e837 into open-telemetry:master Jul 10, 2020
@obecny obecny deleted the up-down-sum-observer branch October 1, 2020 14:19
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement UpDownSumObserver
6 participants