Rework plugin tickers to prevent drift and spread write ticks #7390
Conversation
danielnelson
added
feat
|
|
||
| func TestAlignedTickerJitter(t *testing.T) { | ||
| interval := 10 * time.Second | ||
| jitter := 5 * time.Second |
There was a problem hiding this comment.
Go should use the same rand seed every time, so you should be able to test the specific jitter if desired
There was a problem hiding this comment.
I think I need to redo internal.RandomDuration to support a custom rand instance first, I'm getting fluctuations as-is even if I reset the Seed when the test starts. Anyway it would be nice to have the tests work if ran concurrently.
There was a problem hiding this comment.
I think the problem is that internal.RandomDuration is depending on crypto/rand, which doesn't make a lot of sense. We really don't need cryptographic randomness for setting sleep timers. might want to change this to math/rand and it should start respecting the seed.
agent/tick_test.go
Outdated
| printDist(ticker, clock) | ||
| } | ||
|
|
||
| func printDist(ticker Ticker, clock *clock.Mock) { |
There was a problem hiding this comment.
I think we check a that the avg wait time is within expected bounds, and that we got the number of ticks expected.
agent/tick.go
Outdated
| return | ||
| case <-ticker.C: | ||
| jitter := internal.RandomDuration(t.jitter) | ||
| sleep(ctx, jitter, clock) |
There was a problem hiding this comment.
doesn't handle returned error. next line pushes to chan even if context done
Fix several drift issues when using
round_intervaland use a rolling ticker for doing the full write flush. In the future I would like to rework the output to only do full flushes afterflush_intervalseconds has elapsed without a batch write. This will reduce unneeded calls to write but can be done separately.It is possible that we will want to use the "RollingTicker" for inputs as well, since it would give you the best collection spread. However, making the switch when using jitter requires changing the configuration in order to maintain the same collection pace, so I haven't switched in this pull request.
closes #5937
closes #5335
closes #4232
closes #4652
Required for all PRs: