From 2754a4c204030a5f55ce0ee2fe8faffe77680851 Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Thu, 16 May 2024 18:16:36 +0800 Subject: [PATCH] fix for comments Signed-off-by: Ziqi Zhao --- prometheus/histogram.go | 9 +++++--- prometheus/histogram_test.go | 40 ++++++++++++++---------------------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index d247d77..bf5f68a 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -761,8 +761,9 @@ func (h *histogram) Observe(v float64) { h.observe(v, h.findBucket(v)) } -// ObserveWithExemplar should not be called in high-frequency settings, -// since it isn't lock-free for native histograms with configured exemplars. +// ObserveWithExemplar should not be called in a high-frequency setting +// for a native histogram with configured exemplars. For this case, +// the implementation isn't lock-free and might suffer from lock contention. func (h *histogram) ObserveWithExemplar(v float64, e Labels) { i := h.findBucket(v) h.observe(v, i) @@ -843,6 +844,8 @@ func (h *histogram) Write(out *dto.Metric) error { }} } + // If exemplars are not configured, the cap will be 0. + // So append is not needed in this case. if cap(h.nativeExemplars.exemplars) > 0 { h.nativeExemplars.Lock() his.Exemplars = append(his.Exemplars, h.nativeExemplars.exemplars...) @@ -1709,7 +1712,7 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { nIdx = len(n.exemplars) } - if otIdx != -1 && time.Since(ot) > n.ttl { + if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > n.ttl { rIdx = otIdx } else { // In the previous for loop, when calculating the closest pair of exemplars, diff --git a/prometheus/histogram_test.go b/prometheus/histogram_test.go index 40ced59..57fafd3 100644 --- a/prometheus/histogram_test.go +++ b/prometheus/histogram_test.go @@ -1314,7 +1314,7 @@ func TestNativeHistogramExemplar(t *testing.T) { { name: "remove exemplar with oldest timestamp, the removed index is smaller than inserted index", addFunc: func(h *histogram) { - time.Sleep(10 * time.Second) + h.now = func() time.Time { return time.Now().Add(time.Second * 11) } h.ObserveWithExemplar(6, Labels{"id": "1"}) }, expectedValues: []float64{0, 4, 6}, @@ -1324,14 +1324,7 @@ func TestNativeHistogramExemplar(t *testing.T) { for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { tc.addFunc(h) - if len(h.nativeExemplars.exemplars) != len(tc.expectedValues) { - t.Errorf("the count of exemplars is not %d", len(tc.expectedValues)) - } - for i, e := range h.nativeExemplars.exemplars { - if e.GetValue() != tc.expectedValues[i] { - t.Errorf("the %dth exemplar value %v is not as expected: %v", i, e.GetValue(), tc.expectedValues[i]) - } - } + compareNativeExemplarValues(t, h.nativeExemplars.exemplars, tc.expectedValues) }) } @@ -1385,14 +1378,7 @@ func TestNativeHistogramExemplar(t *testing.T) { for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { tc.addFunc(h) - if len(h.nativeExemplars.exemplars) != len(tc.expectedValues) { - t.Errorf("the count of exemplars is not %d", len(tc.expectedValues)) - } - for i, e := range h.nativeExemplars.exemplars { - if e.GetValue() != tc.expectedValues[i] { - t.Errorf("the %dth exemplar value %v is not as expected: %v", i, e.GetValue(), tc.expectedValues[i]) - } - } + compareNativeExemplarValues(t, h.nativeExemplars.exemplars, tc.expectedValues) }) } @@ -1425,14 +1411,18 @@ func TestNativeHistogramExemplar(t *testing.T) { for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { tc.addFunc(h) - if len(h.nativeExemplars.exemplars) != len(tc.expectedValues) { - t.Errorf("the count of exemplars is not %d", len(tc.expectedValues)) - } - for i, e := range h.nativeExemplars.exemplars { - if e.GetValue() != tc.expectedValues[i] { - t.Errorf("the %dth exemplar value %v is not as expected: %v", i, e.GetValue(), tc.expectedValues[i]) - } - } + compareNativeExemplarValues(t, h.nativeExemplars.exemplars, tc.expectedValues) }) } } + +func compareNativeExemplarValues(t *testing.T, exps []*dto.Exemplar, values []float64) { + if len(exps) != len(values) { + t.Errorf("the count of exemplars is not %d", len(values)) + } + for i, e := range exps { + if e.GetValue() != values[i] { + t.Errorf("the %dth exemplar value %v is not as expected: %v", i, e.GetValue(), values[i]) + } + } +}