-
Notifications
You must be signed in to change notification settings - Fork 440
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
Fix 1585 - Multiple cumulative metric collections without measurement recording. #1586
Fix 1585 - Multiple cumulative metric collections without measurement recording. #1586
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1586 +/- ##
==========================================
+ Coverage 84.96% 85.06% +0.11%
==========================================
Files 156 156
Lines 4977 4978 +1
==========================================
+ Hits 4228 4234 +6
+ Misses 749 744 -5
|
Hi lalit, I just tested your changes, and I observe the when counter metrics are not sent from application side, then the metrics are displayed as 0. However, when the metrics values are sent the counter correctly accumulates from past data: |
@tauseef-ah - Thanks for testing this. How can I reproduce the scenario? Record(2) -> Record(3) -> Collect(5) -> Collect(5) -> Record(4) -> Collect(9) -> Collect(9) I tested something like the above and got the expected result with this PR. |
hi @lalitb , sorry there seems to have been an issue with us picking the wrong binaries at runtime. This solution works fine for us too. Thanks |
return true; | ||
}); | ||
last_aggr_hashmap->GetAllEnteries( | ||
[&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { |
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.
Is this jus reformat?
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.
yes, this is done by clang-format. The only changes are lines 103, and 104 below. To handle the else
scenario when there are no measurements recorded since the last collection.
Fixes #1585
Changes
In case cumulative metric collection is done without any measurement recorded in between, the same metrics should be returned. Merge was not happening properly if there is no unreported metrics. This is fixed now. Also added the tests to validate.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes