Skip to content

Commit

Permalink
Change behaviour to mirror prometheus/prometheus#15245
Browse files Browse the repository at this point in the history 
Signed-off-by: Arve Knudsen <[email protected]>
  • Loading branch information
@charleskorn @aknuds1
charleskorn authored and aknuds1 committed Nov 19, 2024
1 parent e86fb40 commit fe18ce2
Show file tree
Hide file tree
Showing 8 changed files with 402 additions and 247 deletions.
257 changes: 149 additions & 108 deletions pkg/streamingpromql/engine_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1899,6 +1899,46 @@ func TestAnnotations(t *testing.T) {
expr: `sum(metric{type="histogram"})`,
},

"stdvar() with only floats": {
data: mixedFloatHistogramData,
expr: `stdvar(metric{type="float"})`,
},
"stdvar() with only native histograms": {
data: mixedFloatHistogramData,
expr: `stdvar(metric{type="histogram"})`,
expectedInfoAnnotations: []string{"PromQL info: ignored histogram in stdvar aggregation (1:8)"},
},

"stddev() with only floats": {
data: mixedFloatHistogramData,
expr: `stddev(metric{type="float"})`,
},
"stddev() with only native histograms": {
data: mixedFloatHistogramData,
expr: `stddev(metric{type="histogram"})`,
expectedInfoAnnotations: []string{"PromQL info: ignored histogram in stddev aggregation (1:8)"},
},

"min() with only floats": {
data: mixedFloatHistogramData,
expr: `min(metric{type="float"})`,
},
"min() with only native histograms": {
data: mixedFloatHistogramData,
expr: `min(metric{type="histogram"})`,
expectedInfoAnnotations: []string{"PromQL info: ignored histogram in min aggregation (1:5)"},
},

"max() with only floats": {
data: mixedFloatHistogramData,
expr: `max(metric{type="float"})`,
},
"max() with only native histograms": {
data: mixedFloatHistogramData,
expr: `max(metric{type="histogram"})`,
expectedInfoAnnotations: []string{"PromQL info: ignored histogram in max aggregation (1:5)"},
},

"avg() with float and native histogram at same step": {
data: mixedFloatHistogramData,
expr: "avg by (series) (metric)",
Expand Down Expand Up @@ -1986,114 +2026,6 @@ func TestAnnotations(t *testing.T) {
`PromQL warning: vector contains histograms with incompatible custom buckets for metric name "" (1:1)`,
},
},
"binary addition between two floats": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} + ignoring(type) metric{type="float"}`,
},
"binary subtraction between two floats": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} - ignoring(type) metric{type="float"}`,
},
"binary multiplication between two floats": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} * ignoring(type) metric{type="float"}`,
},
"binary division between two floats": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} / ignoring(type) metric{type="float"}`,
},
"binary addition between two histograms": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} + ignoring(type) metric{type="histogram"}`,
},
"binary subtraction between two histograms": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} - ignoring(type) metric{type="histogram"}`,
},
"binary multiplication between two histograms": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} * ignoring(type) metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "*": histogram * histogram (1:1)`},
},
"binary division between two histograms": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} / ignoring(type) metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "/": histogram / histogram (1:1)`},
},
"binary addition between a float on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} + ignoring(type) metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "+": float + histogram (1:1)`},
},
"binary subtraction between a float on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} - ignoring(type) metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "-": float - histogram (1:1)`},
},
"binary multiplication between a float on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} * ignoring(type) metric{type="histogram"}`,
},
"binary division between a float on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="float"} / ignoring(type) metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "/": float / histogram (1:1)`},
},
"binary addition between a histogram on the left side and a float on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} + ignoring(type) metric{type="float"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "+": histogram + float (1:1)`},
},
"binary subtraction between a histogram on the left side and a float on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} - ignoring(type) metric{type="float"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "-": histogram - float (1:1)`},
},
"binary multiplication between a histogram on the left side and a float on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} * ignoring(type) metric{type="float"}`,
},
"binary division between a histogram on the left side and a float on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} / ignoring(type) metric{type="float"}`,
},
"binary addition between a scalar on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `2 + metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "+": float + histogram (1:1)`},
},
"binary subtraction between a scalar on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `2 - metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "-": float - histogram (1:1)`},
},
"binary multiplication between a scalar on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `2 * metric{type="histogram"}`,
},
"binary division between a scalar on the left side and a histogram on the right": {
data: mixedFloatHistogramData,
expr: `2 / metric{type="histogram"}`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "/": float / histogram (1:1)`},
},
"binary addition between a histogram on the left side and a scalar on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} + 2`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "+": histogram + float (1:1)`},
},
"binary subtraction between a histogram on the left side and a scalar on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} - 2`,
expectedInfoAnnotations: []string{`PromQL info: incompatible sample types encountered for binary operator "-": histogram - float (1:1)`},
},
"binary multiplication between a histogram on the left side and a scalar on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} * 2`,
},
"binary division between a histogram on the left side and a scalar on the right": {
data: mixedFloatHistogramData,
expr: `metric{type="histogram"} / 2`,
},

"multiple annotations from different operators": {
data: `
Expand Down Expand Up @@ -2212,6 +2144,115 @@ func TestAnnotations(t *testing.T) {
}
}

binaryOperations := map[string]struct {
floatHistogramSupported bool
histogramFloatSupported bool
histogramHistogramSupported bool
supportsBool bool
}{
"+": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: true,
},
"-": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: true,
},
"*": {
floatHistogramSupported: true,
histogramFloatSupported: true,
histogramHistogramSupported: false,
},
"/": {
floatHistogramSupported: false,
histogramFloatSupported: true,
histogramHistogramSupported: false,
},
"^": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
},
"%": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
},
"atan2": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
},
"==": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: true,
supportsBool: true,
},
"!=": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: true,
supportsBool: true,
},
">": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
supportsBool: true,
},
"<": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
supportsBool: true,
},
">=": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
supportsBool: true,
},
"<=": {
floatHistogramSupported: false,
histogramFloatSupported: false,
histogramHistogramSupported: false,
supportsBool: true,
},
}

addBinopTestCase := func(op string, name string, expr string, left string, right string, supported bool) {
testCase := testCase{
data: mixedFloatHistogramData,
expr: expr,
}

if !supported {
testCase.expectedInfoAnnotations = []string{fmt.Sprintf(`PromQL info: incompatible sample types encountered for binary operator "%v": %v %v %v (1:1)`, op, left, op, right)}
}

testCases[name] = testCase
}

for op, binop := range binaryOperations {
expressions := []string{op}

if binop.supportsBool {
expressions = append(expressions, op+" bool")
}

for _, expr := range expressions {
addBinopTestCase(op, fmt.Sprintf("binary %v between two floats", expr), fmt.Sprintf(`metric{type="float"} %v ignoring(type) metric{type="float"}`, expr), "float", "float", true)
addBinopTestCase(op, fmt.Sprintf("binary %v between a float on the left side and a histogram on the right", expr), fmt.Sprintf(`metric{type="float"} %v ignoring(type) metric{type="histogram"}`, expr), "float", "histogram", binop.floatHistogramSupported)
addBinopTestCase(op, fmt.Sprintf("binary %v between a scalar on the left side and a histogram on the right", expr), fmt.Sprintf(`2 %v metric{type="histogram"}`, expr), "float", "histogram", binop.floatHistogramSupported)
addBinopTestCase(op, fmt.Sprintf("binary %v between a histogram on the left side and a float on the right", expr), fmt.Sprintf(`metric{type="histogram"} %v ignoring(type) metric{type="float"}`, expr), "histogram", "float", binop.histogramFloatSupported)
addBinopTestCase(op, fmt.Sprintf("binary %v between a histogram on the left side and a scalar on the right", expr), fmt.Sprintf(`metric{type="histogram"} %v 2`, expr), "histogram", "float", binop.histogramFloatSupported)
addBinopTestCase(op, fmt.Sprintf("binary %v between two histograms", expr), fmt.Sprintf(`metric{type="histogram"} %v ignoring(type) metric{type="histogram"}`, expr), "histogram", "histogram", binop.histogramHistogramSupported)
}
}

opts := NewTestEngineOpts()
mimirEngine, err := NewEngine(opts, NewStaticQueryLimitsProvider(0), stats.NewQueryMetrics(nil), log.NewNopLogger())
require.NoError(t, err)
Expand Down
30 changes: 18 additions & 12 deletions pkg/streamingpromql/operators/aggregations/min_max.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"math"

"github.com/prometheus/prometheus/promql"
"github.com/prometheus/prometheus/promql/parser/posrange"
"github.com/prometheus/prometheus/util/annotations"

"github.com/grafana/mimir/pkg/streamingpromql/limiting"
"github.com/grafana/mimir/pkg/streamingpromql/types"
Expand All @@ -19,11 +21,12 @@ type MinMaxAggregationGroup struct {
floatPresent []bool

accumulatePoint func(idx int64, f float64)
isMax bool
}

// max represents whether this aggregation is `max` (true), or `min` (false)
func NewMinMaxAggregationGroup(max bool) *MinMaxAggregationGroup {
g := &MinMaxAggregationGroup{}
g := &MinMaxAggregationGroup{isMax: max}
if max {
g.accumulatePoint = g.maxAccumulatePoint
} else {
Expand All @@ -48,11 +51,21 @@ func (g *MinMaxAggregationGroup) minAccumulatePoint(idx int64, f float64) {
}
}

func (g *MinMaxAggregationGroup) AccumulateSeries(data types.InstantVectorSeriesData, timeRange types.QueryTimeRange, memoryConsumptionTracker *limiting.MemoryConsumptionTracker, _ types.EmitAnnotationFunc) error {
if (len(data.Floats) > 0 || len(data.Histograms) > 0) && g.floatValues == nil {
// Even if we only have histograms, we have to populate the float slices, as we'll treat histograms as if they have value 0.
// This is consistent with Prometheus but may not be the desired value: https://github.com/prometheus/prometheus/issues/14711
func (g *MinMaxAggregationGroup) AccumulateSeries(data types.InstantVectorSeriesData, timeRange types.QueryTimeRange, memoryConsumptionTracker *limiting.MemoryConsumptionTracker, emitAnnotation types.EmitAnnotationFunc) error {
// Native histograms are ignored for min and max.
if len(data.Histograms) > 0 {
emitAnnotation(func(_ string, expressionPosition posrange.PositionRange) error {
name := "min"

if g.isMax {
name = "max"
}

return annotations.NewHistogramIgnoredInAggregationInfo(name, expressionPosition)
})
}

if len(data.Floats) > 0 && g.floatValues == nil {
var err error
// First series with float values for this group, populate it.
g.floatValues, err = types.Float64SlicePool.Get(timeRange.StepCount, memoryConsumptionTracker)
Expand All @@ -73,13 +86,6 @@ func (g *MinMaxAggregationGroup) AccumulateSeries(data types.InstantVectorSeries
g.accumulatePoint(idx, p.F)
}

// If a histogram exists max treats it as 0. We have to detect this here so that we return a 0 value instead of nothing.
// This is consistent with Prometheus but may not be the desired value: https://github.com/prometheus/prometheus/issues/14711
for _, p := range data.Histograms {
idx := timeRange.PointIndex(p.T)
g.accumulatePoint(idx, 0)
}

types.PutInstantVectorSeriesData(data, memoryConsumptionTracker)
return nil
}
Expand Down
19 changes: 16 additions & 3 deletions pkg/streamingpromql/operators/aggregations/stddev_stdvar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"math"

"github.com/prometheus/prometheus/promql"
"github.com/prometheus/prometheus/promql/parser/posrange"
"github.com/prometheus/prometheus/util/annotations"

"github.com/grafana/mimir/pkg/streamingpromql/limiting"
"github.com/grafana/mimir/pkg/streamingpromql/types"
Expand All @@ -33,12 +35,23 @@ type StddevStdvarAggregationGroup struct {
groupSeriesCounts []float64
}

func (g *StddevStdvarAggregationGroup) AccumulateSeries(data types.InstantVectorSeriesData, timeRange types.QueryTimeRange, memoryConsumptionTracker *limiting.MemoryConsumptionTracker, _ types.EmitAnnotationFunc) error {
var err error
func (g *StddevStdvarAggregationGroup) AccumulateSeries(data types.InstantVectorSeriesData, timeRange types.QueryTimeRange, memoryConsumptionTracker *limiting.MemoryConsumptionTracker, emitAnnotation types.EmitAnnotationFunc) error {
// Native histograms are ignored for stddev and stdvar.
if len(data.Histograms) > 0 {
emitAnnotation(func(_ string, expressionPosition posrange.PositionRange) error {
name := "stdvar"

if g.stddev {
name = "stddev"
}

return annotations.NewHistogramIgnoredInAggregationInfo(name, expressionPosition)
})
}

// Native histograms are ignored for stddev
if len(data.Floats) > 0 && g.floats == nil {
// First series with float values for this group, populate it.
var err error
g.floats, err = types.Float64SlicePool.Get(timeRange.StepCount, memoryConsumptionTracker)
if err != nil {
return err
Expand Down
6 changes: 5 additions & 1 deletion pkg/streamingpromql/operators/binops/scalar_scalar_binary_operation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (s *ScalarScalarBinaryOperation) GetValues(ctx context.Context) (types.Scal
for i, left := range leftValues.Samples {
right := rightValues.Samples[i]

f, h, ok, err := s.opFunc(left.F, right.F, nil, nil)
f, h, ok, valid, err := s.opFunc(left.F, right.F, nil, nil)

if err != nil {
return types.ScalarData{}, err
Expand All @@ -85,6 +85,10 @@ func (s *ScalarScalarBinaryOperation) GetValues(ctx context.Context) (types.Scal
panic(fmt.Sprintf("%v binary operation between two scalars (%v and %v) did not produce a result, this should never happen", s.Op.String(), left.F, right.F))
}

if !valid {
panic(fmt.Sprintf("%v binary operation between two scalars (%v and %v) was not considered a valid operation, this should never happen", s.Op.String(), left.F, right.F))
}

if h != nil {
panic(fmt.Sprintf("%v binary operation between two scalars (%v and %v) produced a histogram result, this should never happen", s.Op.String(), left.F, right.F))
}
Expand Down
Loading

0 comments on commit fe18ce2

Please sign in to comment.