-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix(ewma): reduce the chances of fake bandwidth spikes #8
Conversation
This makes me uneasy because it introduces a timing condition that summons another class of bugs. How about we break down the act of recording into two methods? On Meter, a Prerecord() method that returns a pointerless Recording type that is just a time.Time of when the recording started. On Recording (value receiver), a Record(float64) performs the actual recording. The whole thing can incur in zero allocs and overcome the ugliness of asking for time with every recording. |
This makes me uneasy because it introduces a timing condition that summons another class of bugs.
We already have a timing condition, this just makes it less of an issue. The problem is:
1. Receive a tick.
2. Read the tick.
3. Pause due to the scheduler.
4. Receive a tick on the channel (but don't read it).
5. Handle the tick first tick.
6. Read and handle the second tick.
The second tick will be _really_ short.
Alternatively, we could switch from a ticker to a timer, resetting it after every tick. But it doesn't _really_ matter much either way.
How about we break down the act of recording into two methods?
On Meter, a Prerecord() method that returns a pointerless Recording type that is just a time.Time of when the recording started.
On Recording (value receiver), a Record(float64) performs the actual recording.
The whole thing can incur in zero allocs and overcome the ugliness of asking for time with every recording.
Note: I'm not concerned about uglyness, I'm concerned about the _cost_ of getting the time. Getting the time is slower (but not by much) than allocating on Linux and requires a syscall on some systems.
However, we can probably record the last tick time in the "sweeper" loop and use that.
|
Actually, I have no idea how to do this without introducing additional performance penalties on write. The issue is that meters can be updated by _multiple_ writers at the same time.
|
I've pushed a fix for non-monotonic clocks. |
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.
CI seems very unhappy with this change.
@@ -30,6 +30,9 @@ func SetClock(c clock.Clock) { | |||
cl = c | |||
} | |||
|
|||
// We tick every second. | |||
var ewmaRate = time.Second |
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.
const?
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.
CI is always unhappy.
ignore updates that are less than 100ms. Otherwise, we could attribute a large amount of bandwidth to a very short period of time and get a _huge_ spike.
Ignore updates that are less than 100ms. Otherwise, we could attribute a large amount of bandwidth to a very short period of time and get a huge spike.
The next step will be to split writes up into chunks before we get to the meter.
An alternative is to record the amount of data sent and a time. However, the
current system was chosen precisely to avoid repeatedly asking for the time.