fix: Prevent a single metric have more than 100 data points #312
Conversation
…ests to check first and second metrics flush.
pull-request-size
bot
added
the
size/M
github-actions
bot
added
the
bug
| { | ||
| if(metric.Values.Count < PowertoolsConfigurations.MaxMetrics) | ||
| metric.AddValue(value); | ||
| else |
There was a problem hiding this comment.
I don't believe we will ever be in this situation (it only accepts one double not an array) but added the throwing exception for consistency with the bellow throw
|
|
||
| if (i == 100) |
There was a problem hiding this comment.
I wanted to make sure and test that the first flush would happen and what are the values
# Conflicts: # libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #312 +/- ##
===========================================
+ Coverage 69.14% 69.19% +0.04%
===========================================
Files 79 79
Lines 3452 3464 +12
===========================================
+ Hits 2387 2397 +10
- Misses 1065 1067 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
|
||
| if (_context.GetMetrics().Count == 100) _instance.Flush(true); | ||
|
|
||
| if (_context.GetMetrics().Count > 0 && |
There was a problem hiding this comment.
use a variable var metrics = _context.GetMetrics(); instead of calling methods multiple times
| @@ -140,20 +140,75 @@ public void When100MetricsAreAdded_FlushAutomatically() | |||
| for (var i = 0; i <= 100; i++) | |||
There was a problem hiding this comment.
We can use the config value instead of hard code
for (var i = 0; i <= PowertoolsConfigurations.MaxMetrics; i++)|
|
||
| if (i == 100) |
There was a problem hiding this comment.
We can use the config value instead of hard code
if (i == PowertoolsConfigurations.MaxMetrics)
Issue number: #311
Summary
Serialize and flush metric data set when a single metric reaches 100 data points.
Throw exception when adding a metric with 100 data points.
Dimensions etc should be kept intact.
Changes
Check if single metrics has more than 100 datapoints and flush.
Add tests to check first and second metrics flush.
User experience
Checklist
Please leave checklist items unchecked if they do not apply to your change.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.