Add exemplars for native histograms #1686
Conversation
Remove unused variable fix tests Signed-off-by: Shivanth <[email protected]>
shivanthzen
force-pushed
the
nativehistogram_exemplars
branch
from
7569bb1 to
6a6a456
Compare
shivanthzen
requested review from
ArthurSens,
bwplotka,
kakkoyun and
vesari
as code owners
Signed-off-by: Shivanth <[email protected]>
|
@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… 😮💨 |
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.
| 10, 12.1, map[int]int64{1: 7, 2: 1, 3: 2}, map[int]int64{}, 0, 2, 0.2, time.Date( | ||
| 2009, 11, 17, 20, 34, 58, 651387237, time.UTC)) | ||
| m := &withExemplarsMetric{Metric: h, exemplars: []*dto.Exemplar{ | ||
| {Value: proto.Float64(2000.0), Timestamp: timestamppb.New(time.Date(2009, 11, 17, 20, 34, 58, 3243244, time.UTC))}, |
There was a problem hiding this comment.
We should also test the following cases:
- More than one exemplar.
- An exemplar without timestamp (to verify that a timestamp is added automatically then, because exemplars without timestamps are not allowed for native histograms).
- A histogram with both classic and native buckets. (I believe for that you might have to create an ad-hoc implementation of
Metricthat spits out a custom-madedto.Metric.)
Co-authored-by: Björn Rabenstein <[email protected]> Signed-off-by: Shivanth MP <[email protected]>
Co-authored-by: Björn Rabenstein <[email protected]> Signed-off-by: Shivanth MP <[email protected]>
|
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 <[email protected]>
Signed-off-by: Shivanth <[email protected]>
shivanthzen
force-pushed
the
nativehistogram_exemplars
branch
from
951bff7 to
7f9f76d
Compare
Signed-off-by: Shivanth <[email protected]>
|
@beorn7 Added a test to cover the case where exemplars are added to both buckets and exemplars. |
prometheus/metric_test.go
Outdated
| // This is a dummy Histogram which has both buckets and nativehistogram | ||
| // to test metrics with exemplars | ||
| func NewDummyConstNativeHistogram( |
There was a problem hiding this comment.
Let's not export this, so that nobody will use this for real. Also, how about a more descriptive name like "newNativeHistogramWithClassicBuckets"? And finally, end a sentence with a period. :)
| // This is a dummy Histogram which has both buckets and nativehistogram | |
| // to test metrics with exemplars | |
| func NewDummyConstNativeHistogram( | |
| // newNativeHistogramWithClassicBuckets returns a Metric representing | |
| // a native histogram that also has classic buckets. This is for testing purposes. | |
| func newNativeHistogramWithClassicBuckets( |
prometheus/metric.go
Outdated
| @@ -187,6 +187,15 @@ func (m *withExemplarsMetric) Write(pb *dto.Metric) error { | |||
| pb.Counter.Exemplar = m.exemplars[len(m.exemplars)-1] | |||
| case pb.Histogram != nil: | |||
| for _, e := range m.exemplars { | |||
| if pb.Histogram.Schema != nil { | |||
| if *pb.Histogram.Schema > math.MinInt32 && e.GetTimestamp() != nil { | |||
There was a problem hiding this comment.
While we use math.MinInt32 as a schema within the histogram struct to mark the absence of sparse buckets, we do not use the same marker in the protobuf. In the protobuf, we know it represents a native histogram if the zero threshold or the zero count or the number of negative spans or the number of positive spans are not zero. (If need be, we add a no-op span.)
So what we have to check is something like the following:
h := pb.Histogram
if (*h.ZeroThreshold != 0 || *h.ZeroCount != 0 || len(h.PositiveSpan) != 0 || len(h.NegativeSpan) != 0) && e.GetTimestamp() != nil {Signed-off-by: Shivanth <[email protected]>
Signed-off-by: Shivanth <[email protected]>
Signed-off-by: Shivanth <[email protected]>
Support exemplars in native histograms:
#1617