From 0abadecea19eca1bb964c341d259c4e911067e51 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 31 Aug 2023 14:48:53 +0200 Subject: [PATCH] metrics: align percentiles handling in resetting timer with rest of the codebase --- metrics/exp/exp.go | 10 ++-- metrics/influxdb/influxdb.go | 2 +- metrics/prometheus/collector.go | 2 +- metrics/resetting_timer.go | 60 +++++------------------ metrics/resetting_timer_test.go | 85 ++++++++++++++------------------- metrics/sample.go | 44 +++++++++-------- metrics/sample_test.go | 14 ++++++ 7 files changed, 95 insertions(+), 122 deletions(-) diff --git a/metrics/exp/exp.go b/metrics/exp/exp.go index 100fd9d167..b273ee03c7 100644 --- 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 index 83c2edfb0d..227c2bd572 100644 --- 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 index 45e8a914be..d6a0b4200c 100644 --- 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 index b5bbc753cc..8fb27c8a5c 100644 --- 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 index 5bc88e38bd..4571fc8eb0 100644 --- 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 index d52a92c068..20a7997478 100644 --- 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. diff --git a/metrics/sample_test.go b/metrics/sample_test.go index a57aba7868..e15ffed15b 100644 --- a/metrics/sample_test.go +++ b/metrics/sample_test.go @@ -367,3 +367,17 @@ func TestUniformSampleConcurrentUpdateCount(t *testing.T) { } quit <- struct{}{} } + +func BenchmarkCalculatePercentiles(b *testing.B) { + pss := []float64{0.5, 0.75, 0.95, 0.99, 0.999, 0.9999} + var vals []int64 + for i := 0; i < 1000; i++ { + vals = append(vals, int64(rand.Int31())) + } + v := make([]int64, len(vals)) + b.ResetTimer() + for i := 0; i < b.N; i++ { + copy(v, vals) + _ = CalculatePercentiles(v, pss) + } +}