Reject histogram buckets with duplicate le label float values - #346
Conversation
Duplicate le strings that parse to the same float64 (e.g. 0.005 and 0.00500000000000000006) produce invalid GCM distributions. Detect them at parse time and during build with clearer errors that include the metric name and le labels.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation to detect and reject duplicate bucket boundaries in Prometheus histograms, specifically addressing cases where different 'le' label strings parse to the same float64 value. It tracks the original 'le' labels and provides descriptive error messages. The review feedback highlights two critical issues: first, returning an error immediately when a duplicate is found during sample processing causes a stream desynchronization bug by leaving remaining samples of the same histogram family unconsumed; second, 'NaN' boundaries can bypass duplicate checks and violate ordering requirements, so they should be explicitly rejected early.
| for i, existing := range dist.bounds { | ||
| if existing == bound { | ||
| prometheusSamplesDiscarded.WithLabelValues("duplicate-bucket-boundary").Add(float64(dist.inputSampleCount())) | ||
| discardExemplarIncIfExists(storage.SeriesRef(s.Ref), exemplars, "duplicate-bucket-boundary") | ||
| lePrev := "" | ||
| if len(dist.leLabels) == len(dist.bounds) { | ||
| lePrev = dist.leLabels[i] | ||
| } | ||
| return nil, samples[consumed:], duplicateBucketBoundaryError(metric, lePrev, leLabel, bound, e.lset) | ||
| } | ||
| } |
There was a problem hiding this comment.
Returning an error immediately inside the loop in buildDistributions causes a stream desynchronization bug. Any remaining samples belonging to the same histogram family will not be consumed, leaving them in the returned samples[consumed:] slice. If the caller continues processing the remainder, it will attempt to process those remaining samples as a new, incomplete histogram, leading to corrupt metrics or further errors.
To fix this, we should consume all remaining samples of the same histogram family before returning the error.
for i, existing := range dist.bounds {
if existing == bound {
prometheusSamplesDiscarded.WithLabelValues("duplicate-bucket-boundary").Add(float64(dist.inputSampleCount()))
discardExemplarIncIfExists(storage.SeriesRef(s.Ref), exemplars, "duplicate-bucket-boundary")
lePrev := ""
if len(dist.leLabels) == len(dist.bounds) {
lePrev = dist.leLabels[i]
}
for _, remainingSample := range samples[consumed:] {
eRem, ok := b.series.get(remainingSample, externalLabels, metadata)
if !ok {
consumed++
continue
}
nameRem := eRem.lset.Get(labels.MetricName)
if !isHistogramSeries(metric, nameRem) {
break
}
consumed++
}
return nil, samples[consumed:], duplicateBucketBoundaryError(metric, lePrev, leLabel, bound, e.lset)
}
}| bound, err := strconv.ParseFloat(leLabel, 64) | ||
| if err != nil { |
There was a problem hiding this comment.
If a metric has a bucket label le="NaN", strconv.ParseFloat will successfully parse it as math.NaN(). This NaN boundary will bypass the duplicate checks (since NaN == NaN is false) and will be appended to dist.bounds. This violates the strict weak ordering required by sort.Sort and will eventually cause the Google Cloud Monitoring API to reject the entire write request.
We should explicitly reject NaN boundaries early as malformed bucket labels.
| bound, err := strconv.ParseFloat(leLabel, 64) | |
| if err != nil { | |
| bound, err := strconv.ParseFloat(leLabel, 64) | |
| if err != nil || math.IsNaN(bound) { |
1 participant
Summary
lelabel strings parse to the samefloat64(e.g."0.005"and"0.00500000000000000006), not only when bounds are strictly decreasinglelabel valuesFixes GoogleCloudPlatform/prometheus-engine#982
Test plan
go test ./google/export/ -count=10.005/0.00500000000000000006duplicate labels