From e061dfae88c0dd63ff477a153096a1ba28f69f7e 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/8] 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 f2fb5bb..c2a14ae 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 dc819ceb1b0f906f1ab124f7492693970733a54d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sat, 31 Aug 2024 10:34:02 +0200 Subject: [PATCH 2/8] Use a trivial solution to #1605 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- prometheus/histogram.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 8d35f2d..c40a98b 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -1682,13 +1682,13 @@ func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { } func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { + n.Lock() + defer n.Unlock() + if cap(n.exemplars) == 0 { return } - n.Lock() - defer n.Unlock() - // The index where to insert the new exemplar. var nIdx int = -1 From dc8e9a4d8a4c7c64d5ae2c9d29a91bb1407d549b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Sat, 31 Aug 2024 11:55:38 +0200 Subject: [PATCH 3/8] 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 --- prometheus/histogram.go | 46 +++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index c40a98b..eb0eb88 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) > n.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 504566f07c680f68743c3a5d239dede48538c7ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Mon, 2 Sep 2024 14:58:23 +0200 Subject: [PATCH 4/8] Use simplified solution from #1609 for the data race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- prometheus/histogram.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index eb0eb88..1690a56 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -1673,6 +1673,7 @@ func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { if maxCount < 0 { maxCount = 0 + ttl = -1 } return nativeExemplars{ @@ -1682,13 +1683,13 @@ func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { } func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { - n.Lock() - defer n.Unlock() - - if cap(n.exemplars) == 0 { + if n.ttl == -1 { return } + n.Lock() + defer n.Unlock() + // The index where to insert the new exemplar. var nIdx int = -1 From d6b8c8925bd16626cf168e642eb70724b17a0d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Wed, 4 Sep 2024 17:03:21 +0200 Subject: [PATCH 5/8] Update comments with more explanations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- prometheus/histogram.go | 61 ++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 1690a56..8a4f49a 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -1658,7 +1658,10 @@ func addAndResetCounts(hot, cold *histogramCounts) { type nativeExemplars struct { sync.Mutex - ttl time.Duration + // Time-to-live for exemplars, it is set to -1 if exemplars are disabled, that is NativeHistogramMaxExemplars is below 0. + // The ttl is used on insertion to remove an exemplar that is older than ttl, if present. + ttl time.Duration + exemplars []*dto.Exemplar } @@ -1690,13 +1693,11 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { n.Lock() defer n.Unlock() - // The index where to insert the new exemplar. - var nIdx int = -1 - // When the number of exemplars has not yet exceeded or // is equal to cap(n.exemplars), then // insert the new exemplar directly. if len(n.exemplars) < cap(n.exemplars) { + var nIdx int for nIdx = 0; nIdx < len(n.exemplars); nIdx++ { if *e.Value < *n.exemplars[nIdx].Value { break @@ -1716,11 +1717,34 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { // When the number of exemplars exceeds the limit, remove one exemplar. var ( - ot = time.Now() // Oldest timestamp seen. - otIdx = -1 // Index of the exemplar with the oldest timestamp. + ot = time.Time{} // Oldest timestamp seen. Initial value doesn't matter as we replace it due to otIdx == -1 in the loop. + otIdx = -1 // Index of the exemplar with the oldest timestamp. - 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. + md = -1.0 // Logarithm of the delta of the closest pair of exemplars. + + // The insertion point of the new exemplar in the exemplars slice after insertion. + // This is calculated purely based on the order of the exemplars by value. + // nIdx == len(n.exemplars) means the new exemplar is to be inserted after the end. + nIdx = -1 + + // rIdx is ultimately the index for the exemplar that we are replacing with the new exemplar. + // The aim is to keep a good spread of exemplars by value and not let them bunch up too much. + // It is calculated in 3 steps: + // 1. First we set rIdx to the index of the older exemplar within the closest pair by value. + // That is the following will be true (on log scale): + // either the exemplar pair on index (rIdx-1, rIdx) or (rIdx, rIdx+1) will have + // the closest values to each other from all pairs. + // For example, suppose the values are distributed like this: + // |-----------x-------------x----------------x----x-----| + // ^--rIdx as this is older. + // Or like this: + // |-----------x-------------x----------------x----x-----| + // ^--rIdx as this is older. + // 2. If there is an exemplar that expired, then we simple reset rIdx to that index. + // 3. We check if by inserting the new exemplar we would create a closer pair at + // (nIdx-1, nIdx) or (nIdx, nIdx+1) and set rIdx to nIdx-1 or nIdx accordingly to + // keep the spread of exemplars by value; otherwise we keep rIdx as it is. + rIdx = -1 cLog float64 // Logarithm of the current exemplar. pLog float64 // Logarithm of the previous exemplar. ) @@ -1745,7 +1769,7 @@ 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. + // The closest exemplar pair is at index: i-1, i. // Choose the exemplar with the older timestamp for replacement. md = diff if n.exemplars[i].Timestamp.AsTime().Before(n.exemplars[i-1].Timestamp.AsTime()) { @@ -1763,7 +1787,8 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { nIdx = len(n.exemplars) } // Here, we have the following relationships: - // n.exemplars[nIdx-1].Value < e.Value <= n.exemplars[nIdx].Value + // n.exemplars[nIdx-1].Value < e.Value (if nIdx > 0) + // e.Value <= n.exemplars[nIdx].Value (if nIdx < len(n.exemplars)) if otIdx != -1 && e.Timestamp.AsTime().Sub(ot) > n.ttl { // If the oldest exemplar has expired, then replace it with the new exemplar. @@ -1776,9 +1801,11 @@ 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. + // The value we are about to insert is closer to the previous exemplar at the insertion point than what we calculated before in rIdx. + // v--rIdx + // |-----------x-n-----------x----------------x----x-----| + // nIdx-1--^ ^--new exemplar value + // Do not make the spread worse, replace nIdx-1 and not rIdx. md = diff rIdx = nIdx - 1 } @@ -1786,9 +1813,11 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { if nIdx < len(n.exemplars) { diff := math.Abs(math.Log(n.exemplars[nIdx].GetValue()) - elog) if diff < md { - // 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. + // The value we are about to insert is closer to the next exemplar at the insertion point than what we calculated before in rIdx. + // v--rIdx + // |-----------x-----------n-x----------------x----x-----| + // new exemplar value--^ ^--nIdx + // Do not make the spread worse, replace nIdx-1 and not rIdx. rIdx = nIdx } } From 937ac63d3d2dda83847f4ca842d62edabce4e743 Mon Sep 17 00:00:00 2001 From: Arthur Silva Sens Date: Thu, 5 Sep 2024 09:23:41 -0300 Subject: [PATCH 6/8] Add changelog entry for 1.20.3 Signed-off-by: Arthur Silva Sens --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f83a027..2c55343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Unreleased +## 1.20.3 / 2024-09-05 + +* [BUGFIX] histograms: Fix possible data race when appending exemplars. #1608 + ## 1.20.2 / 2024-08-23 * [BUGFIX] promhttp: Unset Content-Encoding header when data is uncompressed. #1596 From 1e398ccb1259d20836e3003885bdd949cb21e635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 6 Sep 2024 11:01:04 +0200 Subject: [PATCH 7/8] native histogram: Fix race between Write and addExemplar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 1608 Signed-off-by: György Krajcsovits --- prometheus/histogram.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 8a4f49a..519db34 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -844,9 +844,7 @@ 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 { + if h.nativeExemplars.isEnabled() { h.nativeExemplars.Lock() his.Exemplars = append(his.Exemplars, h.nativeExemplars.exemplars...) h.nativeExemplars.Unlock() @@ -1665,6 +1663,10 @@ type nativeExemplars struct { exemplars []*dto.Exemplar } +func (n *nativeExemplars) isEnabled() bool { + return n.ttl != -1 +} + func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { if ttl == 0 { ttl = 5 * time.Minute @@ -1686,7 +1688,7 @@ func makeNativeExemplars(ttl time.Duration, maxCount int) nativeExemplars { } func (n *nativeExemplars) addExemplar(e *dto.Exemplar) { - if n.ttl == -1 { + if !n.isEnabled() { return } From 209f4c041ed1764866f44dd053a8d94aa051c610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Fri, 6 Sep 2024 12:41:51 +0200 Subject: [PATCH 8/8] Add changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: György Krajcsovits --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c55343..f390ed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased +* [BUGFIX] histograms: Fix possible data race when appending exemplars vs metrics gather. #1623 + ## 1.20.3 / 2024-09-05 * [BUGFIX] histograms: Fix possible data race when appending exemplars. #1608