bugfix: native histogram: exemplars index out of range #1608

krajorama · 2024-08-31T08:27:59Z

Fixes: #1607
Fixes: #1605

fix: native histogram: Simplify and fix addExemplar

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.

Contains an unoptimized fix for #1605 , see discussion / fix in #1609

panic: runtime error: index out of range [10] with length 10

goroutine 10 [running]:
github.com/prometheus/client_golang/prometheus.(*nativeExemplars).addExemplar(0xc0002307c8, 0xc00055f4f0)
	/home/krajo/go/github.com/krajorama/client_golang/prometheus/histogram.go:1791 +0x1969
github.com/prometheus/client_golang/prometheus.(*histogram).updateExemplar(0xc000230700, 0x3ff22643022060a2, 0x0, 0xc00054fcb0)
	/home/krajo/go/github.com/krajorama/client_golang/prometheus/histogram.go:1140 +0x117
github.com/prometheus/client_golang/prometheus.(*histogram).ObserveWithExemplar(0xb64180?, 0xc00054fcb0?, 0xbe246f?)
	/home/krajo/go/github.com/krajorama/client_golang/prometheus/histogram.go:770 +0x6a
github.com/prometheus/client_golang/prometheus.TestNativeHistogramConcurrency.func1.2({0xc0005d0000, 0x46e4, 0x420820?})
	/home/krajo/go/github.com/krajorama/client_golang/prometheus/histogram_test.go:1058 +0x17b
created by github.com/prometheus/client_golang/prometheus.TestNativeHistogramConcurrency.func1 in goroutine 21
	/home/krajo/go/github.com/krajorama/client_golang/prometheus/histogram_test.go:1050 +0x325

krajorama · 2024-08-31T09:57:54Z

cc @fatsheep9146

beorn7

LGTM, but @fatsheep9146 might have an opinion here, too.

Also, leaving it to the maintainers @ArthurSens @kakkoyun @bwplotka to make the final call.

prometheus/histogram.go

ArthurSens

LGTM

bwplotka · 2024-09-02T14:07:38Z

Does it needs a minor 1.20 release with this? Or we are ok with 1.21 in a few months or so?

krajorama · 2024-09-02T17:25:25Z

Does it needs a minor 1.20 release with this? Or we are ok with 1.21 in a few months or so?

I'd love to get a minor release so we can do clean dependency update in Mimir.

ArthurSens · 2024-09-02T18:08:06Z

I'll keep this open for another day or so just in case Bartek or Kemal want to give another review. I can create another release, but then we need to point this PR to the release-1.20 branch

fatsheep9146

Thanks for picking this bug!
This fix is LGTM.

bwplotka

Some readability suggestion, some question likely due to my lack of context for the details. Plus please rebase on release-1.20 for this to be in minor patch. Otherwise LGTM!

prometheus/histogram.go

bwplotka · 2024-09-04T12:51:25Z

prometheus/histogram.go

@@ -1764,23 +1776,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.


Something is off here. Do you mean...

Suggested change

// The closest exemplar pair is this: |e.Value - n.exemplars[nIdx-1].Value| is minimal.

// The smaller exemplar is closer (n.exemplars[nIdx-1].Value), replace that one.

bwplotka · 2024-09-04T12:52:45Z

prometheus/histogram.go

-				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.


bwplotka · 2024-09-04T12:54:28Z

prometheus/histogram.go

-				if n.exemplars[nIdx-1].Timestamp.AsTime().Before(e.Timestamp.AsTime()) {
-					mdIdx = nIdx - 1
-				}
+				rIdx = nIdx - 1


Are we not forgetting to choose larger exemplar rIdx = nIdx for diff > md case? Why we have to recalculate another diff below?

bwplotka · 2024-09-04T12:58:22Z

prometheus/histogram.go

@@ -1764,23 +1776,22 @@ func (n *nativeExemplars) addExemplar(e *dto.Exemplar) {
 		if nIdx > 0 {


Did is odd, why we need this, we just set it to len if it's -1, plus we know len is > 0 because we are in the logic that has to remove something. Is it because nIdx can be zero which means we have to put exemplar upfront?

Then let's fix comment on line 1766, it's wrong:

// Here, we have the following relationships: // n.exemplars[nIdx-1].Value < e.Value <= n.exemplars[nIdx].Value

bwplotka · 2024-09-04T13:00:14Z

prometheus/histogram.go

-				if n.exemplars[nIdx-1].Timestamp.AsTime().Before(e.Timestamp.AsTime()) {
-					mdIdx = nIdx - 1
-				}
+				rIdx = nIdx - 1
 			}
 		}
 		if nIdx < len(n.exemplars) {


I guess this is then guarding the case when nIdx is the last element? Then again, comment 1766 is kind of wrong, or misleading.

krajorama · 2024-09-04T15:03:54Z

@bwplotka I've updated the comments in the code with more explanations and my understanding of the code :)

bwplotka · 2024-09-04T18:38:05Z

I forced this PR to be against release-1.20 branch.. which brought new commits from main. Do you mind rebasing your commits to release-1.20 branch? (:

bwplotka

Thanks! (just rebase)

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

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 <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama · 2024-09-04T19:13:06Z

Thanks! (just rebase)

Rebased and PR re-targetted

krajorama marked this pull request as ready for review August 31, 2024 09:56

krajorama requested a review from beorn7 August 31, 2024 09:57

krajorama mentioned this pull request Aug 31, 2024

bugfix: native histogram: data race in addExemplar #1609

Closed

beorn7 approved these changes Sep 1, 2024

View reviewed changes

prometheus/histogram.go Outdated Show resolved Hide resolved

ArthurSens approved these changes Sep 2, 2024

View reviewed changes

fatsheep9146 approved these changes Sep 4, 2024

View reviewed changes

bwplotka reviewed Sep 4, 2024

View reviewed changes

bwplotka changed the base branch from main to release-1.20 September 4, 2024 18:37

bwplotka approved these changes Sep 4, 2024

View reviewed changes

krajorama added 5 commits September 4, 2024 21:10

native histogram: use exemplars in concurrency test

e061dfa

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Use a trivial solution to prometheus#1605

dc819ce

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Use simplified solution from prometheus#1609 for the data race

504566f

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Update comments with more explanations

d6b8c89

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama force-pushed the index-out-of-range-native-histogram-exemplar branch from d82721b to d6b8c89 Compare September 4, 2024 19:10

krajorama merged commit 6e9914d into prometheus:release-1.20 Sep 4, 2024
8 checks passed

This was referenced Sep 5, 2024

merge upstream at prometheus/prometheus@536d9f9 grafana/mimir-prometheus#690

Merged

native histogram: Fix race between Write and addExemplar #1623

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: native histogram: exemplars index out of range #1608

bugfix: native histogram: exemplars index out of range #1608

krajorama commented Aug 31, 2024 •

edited

Loading

krajorama commented Aug 31, 2024

beorn7 left a comment

ArthurSens left a comment

bwplotka commented Sep 2, 2024

krajorama commented Sep 2, 2024

ArthurSens commented Sep 2, 2024 •

edited

Loading

fatsheep9146 left a comment

bwplotka left a comment

bwplotka Sep 4, 2024

bwplotka Sep 4, 2024

bwplotka Sep 4, 2024

bwplotka Sep 4, 2024

bwplotka Sep 4, 2024

krajorama commented Sep 4, 2024

bwplotka commented Sep 4, 2024

bwplotka left a comment

krajorama commented Sep 4, 2024

	// The closest exemplar pair is this: \|e.Value - n.exemplars[nIdx-1].Value\| is minimal.
	// The smaller exemplar is closer (n.exemplars[nIdx-1].Value), replace that one.

		@@ -1764,23 +1776,22 @@ func (n nativeExemplars) addExemplar(e dto.Exemplar) {
		if nIdx > 0 {

bugfix: native histogram: exemplars index out of range #1608

bugfix: native histogram: exemplars index out of range #1608

Conversation

krajorama commented Aug 31, 2024 • edited Loading

krajorama commented Aug 31, 2024

beorn7 left a comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

bwplotka commented Sep 2, 2024

krajorama commented Sep 2, 2024

ArthurSens commented Sep 2, 2024 • edited Loading

fatsheep9146 left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Sep 4, 2024

Choose a reason for hiding this comment

bwplotka Sep 4, 2024

Choose a reason for hiding this comment

bwplotka Sep 4, 2024

Choose a reason for hiding this comment

bwplotka Sep 4, 2024

Choose a reason for hiding this comment

bwplotka Sep 4, 2024

Choose a reason for hiding this comment

krajorama commented Sep 4, 2024

bwplotka commented Sep 4, 2024

bwplotka left a comment

Choose a reason for hiding this comment

krajorama commented Sep 4, 2024

krajorama commented Aug 31, 2024 •

edited

Loading

ArthurSens commented Sep 2, 2024 •

edited

Loading