-
Notifications
You must be signed in to change notification settings - Fork 821
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
Metrics updates #1700
Conversation
obecny
commented
Nov 26, 2020
- Updated aggregators
- Updated exporter collector to change the temporality
- Rename batcher to processor
- Added missing definition to api (sumobserver, updownsumobserver)
- Updated tests
Codecov Report
@@ 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
|
Is this PR ready for review? |
yes, it doesn't have any wip etc. |
There was a problem hiding this 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.
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
* 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
* 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