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

Metrics updates #1700

Merged
merged 7 commits into from
Dec 2, 2020
Merged

Metrics updates #1700

merged 7 commits into from
Dec 2, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Nov 26, 2020

  1. Updated aggregators
  2. Updated exporter collector to change the temporality
  3. Rename batcher to processor
  4. Added missing definition to api (sumobserver, updownsumobserver)
  5. Updated tests

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #1700 (1e6ab7c) into master (b22ca63) will decrease coverage by 0.02%.
The diff coverage is 87.93%.

@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   91.41%   91.38%   -0.03%     
==========================================
  Files         165      164       -1     
  Lines        5053     5059       +6     
  Branches     1044     1048       +4     
==========================================
+ Hits         4619     4623       +4     
- Misses        434      436       +2     
Impacted Files Coverage Δ
...entelemetry-metrics/src/UpDownSumObserverMetric.ts 100.00% <ø> (ø)
...s/opentelemetry-metrics/src/ValueObserverMetric.ts 100.00% <ø> (ø)
...ges/opentelemetry-metrics/src/export/Controller.ts 79.16% <ø> (ø)
packages/opentelemetry-metrics/src/types.ts 100.00% <ø> (ø)
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts 83.09% <33.33%> (-4.60%) ⬇️
packages/opentelemetry-sdk-node/src/sdk.ts 73.91% <50.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 95.00% <90.00%> (+2.40%) ⬆️
...es/opentelemetry-metrics/src/BaseObserverMetric.ts 100.00% <100.00%> (ø)
...s/opentelemetry-metrics/src/BatchObserverMetric.ts 93.10% <100.00%> (ø)
...ackages/opentelemetry-metrics/src/CounterMetric.ts 100.00% <100.00%> (ø)
... and 6 more

@legendecas
Copy link
Member

Is this PR ready for review?

@obecny
Copy link
Member Author

obecny commented Nov 30, 2020

Is this PR ready for review?

yes, it doesn't have any wip etc.

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.

I've spent the last couple of days going over this and its size makes it a little difficult. Would have been easier for me if it was split into multiple PRs.

That said, I don't see anything too concerning. Aside from one question, I think this is probably fine.

packages/opentelemetry-metrics/src/ObserverResult.ts Outdated Show resolved Hide resolved
@obecny obecny requested a review from dyladan December 2, 2020 00:47
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

overall i'm fine with it, as @dyladan said i would have prefered to have 2 PRs (at least one dedicated to the renaming of batcher -> processor)

if (previous.timestamp[0] !== 0 || previous.timestamp[1] !== 0) {
previousValue = previous.value as LastValue;
}
if (value >= previousValue) {
Copy link
Member

Choose a reason for hiding this comment

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

should we warn when an user tries when its not the case ? I mean the user have no way to know that they are doing it wrong currently

Copy link
Member Author

Choose a reason for hiding this comment

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

I would avoid warning, as this is monotonic metric - SumObserver is monotonic, so it does what it was designed to do. Spec doesn't say anything about warning user as it is the desired behaviour. We can ask in spec if there are any plans like this, but for now I would avoid it. The spec might still change yet, but I would avoid adding this in this PR now.

@obecny obecny merged commit 781b30f into open-telemetry:master Dec 2, 2020
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* feat: renaming batcher to processor, fixing aggregators, adding missing metrics in api

* chore: fixing metrics in exporter collector, updated tests, fixing observer result

* chore: refactoring tests for rest of collectors

* chore: fixing monotonic sum observer, updated test to cover that scenario correctly
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* feat: renaming batcher to processor, fixing aggregators, adding missing metrics in api

* chore: fixing metrics in exporter collector, updated tests, fixing observer result

* chore: refactoring tests for rest of collectors

* chore: fixing monotonic sum observer, updated test to cover that scenario correctly
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

4 participants