Skip to content

Reject histogram buckets with duplicate le label float values #346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sahilleth wants to merge 1 commit into
base: release-2.53.5-gmp
Choose a base branch
from
Open

Reject histogram buckets with duplicate le label float values #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 49 additions & 9 deletions google/export/transform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ type distribution struct {
timestamp int64
resetTimestamp int64
exemplars []record.RefExemplar
// Original le label values for each bucket boundary, parallel to bounds.
leLabels []string
// If all three are true, we can be sure to have observed all series for the
// distribution as buckets must be specified in ascending order.
hasSum, hasCount, hasInfBucket bool
Expand All @@ -238,6 +240,7 @@ func (d *distribution) reset() {
d.timestamp, d.resetTimestamp = 0, 0
d.skip = false
d.exemplars = d.exemplars[:0]
d.leLabels = d.leLabels[:0]
d.hash = 0
d.proto = nil
d.lset = labels.EmptyLabels()
Expand Down Expand Up @@ -269,6 +272,28 @@ func (d *distribution) Less(i, j int) bool {
func (d *distribution) Swap(i, j int) {
d.bounds[i], d.bounds[j] = d.bounds[j], d.bounds[i]
d.values[i], d.values[j] = d.values[j], d.values[i]
d.leLabels[i], d.leLabels[j] = d.leLabels[j], d.leLabels[i]
}

func histogramMetricName(lset labels.Labels) string {
name := lset.Get(labels.MetricName)
if name == "" {
return lset.String()
}
return name
}

func duplicateBucketBoundaryError(metric string, lePrev, leCur string, bound float64, lset labels.Labels) error {
if lePrev != "" && leCur != "" {
return fmt.Errorf(
"invalid histogram metric %s: duplicate bucket boundary from le labels %q and %q (both parse to %g): %s",
metric, lePrev, leCur, bound, lset,
)
}
return fmt.Errorf(
"invalid histogram metric %s: duplicate bucket boundary %g: %s",
metric, bound, lset,
)
}

func (d *distribution) build(lset labels.Labels) (*distribution_pb.Distribution, error) {
Expand Down Expand Up @@ -303,15 +328,17 @@ func (d *distribution) build(lset labels.Labels) (*distribution_pb.Distribution,
}

for i, bound := range d.bounds {
if i > 0 && prevBound == bound {
// Bounds has to be higher than the previous one.
// Rarely, but the same bounds can occur due to string to float imprecision
// or invalid representations of the same float e.g. 1 vs 1.0.
// GCM API rejects those, so reject them early.
if i > 0 && prevBound >= bound && !math.IsInf(bound, 1) {
// Bounds must be strictly increasing. Equal values can occur when different
// le label strings parse to the same float64 (e.g. "0.005" and
// "0.00500000000000000006"). GCM rejects those, so reject them early.
prometheusSamplesDiscarded.WithLabelValues("duplicate-bucket-boundary").Add(float64(d.inputSampleCount()))
err := fmt.Errorf("invalid histogram with duplicates bounds (le label value) %s: count=%f, sum=%f, dev=%f, index=%d, bucketBound=%f, bucketPrevBound=%f",
lset, d.count, d.sum, dev, i, bound, prevBound)
return nil, err
lePrev, leCur := "", ""
if len(d.leLabels) == len(d.bounds) {
lePrev = d.leLabels[i-1]
leCur = d.leLabels[i]
}
return nil, duplicateBucketBoundaryError(histogramMetricName(lset), lePrev, leCur, bound, lset)
}

if math.IsInf(bound, 1) {
Expand Down Expand Up @@ -460,7 +487,8 @@ Loop:
dist.resetTimestamp = rt

case metricSuffixBucket:
bound, err := strconv.ParseFloat(e.lset.Get(labels.BucketLabel), 64)
leLabel := e.lset.Get(labels.BucketLabel)
bound, err := strconv.ParseFloat(leLabel, 64)
if err != nil {
Comment on lines +491 to 492

@gemini-code-assist gemini-code-assist Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
bound, err := strconv.ParseFloat(leLabel, 64)
if err != nil {
bound, err := strconv.ParseFloat(leLabel, 64)
if err != nil || math.IsNaN(bound) {

prometheusSamplesDiscarded.WithLabelValues("malformed-bucket-le-label").Inc()
discardExemplarIncIfExists(storage.SeriesRef(s.Ref), exemplars, "malformed-bucket-le-label")
Expand All @@ -471,11 +499,23 @@ Loop:
discardExemplarIncIfExists(storage.SeriesRef(s.Ref), exemplars, "NaN-bucket-value")
continue
}
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)
}
}
Comment on lines +502 to +512

@gemini-code-assist gemini-code-assist Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)
                }
            }

// Handle cases where +Inf bucket is out-of-order by not overwriting on the last-consumed bucket.
if !dist.hasInfBucket {
dist.hasInfBucket = math.IsInf(bound, 1)
}
dist.bounds = append(dist.bounds, bound)
dist.leLabels = append(dist.leLabels, leLabel)
dist.values = append(dist.values, int64(v))
if exemplar, ok := exemplars[storage.SeriesRef(s.Ref)]; ok {
dist.exemplars = append(dist.exemplars, exemplar)
Expand Down
29 changes: 29 additions & 0 deletions google/export/transform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,35 @@ func TestSampleBuilder(t *testing.T) {
},
wantFailOnLastSample: true,
},
{
doc: "histogram with duplicate le labels that parse to the same float",
metadata: testMetadataFunc(metricMetadataMap{
"metric1": {Type: model.MetricTypeHistogram, Help: "metric1 help text"},
}),
series: seriesMap{
1: labels.FromStrings("job", "job1", "instance", "instance1", "__name__", "metric1_sum"),
2: labels.FromStrings("job", "job1", "instance", "instance1", "__name__", "metric1_count"),
3: labels.FromStrings("job", "job1", "instance", "instance1", "__name__", "metric1_bucket", "le", "0.005"),
4: labels.FromStrings("job", "job1", "instance", "instance1", "__name__", "metric1_bucket", "le", "0.00500000000000000006"),
5: labels.FromStrings("job", "job1", "instance", "instance1", "__name__", "metric1_bucket", "le", "+Inf"),
},
samples: [][]record.RefSample{
{
{Ref: 3, T: 1000, V: 1},
{Ref: 4, T: 1000, V: 1},
{Ref: 5, T: 1000, V: 10},
{Ref: 1, T: 1000, V: 8},
{Ref: 2, T: 1000, V: 10},
}, {
{Ref: 3, T: 2000, V: 2},
{Ref: 4, T: 2000, V: 2},
{Ref: 5, T: 2000, V: 11},
{Ref: 1, T: 2000, V: 9.5},
{Ref: 2, T: 2000, V: 11},
},
},
wantFailOnLastSample: true,
},
{
doc: "convert histogram with exemplars",
metadata: testMetadataFunc(metricMetadataMap{
Expand Down