add native histogram exemplar support

[fatsheep9146]

fix #1126

[ArthurSens]

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

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

[beorn7]

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

[fatsheep9146]

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.

[fatsheep9146]

@beorn7 The pr is ready for review again.

[beorn7]

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

[fatsheep9146]

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

[beorn7]

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]

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]

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

[fatsheep9146]

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

[bwplotka]

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

[fatsheep9146]

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.

[bwplotka]

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]

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]

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

[fatsheep9146]

I convert the compare logic into one common function compareNativeExemplarValues

[ArthurSens]

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 :)

[fatsheep9146]

Done

[beorn7]

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.

[fatsheep9146]

Done

[beorn7]

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.

[beorn7]

Great. I'll merge on green.