Fix: preserve delta start timestamp in fast path#4069
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4069 +/- ##
==========================================
+ Coverage 82.09% 82.09% +0.01%
==========================================
Files 385 385
Lines 15989 15994 +5
==========================================
+ Hits 13124 13129 +5
Misses 2865 2865
🚀 New features to boost your workflow:
|
| has_last_delta_collection_ts_ = true; | ||
| } | ||
| // If no metrics, early return | ||
| if (delta_metrics->Size() == 0) |
There was a problem hiding this comment.
On the empty-collection path, the function early-returns when delta_metrics->Size() == 0 before updating last_delta_collection_ts_. If a cycle at t2 is empty and the next cycle at t3 has data, the emitted point will carry start_ts = t1 (the last non-empty collection) rather than t2, even though the delta map only contains activity from (t2, t3]. Successive delta windows then overlap instead of abutting, which violates the OTel data model's requirement that start_ts(n) == end_ts(n-1) and causes rate computations (value / (end - start)) to under-report. The slow path doesn't have this issue because last_reported_metrics_[collector].collection_ts is updated on every call, including empty ones.
|
Thanks @fmutlu68 for the fix. Please add a unit test that calls |
|
@ThomsonTan Thank you for your detailed feedback. I have created a unit test which contains 3 cycles which you mentioned. Could you check it ? |
| if (reported != last_reported_metrics_.end()) | ||
| { | ||
| // To save the time for next cycle | ||
| reported->second.collection_ts = collection_ts; |
There was a problem hiding this comment.
One thing I want to double check before approving: this new slow-path update may be unreachable. Once a collector has reported, we create an entry in unreported_metrics_ for it, and later we only move present->second, we never erase the map key. So present == end() may not really happen together with reported != end(). Can you confirm whether this branch is actually expected to run?
e701055 to
06e92a4
Compare
|
Could anyone have a look at here ? @ThomsonTan @lalitb It passes all checks |
Fixes #4038
Changes