Skip to content

Commit

Permalink
correctly set the initial total
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
Stebalien committed Nov 1, 2019
1 parent ce5ebda commit 2fc48f1
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 15 deletions.
18 changes: 6 additions & 12 deletions flow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,6 @@ func TestUnregister(t *testing.T) {
m := new(Meter)
go func() {
defer wg.Done()
m.Mark(1)
time.Sleep(time.Second)

ticker := time.NewTicker(100 * time.Millisecond)
defer ticker.Stop()
for i := 0; i < 40; i++ {
Expand All @@ -120,17 +117,14 @@ func TestUnregister(t *testing.T) {

time.Sleep(62 * time.Second)

m.Mark(2)
time.Sleep(time.Second)

for i := 0; i < 40; i++ {
m.Mark(2)
<-ticker.C
}
}()
go func() {
defer wg.Done()
time.Sleep(40*100*time.Millisecond + time.Second)
time.Sleep(40 * 100 * time.Millisecond)

actual := m.Snapshot()
if !approxEq(actual.Rate, 10, 1) {
Expand All @@ -143,19 +137,19 @@ func TestUnregister(t *testing.T) {
}

actual = m.Snapshot()
if actual.Total != 41 {
t.Errorf("expected total 41, got %d", actual.Total)
if actual.Total != 40 {
t.Errorf("expected total 4000, got %d", actual.Total)
}
time.Sleep(3*time.Second + 40*100*time.Millisecond)
time.Sleep(2*time.Second + 40*100*time.Millisecond)

actual = m.Snapshot()
if !approxEq(actual.Rate, 20, 4) {
t.Errorf("expected rate 20 (±4), got %f", actual.Rate)
}
time.Sleep(2 * time.Second)
actual = m.Snapshot()
if actual.Total != 123 {
t.Errorf("expected total 123, got %d", actual.Total)
if actual.Total != 120 {
t.Errorf("expected total 120, got %d", actual.Total)
}
if atomic.LoadUint64(&m.accumulator) == 0 {
t.Error("expected meter to be active")
Expand Down
2 changes: 1 addition & 1 deletion registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func TestRegistry(t *testing.T) {
m1.Mark(10)
m2.Mark(30)

time.Sleep(2 * time.Second)
time.Sleep(2*time.Second + time.Millisecond)

if total := r.Get("first").Snapshot().Total; total != 10 {
t.Errorf("expected first total to be 10, got %d", total)
Expand Down
8 changes: 6 additions & 2 deletions sweeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,15 @@ func (sw *sweeper) update() {
sw.meters[i] = sw.meters[newLen]
}

// Re-add the total to all the newly active accumulators.
// Re-add the total to all the newly active accumulators and set the snapshot to the total.
// 1. We don't do this on register to avoid having to take the snapshot lock.
// 2. We skip calculating the bandwidth for this round so we get an _accurate_ bandwidth calculation.
for _, m := range sw.meters[sw.activeMeters:] {
atomic.AddUint64(&m.accumulator, m.snapshot.Total)
total := atomic.AddUint64(&m.accumulator, m.snapshot.Total)
if total > m.snapshot.Total {
m.snapshot.LastUpdate = now
}
m.snapshot.Total = total
}

// trim the meter list
Expand Down

0 comments on commit 2fc48f1

Please sign in to comment.