From 5fa0936b7e6ee8346f7b821a5076269baac506d9 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Tue, 8 Nov 2022 11:41:57 +0000 Subject: [PATCH] Alerting: Fix mathexp.NoData in ConditionsCmd (#56812) This commit fixes an issue where mathexp.NoData would return an error in ConditionsCmd (Classic Condition) instead of no data. It further refactors the Execute method to make it easier to understand. --- pkg/expr/classic/classic.go | 144 ++++++++++++++++++------------------ 1 file changed, 73 insertions(+), 71 deletions(-) diff --git a/pkg/expr/classic/classic.go b/pkg/expr/classic/classic.go index 4b06f0ce65b..9f4c1100ade 100644 --- a/pkg/expr/classic/classic.go +++ b/pkg/expr/classic/classic.go @@ -69,104 +69,106 @@ func (cmd *ConditionsCmd) NeedsVars() []string { // Execute runs the command and returns the results or an error if the command // failed to execute. func (cmd *ConditionsCmd) Execute(_ context.Context, _ time.Time, vars mathexp.Vars) (mathexp.Results, error) { - firing := true - newRes := mathexp.Results{} - noDataFound := true + // isFiring and isNoData tracks whether ConditionsCmd is firing or no data + var isFiring, isNoData bool + var res mathexp.Results - matches := []EvalMatch{} + matches := make([]EvalMatch, 0) + for ix, cond := range cmd.Conditions { + // isCondFiring and isCondNoData tracks whether the condition is firing or no data + // + // There are a number of reasons a condition can have no data: + // + // 1. The input data vars[cond.InputRefID] has no values + // 2. The input data has one or more values, however all are mathexp.NoData + // 3. The input data has one or more values of mathexp.Number or mathexp.Series, + // however the either all mathexp.Number have a nil float64 or the reduce function + // for all mathexp.Series returns a mathexp.Number with a nil float64 + // 4. The input data is a combination of all mathexp.NoData, mathexp.Number with a nil + // float64, or mathexp.Series that reduce to a nil float64 + var isCondFiring, isCondNoData bool + var numSeriesNoData int - for i, c := range cmd.Conditions { - querySeriesSet := vars[c.InputRefID] - nilReducedCount := 0 - firingCount := 0 - for _, val := range querySeriesSet.Values { - var reducedNum mathexp.Number - var name string - switch v := val.(type) { + series := vars[cond.InputRefID] + for _, value := range series.Values { + var ( + name string + number mathexp.Number + ) + switch v := value.(type) { case mathexp.NoData: + // Reduce expressions return v.New(), however classic conditions use the operator + // in the condition to determine if the outcome of ConditionsCmd is no data. // To keep this code as simple as possible we translate mathexp.NoData into a // mathexp.Number with a nil value so number.GetFloat64Value() returns nil - reducedNum = mathexp.NewNumber("no data", nil) - reducedNum.SetValue(nil) - case mathexp.Series: - reducedNum = c.Reducer.Reduce(v) - name = v.GetName() + number = mathexp.NewNumber("no data", nil) + number.SetValue(nil) case mathexp.Number: - reducedNum = v if len(v.Frame.Fields) > 0 { name = v.Frame.Fields[0].Name } + number = v + case mathexp.Series: + name = v.GetName() + number = cond.Reducer.Reduce(v) default: - return newRes, fmt.Errorf("can only reduce type series, got type %v", val.Type()) + return res, fmt.Errorf("can only reduce type series, got type %v", v.Type()) } - // TODO handle error / no data signals - thisCondNoDataFound := reducedNum.GetFloat64Value() == nil - - if thisCondNoDataFound { - nilReducedCount++ - } - - evalRes := c.Evaluator.Eval(reducedNum) - - if evalRes { - match := EvalMatch{ - Value: reducedNum.GetFloat64Value(), + // Check if the value was either a mathexp.NoData, a mathexp.Number with a nil float64, + // or mathexp.Series that reduced to a nil float64 + if number.GetFloat64Value() == nil { + numSeriesNoData += 1 + } else if isValueFiring := cond.Evaluator.Eval(number); isValueFiring { + isCondFiring = true + // If the condition is met then add it to the list of matching conditions + labels := number.GetLabels() + if labels != nil { + labels = labels.Copy() + } + matches = append(matches, EvalMatch{ Metric: name, - } - if reducedNum.GetLabels() != nil { - match.Labels = reducedNum.GetLabels().Copy() - } - matches = append(matches, match) - firingCount++ + Value: number.GetFloat64Value(), + Labels: labels, + }) } } - thisCondFiring := firingCount > 0 - thisCondNoData := len(querySeriesSet.Values) == nilReducedCount - - if i == 0 { - firing = thisCondFiring - noDataFound = thisCondNoData - } - - if c.Operator == "or" { - firing = firing || thisCondFiring - noDataFound = noDataFound || thisCondNoData - } else { - firing = firing && thisCondFiring - noDataFound = noDataFound && thisCondNoData - } - - if thisCondNoData { + // The condition is no data iff all the input data is a combination of all mathexp.NoData, + // mathexp.Number with a nil loat64, or mathexp.Series that reduce to a nil float64 + isCondNoData = numSeriesNoData == len(series.Values) + if isCondNoData { matches = append(matches, EvalMatch{ Metric: "NoData", }) - noDataFound = true } - firingCount = 0 - nilReducedCount = 0 + if ix == 0 { + isFiring = isCondFiring + isNoData = isCondNoData + } else if cond.Operator == "or" { + isFiring = isFiring || isCondFiring + isNoData = isNoData || isCondNoData + } else { + isFiring = isFiring && isCondFiring + isNoData = isNoData && isCondNoData + } } - num := mathexp.NewNumber("", nil) - - num.SetMeta(matches) - var v float64 - switch { - case noDataFound: - num.SetValue(nil) - case firing: + number := mathexp.NewNumber("", nil) + number.SetMeta(matches) + if isFiring { v = 1 - num.SetValue(&v) - case !firing: - num.SetValue(&v) + number.SetValue(&v) + } else if isNoData { + number.SetValue(nil) + } else { + number.SetValue(&v) } - newRes.Values = append(newRes.Values, num) - - return newRes, nil + res.Values = append(res.Values, number) + return res, nil } // EvalMatch represents the series violating the threshold.