-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add methods to clear stats #10
Conversation
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.
Thanks!
1. Handle `registered` inside the main loop. This way we can avoid atomics. 2. Don't start calculating bandwidth for newly active meters until they've been active for a round. This: 1. Ensures we only write to the snapshot from within the main loop. 2. Gives us a better estimated bandwidth usage.
1. The last update is when we last saw activity. We don't need to update it when we reset the meter. 2. Defer isn't _quite_ as fast as it could be.
@kpp I needed to so some experimentation so it was simpler to make changes directly than to leave comments in a review. Do my changes LGTY? |
Your solution looks even better than mine. LGTM |
Unfortunately your solution does not pass tests from go-libp2p-core/metrics. See https://github.com/libp2p/go-libp2p-core/pull/71/files and play with it.
Whereas mine works fine. Don't forget about |
The issue is that this version starts tracking bandwidth one second later. I'll update those tests. |
The previous attempt actually make the problem worse: 1. It didn't update the "last update" time. 2. It updated the accumulator but then didn't set the total. That meant that the _next_ update would count the bandwidth from two time periods. This change also reverts the changes to the test (the test was right, the code was wrong).
Actually, the issue is that I implemented this wrong, misdiagnosed why the test was failing, then broke the tests 🤦♂️. Fixed. |
No description provided.