Add exemplars for native histograms#1686
Conversation
Remove unused variable fix tests Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
7569bb1 to
6a6a456
Compare
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
|
@ArthurSens @beorn7 Part-2 for constnativehistogram support |
|
Do you mind checking @beorn7 @ArthurSens ? I don't have context on this logic, not sure if this PR is what #1654 (comment) wanted. |
|
This has been on my review list for a while. It's just hard to catch up through all my backlog… 😮💨 |
beorn7
left a comment
There was a problem hiding this comment.
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.
Co-authored-by: Björn Rabenstein <github@rabenste.in> Signed-off-by: Shivanth MP <shivanth.metchem@deliveryhero.com>
Co-authored-by: Björn Rabenstein <github@rabenste.in> Signed-off-by: Shivanth MP <shivanth.metchem@deliveryhero.com>
|
Update: This PR is still being worked on. Not stale. |
|
@shivanthzen let us know when it is ready for the next round of review. |
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
951bff7 to
7f9f76d
Compare
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
|
@beorn7 Added a test to cover the case where exemplars are added to both buckets and exemplars. |
beorn7
left a comment
There was a problem hiding this comment.
Thank you. I have just a naming nit and also just discovered that we need one small actual code change after all.
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
Signed-off-by: Shivanth <shivanth.metchem@deliveryhero.com>
Support exemplars in native histograms:
#1617