Skip to content

Add exemplars for native histograms #1686

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
May 6, 2025
Merged

Add exemplars for native histograms #1686

merged 17 commits into from
May 6, 2025
+283 −7

Conversation

shivanthzen
Copy link
Contributor

@shivanthzen shivanthzen commented Nov 17, 2024 •
edited
Loading

Support exemplars in native histograms:
#1617

@shivanthzen
Add exemplars for native histograms
6a6a456 
Remove unused variable

fix tests

Signed-off-by: Shivanth <[email protected]>
@shivanthzen shivanthzen force-pushed the nativehistogram_exemplars branch from 7569bb1 to 6a6a456 Compare January 4, 2025 20:34
@shivanthzen shivanthzen marked this pull request as ready for review January 4, 2025 20:36
@shivanthzen shivanthzen marked this pull request as draft January 6, 2025 12:38
@shivanthzen shivanthzen marked this pull request as ready for review January 14, 2025 14:45
@shivanthzen
Copy link
Contributor Author

shivanthzen commented Jan 14, 2025 •
edited
Loading

@ArthurSens @beorn7 Part-2 for constnativehistogram support

@bwplotka
Copy link
Member

Do you mind checking @beorn7 @ArthurSens ? I don't have context on this logic, not sure if this PR is what #1654 (comment) wanted.

@beorn7
Copy link
Member

beorn7 commented Jan 21, 2025

This has been on my review list for a while. It's just hard to catch up through all my backlog… 😮‍💨

beorn7
beorn7 requested changes Jan 21, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implements the intended idea. Thank you very much. However, I think we should also be able to handle a histogram with both native and classic buckets (even though we have no MustNewConst... constructor for it, but people could create it on their own using dto primitives). And we need to test that. See comments.

shivanthzen and others added 2 commits February 4, 2025 13:03
@shivanthzen @beorn7
Update prometheus/metric.go
f799a31 
Co-authored-by: Björn Rabenstein <[email protected]>
Signed-off-by: Shivanth MP <[email protected]>
@shivanthzen @beorn7
Update prometheus/metric.go
2ec9081 
Co-authored-by: Björn Rabenstein <[email protected]>
Signed-off-by: Shivanth MP <[email protected]>
@shivanthzen
Copy link
Contributor Author

Update: This PR is still being worked on. Not stale.

@beorn7
Copy link
Member

beorn7 commented Feb 6, 2025

@shivanthzen let us know when it is ready for the next round of review.

@shivanthzen shivanthzen force-pushed the nativehistogram_exemplars branch from 951bff7 to 7f9f76d Compare April 14, 2025 21:17
@shivanthzen
Remove printlns
ca2462c 
Signed-off-by: Shivanth <[email protected]>
@shivanthzen
Copy link
Contributor Author

@beorn7 Added a test to cover the case where exemplars are added to both buckets and exemplars.

beorn7
beorn7 requested changes Apr 15, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have just a naming nit and also just discovered that we need one small actual code change after all.

@shivanthzen shivanthzen requested a review from beorn7 April 17, 2025 16:28
@shivanthzen
Copy link
Contributor Author

@beorn7

beorn7
beorn7 requested changes Apr 29, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the review delay.

A small improvement, see comment.

@shivanthzen @beorn7
Update prometheus/metric.go
e2b5996 
Co-authored-by: Björn Rabenstein <[email protected]>
Signed-off-by: Shivanth MP <[email protected]>
beorn7
beorn7 requested changes Apr 30, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also resolve the merge conflicts? Sorry, there are no merge conflicts. I misread the new GH UI. If you don't mind, you could still rebase to make sure everything still works with the current state in main.

@shivanthzen
Copy link
Contributor Author

@beorn7

beorn7
beorn7 approved these changes May 6, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Thank you.

@beorn7 beorn7 merged commit 2db5bc8 into prometheus:main May 6, 2025
9 checks passed
@beorn7 beorn7 mentioned this pull request May 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 approved these changes

@ArthurSens ArthurSens Awaiting requested review from ArthurSens ArthurSens is a code owner

@bwplotka bwplotka Awaiting requested review from bwplotka bwplotka is a code owner

@kakkoyun kakkoyun Awaiting requested review from kakkoyun kakkoyun is a code owner

@vesari vesari Awaiting requested review from vesari vesari is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shivanthzen @bwplotka @beorn7