Add NH count validation#7072
Conversation
3fd4fe7 to
940f1bd
Compare
| } | ||
|
|
||
| // copy from https://github.com/prometheus/prometheus/blob/v3.6.0/model/histogram/generic.go#L399-L420 | ||
| func checkHistogramBuckets[BC histogram.BucketCount, IBC histogram.InternalBucketCount](buckets []IBC, count *BC, deltas bool) (float64, error) { |
There was a problem hiding this comment.
Why we have to maintain this function and validation ourselves instead of reusing the validation function in Prometheus https://github.com/prometheus/prometheus/blob/main/model/histogram/histogram.go#L425C21-L425C29?
There was a problem hiding this comment.
Yeah we could utilize it. If we do that, it seems like we should return a single error type for these different causes.
There was a problem hiding this comment.
I've updated to use Prometheus's Validate function and moved the check for invalid NH before the limit check to ensure we return a 400 error for them.
Signed-off-by: SungJin1212 <[email protected]>
Signed-off-by: SungJin1212 <[email protected]>
Signed-off-by: SungJin1212 <[email protected]>
940f1bd to
2602c78
Compare
Signed-off-by: SungJin1212 <[email protected]>
Signed-off-by: SungJin1212 <[email protected]>
|
|
||
| if histogramSample.IsFloatHistogram() { | ||
| // Initial check to see if the bucket limit is exceeded or not. If not, we can avoid type casting. | ||
| fh := cortexpb.FloatHistogramProtoToFloatHistogram(histogramSample) |
There was a problem hiding this comment.
For next PR, consider adding sync pool here to reduce allocation.
2 participants
This PR adds count validation for NH in Distributor side.
Which issue(s) this PR fixes:
Fixes #7042
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]