New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add native histogram exemplar support #1471
base: main
Are you sure you want to change the base?
add native histogram exemplar support #1471
Conversation
63156a1
to
5dcfaff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a native histogram expert here, just some small comments :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have this. Thanks for the contribution 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@beorn7 The pr is ready for review again. |
Thank you very much. I'm on vacation this week, but this is high on my list once I'm back in action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool.
I think all that remains is wordsmithing and some formatting tweaks. See all my pedantic comments. :)
@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. |
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. |
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>
Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
6623198
to
85f5475
Compare
@beorn7 I only kept two commits, one for first implementation, the other for reconstitution that you suggested in #1471 (comment) |
fix #1126