mirror of https://github.com/prometheus/prometheus
Fix issue where `sum` over mixed exponential and custom buckets, or incompatible custom buckets, produces incorrect results
Signed-off-by: Charles Korn <charles.korn@grafana.com>pull/14611/head
parent
ee5bba07c0
commit
5ee94f49a2
|
@ -2784,6 +2784,7 @@ type groupedAggregation struct {
|
||||||
seen bool // Was this output groups seen in the input at this timestamp.
|
seen bool // Was this output groups seen in the input at this timestamp.
|
||||||
hasFloat bool // Has at least 1 float64 sample aggregated.
|
hasFloat bool // Has at least 1 float64 sample aggregated.
|
||||||
hasHistogram bool // Has at least 1 histogram sample aggregated.
|
hasHistogram bool // Has at least 1 histogram sample aggregated.
|
||||||
|
abandonHistogram bool // If true, group has seen mixed exponential and custom buckets, or incompatible custom buckets.
|
||||||
groupAggrComplete bool // Used by LIMITK to short-cut series loop when we've reached K elem on every group.
|
groupAggrComplete bool // Used by LIMITK to short-cut series loop when we've reached K elem on every group.
|
||||||
incrementalMean bool // True after reverting to incremental calculation of the mean value.
|
incrementalMean bool // True after reverting to incremental calculation of the mean value.
|
||||||
}
|
}
|
||||||
|
@ -2809,10 +2810,11 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
|
||||||
// Initialize this group if it's the first time we've seen it.
|
// Initialize this group if it's the first time we've seen it.
|
||||||
if !group.seen {
|
if !group.seen {
|
||||||
*group = groupedAggregation{
|
*group = groupedAggregation{
|
||||||
seen: true,
|
seen: true,
|
||||||
floatValue: f,
|
floatValue: f,
|
||||||
floatMean: f,
|
floatMean: f,
|
||||||
groupCount: 1,
|
abandonHistogram: false,
|
||||||
|
groupCount: 1,
|
||||||
}
|
}
|
||||||
switch op {
|
switch op {
|
||||||
case parser.AVG, parser.SUM:
|
case parser.AVG, parser.SUM:
|
||||||
|
@ -2833,6 +2835,10 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if group.abandonHistogram {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
switch op {
|
switch op {
|
||||||
case parser.SUM:
|
case parser.SUM:
|
||||||
if h != nil {
|
if h != nil {
|
||||||
|
@ -2841,6 +2847,7 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
|
||||||
_, err := group.histogramValue.Add(h)
|
_, err := group.histogramValue.Add(h)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos)
|
handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos)
|
||||||
|
group.abandonHistogram = true
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Otherwise the aggregation contained floats
|
// Otherwise the aggregation contained floats
|
||||||
|
@ -2987,7 +2994,9 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
|
||||||
annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(e.Expr.PositionRange()))
|
annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(e.Expr.PositionRange()))
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
if aggr.hasHistogram {
|
if aggr.abandonHistogram {
|
||||||
|
continue
|
||||||
|
} else if aggr.hasHistogram {
|
||||||
aggr.histogramValue.Compact(0)
|
aggr.histogramValue.Compact(0)
|
||||||
} else {
|
} else {
|
||||||
aggr.floatValue += aggr.floatKahanC
|
aggr.floatValue += aggr.floatKahanC
|
||||||
|
|
|
@ -762,3 +762,30 @@ eval_warn instant at 30s rate(some_metric[30s])
|
||||||
# Test the case where we have more than two points for rate
|
# Test the case where we have more than two points for rate
|
||||||
eval_warn instant at 1m rate(some_metric[1m])
|
eval_warn instant at 1m rate(some_metric[1m])
|
||||||
{} {{count:0.03333333333333333 sum:0.03333333333333333 buckets:[0.03333333333333333]}}
|
{} {{count:0.03333333333333333 sum:0.03333333333333333 buckets:[0.03333333333333333]}}
|
||||||
|
|
||||||
|
# Test mixing exponential and custom buckets.
|
||||||
|
load 6m
|
||||||
|
metric{series="exponential"} {{sum:4 count:3 buckets:[1 2 1]}} _ {{sum:4 count:3 buckets:[1 2 1]}}
|
||||||
|
metric{series="other-exponential"} {{sum:3 count:2 buckets:[1 1 1]}} _ {{sum:3 count:2 buckets:[1 1 1]}}
|
||||||
|
metric{series="custom"} _ {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
|
||||||
|
metric{series="other-custom"} _ {{schema:-53 sum:15 count:2 custom_values:[5 10] buckets:[0 2]}} {{schema:-53 sum:15 count:2 custom_values:[5 10] buckets:[0 2]}}
|
||||||
|
|
||||||
|
# T=0: only exponential
|
||||||
|
# T=6: only custom
|
||||||
|
# T=12: mixed, should be ignored and emit an warning
|
||||||
|
eval_warn range from 0 to 12m step 6m sum(metric)
|
||||||
|
{} {{sum:7 count:5 buckets:[2 3 2]}} {{schema:-53 sum:16 count:3 custom_values:[5 10] buckets:[1 2]}} _
|
||||||
|
|
||||||
|
clear
|
||||||
|
|
||||||
|
# Test incompatible custom bucket schemas.
|
||||||
|
load 6m
|
||||||
|
metric{series="1"} _ {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
|
||||||
|
metric{series="2"} {{schema:-53 sum:1 count:1 custom_values:[2] buckets:[1]}} _ {{schema:-53 sum:1 count:1 custom_values:[2] buckets:[1]}}
|
||||||
|
metric{series="3"} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
|
||||||
|
|
||||||
|
# T=0: incompatible, should be ignored and emit a warning
|
||||||
|
# T=6: compatible
|
||||||
|
# T=12: incompatible followed by compatible, should be ignored and emit a warning
|
||||||
|
eval_warn range from 0 to 12m step 6m sum(metric)
|
||||||
|
{} _ {{schema:-53 sum:2 count:2 custom_values:[5 10] buckets:[2]}} _
|
||||||
|
|
Loading…
Reference in New Issue