metrics: align percentiles handling in resetting timer with rest of t…

…he codebase
ethereum · Aug 31, 2023 · 0abadec · 0abadec
1 parent fb8e300
commit 0abadec
Show file tree

Hide file tree

Showing 7 changed files with 95 additions and 122 deletions.
diff --git a/metrics/exp/exp.go b/metrics/exp/exp.go
@@ -176,13 +176,13 @@ func (exp *exp) publishTimer(name string, metric metrics.Timer) {
 
 func (exp *exp) publishResettingTimer(name string, metric metrics.ResettingTimer) {
 	t := metric.Snapshot()
-	ps := t.Percentiles([]float64{50, 75, 95, 99})
+	ps := t.Percentiles([]float64{0.50, 0.75, 0.95, 0.99})
 	exp.getInt(name + ".count").Set(int64(t.Count()))
 	exp.getFloat(name + ".mean").Set(t.Mean())
-	exp.getInt(name + ".50-percentile").Set(ps[0])
-	exp.getInt(name + ".75-percentile").Set(ps[1])
-	exp.getInt(name + ".95-percentile").Set(ps[2])
-	exp.getInt(name + ".99-percentile").Set(ps[3])
+	exp.getFloat(name + ".50-percentile").Set(ps[0])
+	exp.getFloat(name + ".75-percentile").Set(ps[1])
+	exp.getFloat(name + ".95-percentile").Set(ps[2])
+	exp.getFloat(name + ".99-percentile").Set(ps[3])
 }
 
 func (exp *exp) syncToExpvar() {

diff --git a/metrics/influxdb/influxdb.go b/metrics/influxdb/influxdb.go
@@ -102,7 +102,7 @@ func readMeter(namespace, name string, i interface{}) (string, map[string]interf
 		if t.Count() == 0 {
 			break
 		}
-		ps := t.Percentiles([]float64{50, 95, 99})
+		ps := t.Percentiles([]float64{0.50, 0.95, 0.99})
 		measurement := fmt.Sprintf("%s%s.span", namespace, name)
 		fields := map[string]interface{}{
 			"count": t.Count(),

diff --git a/metrics/prometheus/collector.go b/metrics/prometheus/collector.go
@@ -125,7 +125,7 @@ func (c *collector) addResettingTimer(name string, m metrics.ResettingTimerSnaps
 	if m.Count() <= 0 {
 		return
 	}
-	ps := m.Percentiles([]float64{50, 95, 99})
+	ps := m.Percentiles([]float64{0.50, 0.95, 0.99})
 	c.writeSummaryCounter(name, m.Count())
 	c.buff.WriteString(fmt.Sprintf(typeSummaryTpl, mutateKey(name)))
 	c.writeSummaryPercentile(name, "0.50", ps[0])

diff --git a/metrics/resetting_timer.go b/metrics/resetting_timer.go
@@ -1,11 +1,8 @@
 package metrics
 
 import (
-	"math"
 	"sync"
 	"time"
-
-	"golang.org/x/exp/slices"
 )
 
 // Initial slice capacity for the values stored in a ResettingTimer
@@ -16,7 +13,7 @@ type ResettingTimerSnapshot interface {
 	Mean() float64
 	Max() int64
 	Min() int64
-	Percentiles([]float64) []int64
+	Percentiles([]float64) []float64
 }
 
 // ResettingTimer is used for storing aggregated values for timers, which are reset on every flush interval.
@@ -63,7 +60,7 @@ func (NilResettingTimer) Values() []int64                    { return nil }
 func (n NilResettingTimer) Snapshot() ResettingTimerSnapshot { return n }
 func (NilResettingTimer) Time(f func())                      { f() }
 func (NilResettingTimer) Update(time.Duration)               {}
-func (NilResettingTimer) Percentiles([]float64) []int64      { return nil }
+func (NilResettingTimer) Percentiles([]float64) []float64    { return nil }
 func (NilResettingTimer) Mean() float64                      { return 0.0 }
 func (NilResettingTimer) Max() int64                         { return 0 }
 func (NilResettingTimer) Min() int64                         { return 0 }
@@ -116,7 +113,7 @@ type resettingTimerSnapshot struct {
 	mean                float64
 	max                 int64
 	min                 int64
-	thresholdBoundaries []int64
+	thresholdBoundaries []float64
 	calculated          bool
 }
 
@@ -127,7 +124,7 @@ func (t *resettingTimerSnapshot) Count() int {
 
 // Percentiles returns the boundaries for the input percentiles.
 // note: this method is not thread safe
-func (t *resettingTimerSnapshot) Percentiles(percentiles []float64) []int64 {
+func (t *resettingTimerSnapshot) Percentiles(percentiles []float64) []float64 {
 	t.calc(percentiles)
 	return t.thresholdBoundaries
 }
@@ -148,7 +145,6 @@ func (t *resettingTimerSnapshot) Max() int64 {
 	if !t.calculated {
 		t.calc(nil)
 	}
-
 	return t.max
 }
 
@@ -158,52 +154,20 @@ func (t *resettingTimerSnapshot) Min() int64 {
 	if !t.calculated {
 		t.calc(nil)
 	}
-
 	return t.min
 }
 
 func (t *resettingTimerSnapshot) calc(percentiles []float64) {
-	slices.Sort(t.values)
-	count := len(t.values)
-	if count == 0 {
-		t.thresholdBoundaries = make([]int64, len(percentiles))
-		t.mean = 0
-		t.calculated = true
+	scores := CalculatePercentiles(t.values, percentiles)
+	t.thresholdBoundaries = scores
+	if len(t.values) == 0 {
 		return
 	}
 	t.min = t.values[0]
-	t.max = t.values[count-1]
-
-	cumulativeValues := make([]int64, count)
-	cumulativeValues[0] = t.min
-	for i := 1; i < count; i++ {
-		cumulativeValues[i] = t.values[i] + cumulativeValues[i-1]
+	t.max = t.values[len(t.values)-1]
+	var sum int64
+	for _, v := range t.values {
+		sum += v
 	}
-
-	t.thresholdBoundaries = make([]int64, len(percentiles))
-
-	thresholdBoundary := t.max
-
-	for i, pct := range percentiles {
-		if count > 1 {
-			var abs float64
-			if pct >= 0 {
-				abs = pct
-			} else {
-				abs = 100 + pct
-			}
-			// poor man's math.Round(x):
-			// math.Floor(x + 0.5)
-			indexOfPerc := int(math.Floor(((abs / 100.0) * float64(count)) + 0.5))
-			if pct >= 0 && indexOfPerc > 0 {
-				indexOfPerc -= 1 // index offset=0
-			}
-			thresholdBoundary = t.values[indexOfPerc]
-		}
-
-		t.thresholdBoundaries[i] = thresholdBoundary
-	}
-
-	sum := cumulativeValues[count-1]
-	t.mean = float64(sum) / float64(count)
+	t.mean = float64(sum) / float64(len(t.values))
 }
diff --git a/metrics/resetting_timer_test.go b/metrics/resetting_timer_test.go
@@ -10,9 +10,9 @@ func TestResettingTimer(t *testing.T) {
 		values   []int64
 		start    int
 		end      int
-		wantP50  int64
-		wantP95  int64
-		wantP99  int64
+		wantP50  float64
+		wantP95  float64
+		wantP99  float64
 		wantMean float64
 		wantMin  int64
 		wantMax  int64
@@ -21,14 +21,14 @@ func TestResettingTimer(t *testing.T) {
 			values:  []int64{},
 			start:   1,
 			end:     11,
-			wantP50: 5, wantP95: 10, wantP99: 10,
+			wantP50: 5.5, wantP95: 10, wantP99: 10,
 			wantMin: 1, wantMax: 10, wantMean: 5.5,
 		},
 		{
 			values:  []int64{},
 			start:   1,
 			end:     101,
-			wantP50: 50, wantP95: 95, wantP99: 99,
+			wantP50: 50.5, wantP95: 95.94999999999999, wantP99: 99.99,
 			wantMin: 1, wantMax: 100, wantMean: 50.5,
 		},
 		{
@@ -56,11 +56,11 @@ func TestResettingTimer(t *testing.T) {
 			values:  []int64{1, 10},
 			start:   0,
 			end:     0,
-			wantP50: 1, wantP95: 10, wantP99: 10,
+			wantP50: 5.5, wantP95: 10, wantP99: 10,
 			wantMin: 1, wantMax: 10, wantMean: 5.5,
 		},
 	}
-	for ind, tt := range tests {
+	for i, tt := range tests {
 		timer := NewResettingTimer()
 
 		for i := tt.start; i < tt.end; i++ {
@@ -70,33 +70,27 @@ func TestResettingTimer(t *testing.T) {
 		for _, v := range tt.values {
 			timer.Update(time.Duration(v))
 		}
-
 		snap := timer.Snapshot()
 
-		ps := snap.Percentiles([]float64{50, 95, 99})
+		ps := snap.Percentiles([]float64{0.50, 0.95, 0.99})
 
-		if tt.wantMin != snap.Min() {
-			t.Fatalf("%d: min: got %d, want %d", ind, snap.Min(), tt.wantMin)
+		if have, want := snap.Min(), tt.wantMin; have != want {
+			t.Fatalf("%d: min: have %d, want %d", i, have, want)
 		}
-
-		if tt.wantMax != snap.Max() {
-			t.Fatalf("%d: max: got %d, want %d", ind, snap.Max(), tt.wantMax)
+		if have, want := snap.Max(), tt.wantMax; have != want {
+			t.Fatalf("%d: max: have %d, want %d", i, have, want)
 		}
-
-		if tt.wantMean != snap.Mean() {
-			t.Fatalf("%d: mean: got %.2f, want %.2f", ind, snap.Mean(), tt.wantMean)
+		if have, want := snap.Mean(), tt.wantMean; have != want {
+			t.Fatalf("%d: mean: have %v, want %v", i, have, want)
 		}
-
-		if tt.wantP50 != ps[0] {
-			t.Fatalf("%d: p50: got %d, want %d", ind, ps[0], tt.wantP50)
+		if have, want := ps[0], tt.wantP50; have != want {
+			t.Errorf("%d: p50: have %v, want %v", i, have, want)
 		}
-
-		if tt.wantP95 != ps[1] {
-			t.Fatalf("%d: p95: got %d, want %d", ind, ps[1], tt.wantP95)
+		if have, want := ps[1], tt.wantP95; have != want {
+			t.Errorf("%d: p95: have %v, want %v", i, have, want)
 		}
-
-		if tt.wantP99 != ps[2] {
-			t.Fatalf("%d: p99: got %d, want %d", ind, ps[2], tt.wantP99)
+		if have, want := ps[2], tt.wantP99; have != want {
+			t.Errorf("%d: p99: have %v, want %v", i, have, want)
 		}
 	}
 }
@@ -106,11 +100,11 @@ func TestResettingTimerWithFivePercentiles(t *testing.T) {
 		values   []int64
 		start    int
 		end      int
-		wantP05  int64
-		wantP20  int64
-		wantP50  int64
-		wantP95  int64
-		wantP99  int64
+		wantP05  float64
+		wantP20  float64
+		wantP50  float64
+		wantP95  float64
+		wantP99  float64
 		wantMean float64
 		wantMin  int64
 		wantMax  int64
@@ -119,14 +113,14 @@ func TestResettingTimerWithFivePercentiles(t *testing.T) {
 			values:  []int64{},
 			start:   1,
 			end:     11,
-			wantP05: 1, wantP20: 2, wantP50: 5, wantP95: 10, wantP99: 10,
+			wantP05: 1, wantP20: 2.2, wantP50: 5.5, wantP95: 10, wantP99: 10,
 			wantMin: 1, wantMax: 10, wantMean: 5.5,
 		},
 		{
 			values:  []int64{},
 			start:   1,
 			end:     101,
-			wantP05: 5, wantP20: 20, wantP50: 50, wantP95: 95, wantP99: 99,
+			wantP05: 5.050000000000001, wantP20: 20.200000000000003, wantP50: 50.5, wantP95: 95.94999999999999, wantP99: 99.99,
 			wantMin: 1, wantMax: 100, wantMean: 50.5,
 		},
 		{
@@ -154,7 +148,7 @@ func TestResettingTimerWithFivePercentiles(t *testing.T) {
 			values:  []int64{1, 10},
 			start:   0,
 			end:     0,
-			wantP05: 1, wantP20: 1, wantP50: 1, wantP95: 10, wantP99: 10,
+			wantP05: 1, wantP20: 1, wantP50: 5.5, wantP95: 10, wantP99: 10,
 			wantMin: 1, wantMax: 10, wantMean: 5.5,
 		},
 	}
@@ -171,38 +165,33 @@ func TestResettingTimerWithFivePercentiles(t *testing.T) {
 
 		snap := timer.Snapshot()
 
-		ps := snap.Percentiles([]float64{5, 20, 50, 95, 99})
+		ps := snap.Percentiles([]float64{0.05, 0.20, 0.50, 0.95, 0.99})
 
 		if tt.wantMin != snap.Min() {
-			t.Fatalf("%d: min: got %d, want %d", ind, snap.Min(), tt.wantMin)
+			t.Errorf("%d: min: got %d, want %d", ind, snap.Min(), tt.wantMin)
 		}
 
 		if tt.wantMax != snap.Max() {
-			t.Fatalf("%d: max: got %d, want %d", ind, snap.Max(), tt.wantMax)
+			t.Errorf("%d: max: got %d, want %d", ind, snap.Max(), tt.wantMax)
 		}
 
 		if tt.wantMean != snap.Mean() {
-			t.Fatalf("%d: mean: got %.2f, want %.2f", ind, snap.Mean(), tt.wantMean)
+			t.Errorf("%d: mean: got %.2f, want %.2f", ind, snap.Mean(), tt.wantMean)
 		}
-
 		if tt.wantP05 != ps[0] {
-			t.Fatalf("%d: p05: got %d, want %d", ind, ps[0], tt.wantP05)
+			t.Errorf("%d: p05: got %v, want %v", ind, ps[0], tt.wantP05)
 		}
-
 		if tt.wantP20 != ps[1] {
-			t.Fatalf("%d: p20: got %d, want %d", ind, ps[1], tt.wantP20)
+			t.Errorf("%d: p20: got %v, want %v", ind, ps[1], tt.wantP20)
 		}
-
 		if tt.wantP50 != ps[2] {
-			t.Fatalf("%d: p50: got %d, want %d", ind, ps[2], tt.wantP50)
+			t.Errorf("%d: p50: got %v, want %v", ind, ps[2], tt.wantP50)
 		}
-
 		if tt.wantP95 != ps[3] {
-			t.Fatalf("%d: p95: got %d, want %d", ind, ps[3], tt.wantP95)
+			t.Errorf("%d: p95: got %v, want %v", ind, ps[3], tt.wantP95)
 		}
-
 		if tt.wantP99 != ps[4] {
-			t.Fatalf("%d: p99: got %d, want %d", ind, ps[4], tt.wantP99)
+			t.Errorf("%d: p99: got %v, want %v", ind, ps[4], tt.wantP99)
 		}
 	}
 }
diff --git a/metrics/sample.go b/metrics/sample.go
@@ -176,29 +176,35 @@ func (NilSample) Values() []int64 { return []int64{} }
 // Variance is a no-op.
 func (NilSample) Variance() float64 { return 0.0 }
 
-// SamplePercentiles returns an arbitrary percentile of the slice of int64.
+// CalculatePercentiles returns an arbitrary percentile of the slice of int64.
 func SamplePercentile(values []int64, p float64) float64 {
-	return SamplePercentiles(values, []float64{p})[0]
+	return CalculatePercentiles(values, []float64{p})[0]
 }
 
-// SamplePercentiles returns a slice of arbitrary percentiles of the slice of
-// int64.
-func SamplePercentiles(values []int64, ps []float64) []float64 {
+// CalculatePercentiles returns a slice of arbitrary percentiles of the slice of
+// int64. This method returns interpolated results, so e.g if there are only two
+// values, [0, 10], a 50% percentile will land between them.
+//
+// Note: As a side-effect, this method will also sort the slice of values.
+// Note2: The input format for percentiles is NOT percent! To express 50%, use 0.5, not 50.
+func CalculatePercentiles(values []int64, ps []float64) []float64 {
 	scores := make([]float64, len(ps))
 	size := len(values)
-	if size > 0 {
-		slices.Sort(values)
-		for i, p := range ps {
-			pos := p * float64(size+1)
-			if pos < 1.0 {
-				scores[i] = float64(values[0])
-			} else if pos >= float64(size) {
-				scores[i] = float64(values[size-1])
-			} else {
-				lower := float64(values[int(pos)-1])
-				upper := float64(values[int(pos)])
-				scores[i] = lower + (pos-math.Floor(pos))*(upper-lower)
-			}
+	if size == 0 {
+		return scores
+	}
+	slices.Sort(values)
+	for i, p := range ps {
+		pos := p * float64(size+1)
+
+		if pos < 1.0 {
+			scores[i] = float64(values[0])
+		} else if pos >= float64(size) {
+			scores[i] = float64(values[size-1])
+		} else {
+			lower := float64(values[int(pos)-1])
+			upper := float64(values[int(pos)])
+			scores[i] = lower + (pos-math.Floor(pos))*(upper-lower)
 		}
 	}
 	return scores
@@ -267,7 +273,7 @@ func (s *sampleSnapshot) Percentile(p float64) float64 {
 // Percentiles returns a slice of arbitrary percentiles of values at the time
 // the snapshot was taken.
 func (s *sampleSnapshot) Percentiles(ps []float64) []float64 {
-	return SamplePercentiles(s.values, ps)
+	return CalculatePercentiles(s.values, ps)
 }
 
 // Size returns the size of the sample at the time the snapshot was taken.