Fix metrics upload race condition - #638
Conversation
|
|
||
| for (auto& m : uploads) { | ||
| m->accumulator().flush(end); | ||
| } | ||
|
|
||
| upload_checkpoint_ = end; |
There was a problem hiding this comment.
They should be outside of the for loop
| std::vector<std::shared_ptr<Metric>> uploads; | ||
|
|
||
| TimePoint begin = upload_checkpoint_; | ||
| TimePoint end = Clock::now() - std::chrono::seconds(1); |
There was a problem hiding this comment.
Let's start adding comments for the changes. It will help readers
| @@ -136,20 +152,20 @@ class AccumulatorList { | |||
| } | |||
There was a problem hiding this comment.
buckets_between is not used so it can be removed
| return std::pair<TimePoint, TimePoint>(end - BucketSize(buckets), end); | ||
| } | ||
|
|
||
| static inline TimePoint current_time() { |
There was a problem hiding this comment.
Maybe combine buckets_since_epoch with current_time. Please add some comments for the newly added comments
| static inline std::pair<TimePoint, TimePoint> range(size_t buckets) { | ||
| return range(buckets, current_time()); | ||
| } | ||
|
|
||
| static inline std::pair<TimePoint, TimePoint> range(size_t buckets, TimePoint end) { | ||
| return std::pair<TimePoint, TimePoint>(end - BucketSize(buckets), end); | ||
| } |
There was a problem hiding this comment.
These methods shouldn't be needed once we remove the original usage of BucketSize based timepoint calculation. Please add a TODO here to clean these up.
| } | ||
| } | ||
|
|
||
| void flush(TimePoint checkpoint) { |
There was a problem hiding this comment.
Can we have a test for this?
…load fails or severely stalls and add flush accumulator unit test
|
|
||
| std::shared_ptr<aws::utils::ScheduledCallback> scheduled_upload_; | ||
| TimePoint upload_checkpoint_; | ||
| TimePoint next_run_; |
There was a problem hiding this comment.
nit : better name for this variable
2 participants
Issue #, if available: #188
Description of changes: Fixes a race condition between the metrics manager and the metrics accumulator that could cause missed data points. If these were the only data points in the last 60 seconds, this could cause CloudWatch to fail the PutMetricData request as it would try to upload the default values in C++ Boost accumulators.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.