Don't change Histogram values during Snapshot (#142)

* Calling Snapshot changes recorded Histograms Update test to call `scope.Snapshot()` repeatedly. Call `(*counter).snapshot` instead of `(*counter).value` when compiling a snapshot of histograms, so as not to alter the value of the histogram during the operation.
uber-go · Jun 3, 2020 · 164eb6a · 164eb6a
1 parent dbf6b10
commit 164eb6a
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 40 deletions.
diff --git a/scope_test.go b/scope_test.go
@@ -21,6 +21,7 @@
 package tally
 
 import (
+	"fmt"
 	"math"
 	"sync"
 	"sync/atomic"
@@ -709,44 +710,49 @@ func TestSnapshot(t *testing.T) {
 	s.Histogram("buzz", DurationBuckets{time.Second * 2, time.Second * 4}).RecordDuration(time.Second)
 	child.Counter("boop").Inc(1)
 
-	snap := s.Snapshot()
-	counters, gauges, timers, histograms :=
-		snap.Counters(), snap.Gauges(), snap.Timers(), snap.Histograms()
-
-	assert.EqualValues(t, 1, counters["foo.beep+env=test"].Value())
-	assert.EqualValues(t, commonTags, counters["foo.beep+env=test"].Tags())
-
-	assert.EqualValues(t, 2, gauges["foo.bzzt+env=test"].Value())
-	assert.EqualValues(t, commonTags, gauges["foo.bzzt+env=test"].Tags())
-
-	assert.EqualValues(t, []time.Duration{
-		1 * time.Second,
-		2 * time.Second,
-	}, timers["foo.brrr+env=test"].Values())
-	assert.EqualValues(t, commonTags, timers["foo.brrr+env=test"].Tags())
-
-	assert.EqualValues(t, map[float64]int64{
-		0:               0,
-		2:               1,
-		4:               0,
-		math.MaxFloat64: 1,
-	}, histograms["foo.fizz+env=test"].Values())
-	assert.EqualValues(t, map[time.Duration]int64(nil), histograms["foo.fizz+env=test"].Durations())
-	assert.EqualValues(t, commonTags, histograms["foo.fizz+env=test"].Tags())
-
-	assert.EqualValues(t, map[float64]int64(nil), histograms["foo.buzz+env=test"].Values())
-	assert.EqualValues(t, map[time.Duration]int64{
-		time.Second * 2: 1,
-		time.Second * 4: 0,
-		math.MaxInt64:   0,
-	}, histograms["foo.buzz+env=test"].Durations())
-	assert.EqualValues(t, commonTags, histograms["foo.buzz+env=test"].Tags())
-
-	assert.EqualValues(t, 1, counters["foo.boop+env=test,service=test"].Value())
-	assert.EqualValues(t, map[string]string{
-		"env":     "test",
-		"service": "test",
-	}, counters["foo.boop+env=test,service=test"].Tags())
+	// Should be able to call Snapshot any number of times and get same result.
+	for i := 0; i < 3; i++ {
+		t.Run(fmt.Sprintf("attempt %d", i), func(t *testing.T) {
+			snap := s.Snapshot()
+			counters, gauges, timers, histograms :=
+				snap.Counters(), snap.Gauges(), snap.Timers(), snap.Histograms()
+
+			assert.EqualValues(t, 1, counters["foo.beep+env=test"].Value())
+			assert.EqualValues(t, commonTags, counters["foo.beep+env=test"].Tags())
+
+			assert.EqualValues(t, 2, gauges["foo.bzzt+env=test"].Value())
+			assert.EqualValues(t, commonTags, gauges["foo.bzzt+env=test"].Tags())
+
+			assert.EqualValues(t, []time.Duration{
+				1 * time.Second,
+				2 * time.Second,
+			}, timers["foo.brrr+env=test"].Values())
+			assert.EqualValues(t, commonTags, timers["foo.brrr+env=test"].Tags())
+
+			assert.EqualValues(t, map[float64]int64{
+				0:               0,
+				2:               1,
+				4:               0,
+				math.MaxFloat64: 1,
+			}, histograms["foo.fizz+env=test"].Values())
+			assert.EqualValues(t, map[time.Duration]int64(nil), histograms["foo.fizz+env=test"].Durations())
+			assert.EqualValues(t, commonTags, histograms["foo.fizz+env=test"].Tags())
+
+			assert.EqualValues(t, map[float64]int64(nil), histograms["foo.buzz+env=test"].Values())
+			assert.EqualValues(t, map[time.Duration]int64{
+				time.Second * 2: 1,
+				time.Second * 4: 0,
+				math.MaxInt64:   0,
+			}, histograms["foo.buzz+env=test"].Durations())
+			assert.EqualValues(t, commonTags, histograms["foo.buzz+env=test"].Tags())
+
+			assert.EqualValues(t, 1, counters["foo.boop+env=test,service=test"].Value())
+			assert.EqualValues(t, map[string]string{
+				"env":     "test",
+				"service": "test",
+			}, counters["foo.boop+env=test,service=test"].Tags())
+		})
+	}
 }
 
 func TestCapabilities(t *testing.T) {

diff --git a/stats.go b/stats.go
@@ -382,7 +382,7 @@ func (h *histogram) snapshotValues() map[float64]int64 {
 
 	vals := make(map[float64]int64, len(h.buckets))
 	for i := range h.buckets {
-		vals[h.buckets[i].valueUpperBound] = h.buckets[i].samples.value()
+		vals[h.buckets[i].valueUpperBound] = h.buckets[i].samples.snapshot()
 	}
 
 	return vals
@@ -395,7 +395,7 @@ func (h *histogram) snapshotDurations() map[time.Duration]int64 {
 
 	durations := make(map[time.Duration]int64, len(h.buckets))
 	for i := range h.buckets {
-		durations[h.buckets[i].durationUpperBound] = h.buckets[i].samples.value()
+		durations[h.buckets[i].durationUpperBound] = h.buckets[i].samples.snapshot()
 	}
 
 	return durations