Open
Description
Hello,
When mocking time for test purpose I found a bug in the reset condition of the burst sampler. Here is simple test to reproduce the bug:
func TestBurst(t *testing.T) {
sampler := &BurstSampler{Burst: 1, Period: time.Second}
t0 := time.Now()
now := t0
mockedTime := func() time.Time {
return now
}
TimestampFunc = mockedTime
defer func() { TimestampFunc = time.Now }()
scenario := []struct {
tm time.Time
want bool
}{
{t0, true},
{t0.Add(time.Second - time.Nanosecond), false},
{t0.Add(time.Second), true},
{t0.Add(time.Second + time.Nanosecond), false},
}
for i, step := range scenario {
now = step.tm
got := sampler.Sample(NoLevel)
if got != step.want {
t.Errorf("step %d (t=%s): expect %t got %t", i, step.tm, step.want, got)
}
}
}
The fix is trivial. In the inc()
function, replace the >
comparison with >=
.
func (s *BurstSampler) inc() uint32 {
now := TimestampFunc().UnixNano()
resetAt := atomic.LoadInt64(&s.resetAt)
var c uint32
if now > resetAt { // to be replaced by: if now >= resetAt
c = 1
atomic.StoreUint32(&s.counter, c)
newResetAt := now + s.Period.Nanoseconds()
reset := atomic.CompareAndSwapInt64(&s.resetAt, resetAt, newResetAt)
if !reset {
// Lost the race with another goroutine trying to reset.
c = atomic.AddUint32(&s.counter, 1)
}
} else {
c = atomic.AddUint32(&s.counter, 1)
}
return c
}
If it's ok for you I open a PR with the fix and the unit test.
Metadata
Metadata
Assignees
Labels
No labels