Conversation
Aditional reverse was changing order of snapshots and the rest of code expected latest snapshots last. Simplified ratio calculation. Remove fake 0.001s for zero timed measurements. Snapshot of zero duration should not contribute to ratio calculation.
Original with extra debug
-Ensure time ordering. -Remove fake time weight for samples from same time. -Remove unnecesary reversing of snapshot. Add test for memory and cpu snapshot ordering.
github-actions
bot
added
t-tooling
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
bisect.insort does not support 'key' keyword untill python 3.10 sortedcollections are already in part of project, so no extra dependency
Remove unused compute_weighted_avg and tests.
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
One last type-inference thing, otherwise LGTM. Let's wait for Honza's approval here as well though.
Co-authored-by: Vlada Dusek <[email protected]>
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
Nice, just a couple suggestions mostly regarding tests.
There was a problem hiding this comment.
I'm not a huge fan of accessing the private interface of Snapshotter to mock the snapshots. There must be a better way 🙂 BTW, I know the original code also touched private stuff, but this PR is all about small improvements...
There was a problem hiding this comment.
IMO out of scope of this issue. Figuring out a better way of testing the Snapshotter should be part of #73.
There was a problem hiding this comment.
This file also has a lot of private member accesses.
There was a problem hiding this comment.
Existing tests were already doing it. Newly added test does not access a single private member. I would suggest to limit scope of this and refactor the older tests in different change.
| raise ValueError('Failed to compute weighted average for the sample.') from exc | ||
|
|
||
| return LoadRatioInfo(limit_ratio=threshold, actual_ratio=round(weighted_avg, 3)) | ||
| time = (current.created_at - previous.created_at).total_seconds() |
There was a problem hiding this comment.
Could we make sure that time is positive here? (non-negative? not sure)
There was a problem hiding this comment.
Explicit exception now.
| overloaded_time = 0.0 | ||
| non_overloaded_time = 0.0 |
There was a problem hiding this comment.
Very minor nit, but it may be clearer to just track overloaded_time and total_time.
There was a problem hiding this comment.
I did halfway change. I think it is equally readable If I track both overloaded_time and total_time then I have to do some extra sum on each cycle. Super insignificant, but since I already think both approaches are kind of the same, this tips the scales for my decision.
3 participants
Small fixes to snapshot handling: