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.
| if *pb.Histogram.Schema > math.MinInt32 && e.GetTimestamp() != nil { | ||
| pb.Histogram.Exemplars = append(pb.Histogram.Exemplars, e) | ||
| } | ||
| continue |
There was a problem hiding this comment.
If a classic and a native histogram is exposed at the same time (for the scraper to pick), we should have the exemplars on both. Maybe the following works for that:
| continue | |
| if len(pb.Histogram.Bucket) == 0 { | |
| // Don't proceed to classic buckets if there are none. | |
| continue | |
| } |
We do have to check if there are already buckets defined, because otherwise there will be an Inf bucket created just for the histograms.
| 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.)
| @@ -227,6 +233,7 @@ type Exemplar struct { | |||
| // Only last applicable exemplar is injected from the list. | |||
| // For example for Counter it means last exemplar is injected. | |||
| // For Histogram, it means last applicable exemplar for each bucket is injected. | |||
| // Note that for a nativeHistogram, all valid exemplars are injected. | |||
There was a problem hiding this comment.
| // Note that for a nativeHistogram, all valid exemplars are injected. | |
| // For a Native Histogram, all valid exemplars are injected. |
Support exemplars in native histograms:
#1617