Skip to content

Commit

Permalink
Corrects the behaviour of binary opperators between histogram and flo…
Browse files Browse the repository at this point in the history 
…at (prometheus#14726)

promql: corrects binary operators functioning for mixed sample with histogram and float

For invalid pairings of sample types, an annotation is added now.

Signed-off-by: Neeraj Gartia <[email protected]>

---------

Signed-off-by: Neeraj Gartia <[email protected]>
  • Loading branch information
@NeerajGartia21
NeerajGartia21 authored Oct 15, 2024
1 parent 1e1f6ab commit d4b1f9e
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 22 deletions.
8 changes: 8 additions & 0 deletions model/histogram/float_histogram.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,14 @@ func (h *FloatHistogram) Div(scalar float64) *FloatHistogram {
h.ZeroCount /= scalar
h.Count /= scalar
h.Sum /= scalar
// Division by zero removes all buckets.
if scalar == 0 {
h.PositiveBuckets = nil
h.NegativeBuckets = nil
h.PositiveSpans = nil
h.NegativeSpans = nil
return h
}
for i := range h.PositiveBuckets {
h.PositiveBuckets[i] /= scalar
}
Expand Down
12 changes: 4 additions & 8 deletions model/histogram/float_histogram_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,14 +399,10 @@ func TestFloatHistogramDiv(t *testing.T) {
},
0,
&FloatHistogram{
ZeroThreshold: 0.01,
ZeroCount: math.Inf(1),
Count: math.Inf(1),
Sum: math.Inf(1),
PositiveSpans: []Span{{-2, 1}, {2, 3}},
PositiveBuckets: []float64{math.Inf(1), math.Inf(1), math.Inf(1), math.Inf(1)},
NegativeSpans: []Span{{3, 2}, {3, 2}},
NegativeBuckets: []float64{math.Inf(1), math.Inf(1), math.Inf(1), math.Inf(1)},
ZeroThreshold: 0.01,
Count: math.Inf(1),
Sum: math.Inf(1),
ZeroCount: math.Inf(1),
},
},
{
Expand Down
45 changes: 35 additions & 10 deletions promql/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"github.com/prometheus/prometheus/model/timestamp"
"github.com/prometheus/prometheus/model/value"
"github.com/prometheus/prometheus/promql/parser"
"github.com/prometheus/prometheus/promql/parser/posrange"
"github.com/prometheus/prometheus/storage"
"github.com/prometheus/prometheus/tsdb/chunkenc"
"github.com/prometheus/prometheus/util/annotations"
Expand Down Expand Up @@ -1929,20 +1930,20 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value,
}, e.LHS, e.RHS)
default:
return ev.rangeEval(ctx, initSignatures, func(v []parser.Value, sh [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, annotations.Annotations) {
vec, err := ev.VectorBinop(e.Op, v[0].(Vector), v[1].(Vector), e.VectorMatching, e.ReturnBool, sh[0], sh[1], enh)
vec, err := ev.VectorBinop(e.Op, v[0].(Vector), v[1].(Vector), e.VectorMatching, e.ReturnBool, sh[0], sh[1], enh, e.PositionRange())
return vec, handleVectorBinopError(err, e)
}, e.LHS, e.RHS)
}

case lt == parser.ValueTypeVector && rt == parser.ValueTypeScalar:
return ev.rangeEval(ctx, nil, func(v []parser.Value, _ [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, annotations.Annotations) {
vec, err := ev.VectorscalarBinop(e.Op, v[0].(Vector), Scalar{V: v[1].(Vector)[0].F}, false, e.ReturnBool, enh)
vec, err := ev.VectorscalarBinop(e.Op, v[0].(Vector), Scalar{V: v[1].(Vector)[0].F}, false, e.ReturnBool, enh, e.PositionRange())
return vec, handleVectorBinopError(err, e)
}, e.LHS, e.RHS)

case lt == parser.ValueTypeScalar && rt == parser.ValueTypeVector:
return ev.rangeEval(ctx, nil, func(v []parser.Value, _ [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, annotations.Annotations) {
vec, err := ev.VectorscalarBinop(e.Op, v[1].(Vector), Scalar{V: v[0].(Vector)[0].F}, true, e.ReturnBool, enh)
vec, err := ev.VectorscalarBinop(e.Op, v[1].(Vector), Scalar{V: v[0].(Vector)[0].F}, true, e.ReturnBool, enh, e.PositionRange())
return vec, handleVectorBinopError(err, e)
}, e.LHS, e.RHS)
}
Expand Down Expand Up @@ -2534,7 +2535,7 @@ func (ev *evaluator) VectorUnless(lhs, rhs Vector, matching *parser.VectorMatchi
}

// VectorBinop evaluates a binary operation between two Vectors, excluding set operators.
func (ev *evaluator) VectorBinop(op parser.ItemType, lhs, rhs Vector, matching *parser.VectorMatching, returnBool bool, lhsh, rhsh []EvalSeriesHelper, enh *EvalNodeHelper) (Vector, error) {
func (ev *evaluator) VectorBinop(op parser.ItemType, lhs, rhs Vector, matching *parser.VectorMatching, returnBool bool, lhsh, rhsh []EvalSeriesHelper, enh *EvalNodeHelper, pos posrange.PositionRange) (Vector, error) {
if matching.Card == parser.CardManyToMany {
panic("many-to-many only allowed for set operators")
}
Expand Down Expand Up @@ -2608,7 +2609,7 @@ func (ev *evaluator) VectorBinop(op parser.ItemType, lhs, rhs Vector, matching *
fl, fr = fr, fl
hl, hr = hr, hl
}
floatValue, histogramValue, keep, err := vectorElemBinop(op, fl, fr, hl, hr)
floatValue, histogramValue, keep, err := vectorElemBinop(op, fl, fr, hl, hr, pos)
if err != nil {
lastErr = err
}
Expand Down Expand Up @@ -2717,7 +2718,7 @@ func resultMetric(lhs, rhs labels.Labels, op parser.ItemType, matching *parser.V
}

// VectorscalarBinop evaluates a binary operation between a Vector and a Scalar.
func (ev *evaluator) VectorscalarBinop(op parser.ItemType, lhs Vector, rhs Scalar, swap, returnBool bool, enh *EvalNodeHelper) (Vector, error) {
func (ev *evaluator) VectorscalarBinop(op parser.ItemType, lhs Vector, rhs Scalar, swap, returnBool bool, enh *EvalNodeHelper, pos posrange.PositionRange) (Vector, error) {
var lastErr error
for _, lhsSample := range lhs {
lf, rf := lhsSample.F, rhs.V
Expand All @@ -2729,7 +2730,7 @@ func (ev *evaluator) VectorscalarBinop(op parser.ItemType, lhs Vector, rhs Scala
lf, rf = rf, lf
lh, rh = rh, lh
}
float, histogram, keep, err := vectorElemBinop(op, lf, rf, lh, rh)
float, histogram, keep, err := vectorElemBinop(op, lf, rf, lh, rh, pos)
if err != nil {
lastErr = err
}
Expand Down Expand Up @@ -2796,7 +2797,7 @@ func scalarBinop(op parser.ItemType, lhs, rhs float64) float64 {
}

// vectorElemBinop evaluates a binary operation between two Vector elements.
func vectorElemBinop(op parser.ItemType, lhs, rhs float64, hlhs, hrhs *histogram.FloatHistogram) (float64, *histogram.FloatHistogram, bool, error) {
func vectorElemBinop(op parser.ItemType, lhs, rhs float64, hlhs, hrhs *histogram.FloatHistogram, pos posrange.PositionRange) (float64, *histogram.FloatHistogram, bool, error) {
switch op {
case parser.ADD:
if hlhs != nil && hrhs != nil {
Expand All @@ -2806,7 +2807,13 @@ func vectorElemBinop(op parser.ItemType, lhs, rhs float64, hlhs, hrhs *histogram
}
return 0, res.Compact(0), true, nil
}
return lhs + rhs, nil, true, nil
if hlhs == nil && hrhs == nil {
return lhs + rhs, nil, true, nil
}
if hlhs != nil {
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("histogram", "+", "float", pos)
}
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("float", "+", "histogram", pos)
case parser.SUB:
if hlhs != nil && hrhs != nil {
res, err := hlhs.Copy().Sub(hrhs)
Expand All @@ -2815,19 +2822,34 @@ func vectorElemBinop(op parser.ItemType, lhs, rhs float64, hlhs, hrhs *histogram
}
return 0, res.Compact(0), true, nil
}
return lhs - rhs, nil, true, nil
if hlhs == nil && hrhs == nil {
return lhs - rhs, nil, true, nil
}
if hlhs != nil {
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("histogram", "-", "float", pos)
}
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("float", "-", "histogram", pos)
case parser.MUL:
if hlhs != nil && hrhs == nil {
return 0, hlhs.Copy().Mul(rhs), true, nil
}
if hlhs == nil && hrhs != nil {
return 0, hrhs.Copy().Mul(lhs), true, nil
}
if hlhs != nil && hrhs != nil {
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("histogram", "*", "histogram", pos)
}
return lhs * rhs, nil, true, nil
case parser.DIV:
if hlhs != nil && hrhs == nil {
return 0, hlhs.Copy().Div(rhs), true, nil
}
if hrhs != nil {
if hlhs != nil {
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("histogram", "/", "histogram", pos)
}
return 0, nil, false, annotations.NewIncompatibleTypesInBinOpInfo("float", "/", "histogram", pos)
}
return lhs / rhs, nil, true, nil
case parser.POW:
return math.Pow(lhs, rhs), nil, true, nil
Expand Down Expand Up @@ -3365,6 +3387,9 @@ func handleVectorBinopError(err error, e *parser.BinaryExpr) annotations.Annotat
}
metricName := ""
pos := e.PositionRange()
if errors.Is(err, annotations.PromQLInfo) || errors.Is(err, annotations.PromQLWarning) {
return annotations.New().Add(err)
}
if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) {
return annotations.New().Add(annotations.NewMixedExponentialCustomHistogramsWarning(metricName, pos))
} else if errors.Is(err, histogram.ErrHistogramsIncompatibleBounds) {
Expand Down
30 changes: 26 additions & 4 deletions promql/promqltest/testdata/native_histograms.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,18 +948,40 @@ eval instant at 10m histogram_mul_div*float_series_0
eval instant at 10m float_series_0*histogram_mul_div
{} {{schema:0 count:0 sum:0 z_bucket:0 z_bucket_w:0.001 buckets:[0 0 0] n_buckets:[0 0 0]}}

# TODO: (NeerajGartia21) remove all the histogram buckets in case of division with zero. See: https://github.com/prometheus/prometheus/issues/13934
eval instant at 10m histogram_mul_div/0
{} {{schema:0 count:Inf sum:Inf z_bucket:Inf z_bucket_w:0.001 buckets:[Inf Inf Inf] n_buckets:[Inf Inf Inf]}}
{} {{schema:0 count:Inf sum:Inf z_bucket_w:0.001 z_bucket:Inf}}

eval instant at 10m histogram_mul_div/float_series_0
{} {{schema:0 count:Inf sum:Inf z_bucket:Inf z_bucket_w:0.001 buckets:[Inf Inf Inf] n_buckets:[Inf Inf Inf]}}
{} {{schema:0 count:Inf sum:Inf z_bucket_w:0.001 z_bucket:Inf}}

eval instant at 10m histogram_mul_div*0/0
{} {{schema:0 count:NaN sum:NaN z_bucket:NaN z_bucket_w:0.001 buckets:[NaN NaN NaN] n_buckets:[NaN NaN NaN]}}
{} {{schema:0 count:NaN sum:NaN z_bucket_w:0.001 z_bucket:NaN}}

eval_info instant at 10m histogram_mul_div*histogram_mul_div

eval_info instant at 10m histogram_mul_div/histogram_mul_div

eval_info instant at 10m float_series_3/histogram_mul_div

eval_info instant at 10m 0/histogram_mul_div

clear

# Apply binary operators to mixed histogram and float samples.
# TODO:(NeerajGartia21) move these tests to their respective locations when tests from engine_test.go are be moved here.

load 10m
histogram_sample {{schema:0 count:24 sum:100 z_bucket:4 z_bucket_w:0.001 buckets:[2 3 0 1 4] n_buckets:[2 3 0 1 4]}}x1
float_sample 0x1

eval_info instant at 10m float_sample+histogram_sample

eval_info instant at 10m histogram_sample+float_sample

eval_info instant at 10m float_sample-histogram_sample

eval_info instant at 10m histogram_sample-float_sample

# Counter reset only noticeable in a single bucket.
load 5m
reset_in_bucket {{schema:0 count:4 sum:5 buckets:[1 2 1]}} {{schema:0 count:5 sum:6 buckets:[1 1 3]}} {{schema:0 count:6 sum:7 buckets:[1 2 3]}}
Expand Down
10 changes: 10 additions & 0 deletions util/annotations/annotations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ var (

PossibleNonCounterInfo = fmt.Errorf("%w: metric might not be a counter, name does not end in _total/_sum/_count/_bucket:", PromQLInfo)
HistogramQuantileForcedMonotonicityInfo = fmt.Errorf("%w: input to histogram_quantile needed to be fixed for monotonicity (see https://prometheus.io/docs/prometheus/latest/querying/functions/#histogram_quantile) for metric name", PromQLInfo)
IncompatibleTypesInBinOpInfo = fmt.Errorf("%w: incompatible sample types encountered for binary operator", PromQLInfo)
)

type annoErr struct {
Expand Down Expand Up @@ -273,3 +274,12 @@ func NewHistogramQuantileForcedMonotonicityInfo(metricName string, pos posrange.
Err: fmt.Errorf("%w %q", HistogramQuantileForcedMonotonicityInfo, metricName),
}
}

// NewIncompatibleTypesInBinOpInfo is used if binary operators act on a
// combination of types that doesn't work and therefore returns no result.
func NewIncompatibleTypesInBinOpInfo(lhsType, operator, rhsType string, pos posrange.PositionRange) error {
return annoErr{
PositionRange: pos,
Err: fmt.Errorf("%w %q: %s %s %s", IncompatibleTypesInBinOpInfo, operator, lhsType, operator, rhsType),
}
}

0 comments on commit d4b1f9e

Please sign in to comment.