add native histogram exemplar support #1471

fatsheep9146 · 2024-03-15T01:28:45Z

ArthurSens

not a native histogram expert here, just some small comments :)

prometheus/histogram.go

prometheus/histogram_test.go

beorn7

Thanks for doing this. I think this implements the correct algorithm, but there is the huge problem of concurrency. See comments. A also think a more basic approach with a single slice rather than two linked list and a map will be faster in practice and consume less memory. Also see comments.

prometheus/histogram.go

beorn7 · 2024-04-03T13:07:36Z

I'll try to get to this ASAP, but currently, I'm sick.

fatsheep9146 · 2024-04-04T07:27:48Z

I'll try to get to this ASAP, but currently, I'm sick.

Thanks! Please take good care of yourself and rest well, there is no rush

kakkoyun

Nice to have this. Thanks for the contribution 🎉

beorn7

Thanks a lot. I finally have done a detailed review. Things are a bit tricky, but I hope I have explained it well enough.

In different news, the doc comment for the ExemplarObserver interface (in file observer.go) needs an update, explaining broadly the behavior for native histograms, which should also include a warning that ObserveWithExemplar isn't lock-free if called on a native histogram with configured exemplars so that it should not be called too frequently.

prometheus/histogram.go

fatsheep9146 · 2024-04-20T03:04:20Z

@beorn7 The pr is ready for review again.

beorn7 · 2024-04-23T11:46:39Z

Thank you very much. I'm on vacation this week, but this is high on my list once I'm back in action.

beorn7

Very cool.

I think all that remains is wordsmithing and some formatting tweaks. See all my pedantic comments. :)

prometheus/histogram.go

fatsheep9146 · 2024-05-03T02:44:00Z

@beorn7
The commit history seems too long, I can squash to one commit if you think there are no more changes needed.

beorn7 · 2024-05-08T17:42:21Z

No worries, I can just squash during merge.

Or, if you like a more detailed commit structure with more than one commit, but not the 20 we have now, you could rebase on your own.

beorn7 · 2024-05-08T17:46:16Z

One thing fell through the cracks: The doc comment to the ObserveWithExemplar method in histogram.go should contain a warning that it isn't lock-free for native histograms with configured exemplars and therefore should not be called in high-frequency settings.

beorn7 · 2024-05-08T17:46:49Z

Otherwise, this looks good. I'll give @ArthurSens @bwplotka and @kakkoyun a few more days to raise objections.

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 · 2024-05-08T23:22:27Z

@beorn7 I only kept two commits, one for first implementation, the other for reconstitution that you suggested in #1471 (comment)

bwplotka

Happy to get this in, just one suggestion. Thanks!!! Sorry for lag on review.

prometheus/histogram.go

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

bwplotka · 2024-05-09T15:10:59Z

It feels @beorn7 comments are resolved, did you intent @beorn7 to approve it after you requested changes? #1471 (review)

fatsheep9146 · 2024-05-10T01:08:59Z

It feels @beorn7 comments are resolved, did you intent @beorn7 to approve it after you requested changes?

Yes, all comments are resolved, but I think @beorn7 is waiting for other reviewers to raise objections?

Otherwise, this looks good. I'll give @ArthurSens @bwplotka and @kakkoyun a few more days to raise objections.

prometheus/histogram.go

bwplotka · 2024-05-10T08:21:48Z

Well, but "Change Requested" status is still there. And that from @beorn7 .... one never should mess with the red mark from @beorn7 =DDDD

On serious note, I wanted to be lazy and trust @beorn7 to approve the most complex part of this PR, so addExemplar body properly, I only skimmed through that. Let me check that next week (I have to focus on other work now), if @beorn7 won't have time.

Is that ok? We can also merge and iterate... it's a new functionality.

fatsheep9146 · 2024-05-11T01:39:23Z

Well, but "Change Requested" status is still there. And that from @beorn7 .... one never should mess with the red mark from @beorn7 =DDDD

On serious note, I wanted to be lazy and trust @beorn7 to approve the most complex part of this PR, so addExemplar body properly, I only skimmed through that. Let me check that next week (I have to focus on other work now), if @beorn7 won't have time.

Is that ok? We can also merge and iterate... it's a new functionality.

I think it's better to do a more detailed review of this pr when you have time next week. I'm definitely ok with that. @bwplotka

ArthurSens

I just a quick read through the tests and I already have to leave 😬

I'll try to take another look at addExamplar tomorrow but I'm similar to bartek here, I trust Beorn's review and there's no need to wait for me if we want to merge it already

ArthurSens · 2024-05-14T16:46:14Z

prometheus/histogram_test.go

+	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])
+				}
+			}
+		})
+	}


This part repeats 3x in this test, maybe we can extract into a separate function just to reduce a few lines?

I convert the compare logic into one common function compareNativeExemplarValues

ArthurSens · 2024-05-14T16:52:07Z

prometheus/histogram_test.go

+		{
+			name: "remove exemplar with oldest timestamp, the removed index is smaller than inserted index",
+			addFunc: func(h *histogram) {
+				time.Sleep(10 * time.Second)


histograms have a field called now, their intended usage is exactly to mock time and avoid sleeps in tests.

You could do something like this when creating the histogram

now := time.Now() h.now = func time.Time { return now }

in addFunc you can change the returned value to:

h.now = func time.Time { return now.Add(time.Second * 10) }

this would pretend that 10 seconds have passed, without needing to wait 10 seconds :)

beorn7 · 2024-05-15T16:56:42Z

prometheus/histogram.go

+// ObserveWithExemplar should not be called in high-frequency settings,
+// since it isn't lock-free for native histograms with configured exemplars.


Slight clarification:

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.

beorn7 · 2024-05-15T16:58:45Z

I think @ArthurSens 's comments about the tests are valid. @fatsheep9146 I think you should address them.

Otherwise, I only have a slight clarification for the ObserveWithExemplar doc comment (to make clear that it is still lock-free if called for a native histogram without exemplars or for a classic histogram).

And then this is good to go. Thanks for all your efforts and patience. This was quite involved.

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

beorn7 · 2024-05-16T10:18:44Z

Great. I'll merge on green.

fatsheep9146 force-pushed the native-histogram-exemplar branch from 63156a1 to 5dcfaff Compare March 15, 2024 01:30

beorn7 self-requested a review March 15, 2024 08:56

ArthurSens reviewed Mar 15, 2024

View reviewed changes

prometheus/histogram.go Outdated Show resolved Hide resolved

prometheus/histogram_test.go Outdated Show resolved Hide resolved

beorn7 requested changes Mar 20, 2024

View reviewed changes

beorn7 reviewed Mar 27, 2024

View reviewed changes

prometheus/histogram.go Outdated Show resolved Hide resolved

kakkoyun reviewed Apr 8, 2024

View reviewed changes

beorn7 requested changes Apr 10, 2024

View reviewed changes

fatsheep9146 requested a review from beorn7 April 20, 2024 03:03

beorn7 requested changes May 2, 2024

View reviewed changes

fatsheep9146 requested review from ArthurSens and beorn7 May 3, 2024 01:38

add native histogram exemplar support

494ccce

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 force-pushed the native-histogram-exemplar branch from 6623198 to 85f5475 Compare May 8, 2024 23:19

fatsheep9146 requested a review from kakkoyun May 8, 2024 23:24

bwplotka approved these changes May 9, 2024

View reviewed changes

prometheus/histogram.go Outdated Show resolved Hide resolved

refract the implementation

d8c7074

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 force-pushed the native-histogram-exemplar branch from 85f5475 to d8c7074 Compare May 9, 2024 14:12

bwplotka reviewed May 10, 2024

View reviewed changes

prometheus/histogram.go Show resolved Hide resolved

ArthurSens reviewed May 14, 2024

View reviewed changes

beorn7 reviewed May 15, 2024

View reviewed changes

fix for comments

2754a4c

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 requested review from ArthurSens and beorn7 May 16, 2024 10:17

beorn7 approved these changes May 16, 2024

View reviewed changes

beorn7 merged commit 542f7e6 into prometheus:main May 16, 2024
9 checks passed

renovate bot mentioned this pull request Aug 20, 2024

chore(deps): update all dependencies (main) ksimon1/ssp-operator#2

Open

1 task

yvrhdn mentioned this pull request Dec 20, 2024

Metrics-generator: use exemplars from native histograms grafana/tempo#4482

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add native histogram exemplar support #1471

add native histogram exemplar support #1471

fatsheep9146 commented Mar 15, 2024

ArthurSens left a comment

beorn7 left a comment

beorn7 commented Apr 3, 2024

fatsheep9146 commented Apr 4, 2024

kakkoyun left a comment

beorn7 left a comment

fatsheep9146 commented Apr 20, 2024

beorn7 commented Apr 23, 2024

beorn7 left a comment

fatsheep9146 commented May 3, 2024

beorn7 commented May 8, 2024

beorn7 commented May 8, 2024

beorn7 commented May 8, 2024

fatsheep9146 commented May 8, 2024

bwplotka left a comment

bwplotka commented May 9, 2024

fatsheep9146 commented May 10, 2024

bwplotka commented May 10, 2024

fatsheep9146 commented May 11, 2024

ArthurSens left a comment

ArthurSens May 14, 2024

fatsheep9146 May 16, 2024

ArthurSens May 14, 2024

fatsheep9146 May 16, 2024

beorn7 May 15, 2024

fatsheep9146 May 16, 2024

beorn7 commented May 15, 2024

beorn7 commented May 16, 2024

		// ObserveWithExemplar should not be called in high-frequency settings,
		// since it isn't lock-free for native histograms with configured exemplars.

add native histogram exemplar support #1471

add native histogram exemplar support #1471

Conversation

fatsheep9146 commented Mar 15, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 commented Apr 3, 2024

fatsheep9146 commented Apr 4, 2024

kakkoyun left a comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Apr 20, 2024

beorn7 commented Apr 23, 2024

beorn7 left a comment

Choose a reason for hiding this comment

fatsheep9146 commented May 3, 2024

beorn7 commented May 8, 2024

beorn7 commented May 8, 2024

beorn7 commented May 8, 2024

fatsheep9146 commented May 8, 2024

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented May 9, 2024

fatsheep9146 commented May 10, 2024

bwplotka commented May 10, 2024

fatsheep9146 commented May 11, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens May 14, 2024

Choose a reason for hiding this comment

fatsheep9146 May 16, 2024

Choose a reason for hiding this comment

ArthurSens May 14, 2024

Choose a reason for hiding this comment

fatsheep9146 May 16, 2024

Choose a reason for hiding this comment

beorn7 May 15, 2024

Choose a reason for hiding this comment

fatsheep9146 May 16, 2024

Choose a reason for hiding this comment

beorn7 commented May 15, 2024

beorn7 commented May 16, 2024