From 8fb77a34a97e52b91d2fbd85db122684c0732e78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sat, 31 Aug 2024 10:27:18 +0200 Subject: [PATCH 1/4] native histogram: use exemplars in concurrency test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- prometheus/histogram_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/prometheus/histogram_test.go b/prometheus/histogram_test.go index f2fb5bb8c..c2a14ae72 100644 --- a/prometheus/histogram_test.go +++ b/prometheus/histogram_test.go @@ -1049,10 +1049,14 @@ func TestNativeHistogramConcurrency(t *testing.T) { go func(vals []float64) { start.Wait() - for _, v := range vals { + for i, v := range vals { // An observation every 1 to 10 seconds. atomic.AddInt64(&ts, rand.Int63n(10)+1) - his.Observe(v) + if i%2 == 0 { + his.Observe(v) + } else { + his.(ExemplarObserver).ObserveWithExemplar(v, Labels{"foo": "bar"}) + } } end.Done() }(vals) From 5123705714d160f67c6e050536bdd1e54c53c3a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sat, 31 Aug 2024 10:51:37 +0200 Subject: [PATCH 2/4] Use an atomic variable to check if exemplars are disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of checking non thread safe slice capacity Signed-off-by: György Krajcsovits --- prometheus/histogram.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 8d35f2d8a..b94bdd815 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -573,7 +573,7 @@ func newHistogram(desc *Desc, opts HistogramOpts, labelValues ...string) Histogr h.nativeHistogramZeroThreshold = DefNativeHistogramZeroThreshold } // Leave h.nativeHistogramZeroThreshold at 0 otherwise. h.nativeHistogramSchema = pickSchema(opts.NativeHistogramBucketFactor) - h.nativeExemplars = makeNativeExemplars(opts.NativeHistogramExemplarTTL, opts.NativeHistogramMaxExemplars) + makeNativeExemplars(&h.nativeExemplars, opts.NativeHistogramExemplarTTL, opts.NativeHistogramMaxExemplars) } for i, upperBound := range h.upperBounds { if i < len(h.upperBounds)-1 { @@ -1658,11 +1658,11 @@ func addAndResetCounts(hot, cold *histogramCounts) { type nativeExemplars struct { sync.Mutex - ttl time.Duration + ttl atomic.Int64 // It is a duration, but also used to check concurrently if exemplars are enabled. exemplars []*dto.Exemplar } -func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { +func makeNativeExemplars(exemplars *nativeExemplars, ttl time.Duration, maxCount int) { if ttl == 0 { ttl = 5 * time.Minute } @@ -1673,16 +1673,16 @@ func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { if maxCount < 0 { maxCount = 0 + ttl = -1 } - return nativeExemplars{ - ttl: ttl, - exemplars: make([]*dto.Exemplar, 0, maxCount), - } + exemplars.ttl.Store(int64(ttl)) + exemplars.exemplars = make([]*dto.Exemplar, 0, maxCount) } func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { - if cap(n.exemplars) == 0 { + ttl := n.ttl.Load() + if ttl == -1 { return } @@ -1754,7 +1754,7 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { nIdx = len(n.exemplars) } - if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > n.ttl { + if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > time.Duration(ttl) { rIdx = otIdx } else { // In the previous for loop, when calculating the closest pair of exemplars, From e9cbc7b6316457b7e0f55575317cb7339d61a1fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sat, 31 Aug 2024 13:06:47 +0200 Subject: [PATCH 3/4] fix: native histogram: Simplify and fix addExemplar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mdIdx was redundant when len(exemplars)>1, so got rid of it, rIdx is enough. Don't compare timestamp of incoming exemplar to timestamp of minimal distance exemplar. Most of the time the incoming exemplar will be newer. And if not, the previous code just replaced an exemplar one index after the minimal distance exemplar. Which had an index out of range bug, plus is essentially random. Signed-off-by: György Krajcsovits # Conflicts: # prometheus/histogram.go --- prometheus/histogram.go | 46 +++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index b94bdd815..501635aa4 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -1705,17 +1705,23 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { return } + if len(n.exemplars) == 1 { + // When the number of exemplars is 1, then + // replace the existing exemplar with the new exemplar. + n.exemplars[0] = e + return + } + // From this point on, the number of exemplars is greater than 1. + // When the number of exemplars exceeds the limit, remove one exemplar. var ( - rIdx int // The index where to remove the old exemplar. - ot = time.Now() // Oldest timestamp seen. otIdx = -1 // Index of the exemplar with the oldest timestamp. - md = -1.0 // Logarithm of the delta of the closest pair of exemplars. - mdIdx = -1 // Index of the older exemplar within the closest pair. - cLog float64 // Logarithm of the current exemplar. - pLog float64 // Logarithm of the previous exemplar. + md = -1.0 // Logarithm of the delta of the closest pair of exemplars. + rIdx = -1 // Index of the older exemplar within the closest pair and where we need to insert the new exemplar. + cLog float64 // Logarithm of the current exemplar. + pLog float64 // Logarithm of the previous exemplar. ) for i, exemplar := range n.exemplars { @@ -1726,7 +1732,7 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { } // Find the index at which to insert new the exemplar. - if *e.Value <= *exemplar.Value && nIdx == -1 { + if nIdx == -1 && *e.Value <= *exemplar.Value { nIdx = i } @@ -1738,11 +1744,13 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { } diff := math.Abs(cLog - pLog) if md == -1 || diff < md { + // The closest exemplar pair is this: |exemplar.[i] - n.exemplars[i-1].Value| is minimal. + // Choose the exemplar with the older timestamp for replacement. md = diff if n.exemplars[i].Timestamp.AsTime().Before(n.exemplars[i-1].Timestamp.AsTime()) { - mdIdx = i + rIdx = i } else { - mdIdx = i - 1 + rIdx = i - 1 } } @@ -1753,8 +1761,11 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { if nIdx == -1 { nIdx = len(n.exemplars) } + // Here, we have the following relationships: + // n.exemplars[nIdx-1].Value < e.Value <= n.exemplars[nIdx].Value if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > time.Duration(ttl) { + // If the oldest exemplar has expired, then replace it with the new exemplar. rIdx = otIdx } else { // In the previous for loop, when calculating the closest pair of exemplars, @@ -1764,23 +1775,22 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { if nIdx > 0 { diff := math.Abs(elog - math.Log(n.exemplars[nIdx-1].GetValue())) if diff < md { + // The closest exemplar pair is this: |e.Value - n.exemplars[nIdx-1].Value| is minimal. + // Assume that the exemplar we are inserting has a newer timestamp. This is not always + // true, due to concurrency, but it's a good enough approximation. md = diff - mdIdx = nIdx - if n.exemplars[nIdx-1].Timestamp.AsTime().Before(e.Timestamp.AsTime()) { - mdIdx = nIdx - 1 - } + rIdx = nIdx - 1 } } if nIdx < len(n.exemplars) { diff := math.Abs(math.Log(n.exemplars[nIdx].GetValue()) - elog) if diff < md { - mdIdx = nIdx - if n.exemplars[nIdx].Timestamp.AsTime().Before(e.Timestamp.AsTime()) { - mdIdx = nIdx - } + // The closest exemplar pair is this: |n.exemplars[nIdx].Value - e.Value| is minimal. + // Assume that the exemplar we are inserting has a newer timestamp. This is not always + // true, due to concurrency, but it's a good enough approximation. + rIdx = nIdx } } - rIdx = mdIdx } // Adjust the slice according to rIdx and nIdx. From 6c6aa53f38d118735011749dc75322495b0762aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Mon, 2 Sep 2024 08:56:57 +0200 Subject: [PATCH 4/4] Do not use atomics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- prometheus/histogram.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 501635aa4..1690a5695 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -573,7 +573,7 @@ func newHistogram(desc *Desc, opts HistogramOpts, labelValues ...string) Histogr h.nativeHistogramZeroThreshold = DefNativeHistogramZeroThreshold } // Leave h.nativeHistogramZeroThreshold at 0 otherwise. h.nativeHistogramSchema = pickSchema(opts.NativeHistogramBucketFactor) - makeNativeExemplars(&h.nativeExemplars, opts.NativeHistogramExemplarTTL, opts.NativeHistogramMaxExemplars) + h.nativeExemplars = makeNativeExemplars(opts.NativeHistogramExemplarTTL, opts.NativeHistogramMaxExemplars) } for i, upperBound := range h.upperBounds { if i < len(h.upperBounds)-1 { @@ -1658,11 +1658,11 @@ func addAndResetCounts(hot, cold *histogramCounts) { type nativeExemplars struct { sync.Mutex - ttl atomic.Int64 // It is a duration, but also used to check concurrently if exemplars are enabled. + ttl time.Duration exemplars []*dto.Exemplar } -func makeNativeExemplars(exemplars *nativeExemplars, ttl time.Duration, maxCount int) { +func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { if ttl == 0 { ttl = 5 * time.Minute } @@ -1676,13 +1676,14 @@ func makeNativeExemplars(exemplars *nativeExemplars, ttl time.Duration, maxCount ttl = -1 } - exemplars.ttl.Store(int64(ttl)) - exemplars.exemplars = make([]*dto.Exemplar, 0, maxCount) + return nativeExemplars{ + ttl: ttl, + exemplars: make([]*dto.Exemplar, 0, maxCount), + } } func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { - ttl := n.ttl.Load() - if ttl == -1 { + if n.ttl == -1 { return } @@ -1764,7 +1765,7 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { // Here, we have the following relationships: // n.exemplars[nIdx-1].Value < e.Value <= n.exemplars[nIdx].Value - if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > time.Duration(ttl) { + if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > n.ttl { // If the oldest exemplar has expired, then replace it with the new exemplar. rIdx = otIdx } else {