Bubble up errors to promql from populating iterators (#4136)

This changes the Walk/Inspect API inside the promql package to bubble
up errors. This is done by having the inspector return an error (instead
of a bool) and then bubbling that up in the Walk. This way if any error
is encountered in the Walk() the walk will stop and return the error.
This avoids issues where errors from the Querier where being ignored
(causing incorrect promql evaluation).

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes #4136

promql/ast.go

@@ -237,57 +237,80 @@ type VectorMatching struct {
 
 // Visitor allows visiting a Node and its child nodes. The Visit method is
 // invoked for each node with the path leading to the node provided additionally.
-// If the result visitor w is not nil, Walk visits each of the children
+// If the result visitor w is not nil and no error, Walk visits each of the children
 // of node with the visitor w, followed by a call of w.Visit(nil, nil).
 type Visitor interface {
-	Visit(node Node, path []Node) (w Visitor)
+	Visit(node Node, path []Node) (w Visitor, err error)
 }
 
 // Walk traverses an AST in depth-first order: It starts by calling
 // v.Visit(node, path); node must not be nil. If the visitor w returned by
-// v.Visit(node, path) is not nil, Walk is invoked recursively with visitor
+// v.Visit(node, path) is not nil and the visitor returns no error, Walk is
-// w for each of the non-nil children of node, followed by a call of
+// invoked recursively with visitor w for each of the non-nil children of node,
-// w.Visit(nil).
+// followed by a call of w.Visit(nil), returning an error
 // As the tree is descended the path of previous nodes is provided.
-func Walk(v Visitor, node Node, path []Node) {
+func Walk(v Visitor, node Node, path []Node) error {
-	if v = v.Visit(node, path); v == nil {
+	var err error
-		return
+	if v, err = v.Visit(node, path); v == nil || err != nil {
+		return err
 	}
 	path = append(path, node)
 
 	switch n := node.(type) {
 	case Statements:
 		for _, s := range n {
-			Walk(v, s, path)
+			if err := Walk(v, s, path); err != nil {
+				return err
+			}
 		}
 	case *AlertStmt:
-		Walk(v, n.Expr, path)
+		if err := Walk(v, n.Expr, path); err != nil {
+			return err
+		}
 
 	case *EvalStmt:
-		Walk(v, n.Expr, path)
+		if err := Walk(v, n.Expr, path); err != nil {
+			return err
+		}
 
 	case *RecordStmt:
-		Walk(v, n.Expr, path)
+		if err := Walk(v, n.Expr, path); err != nil {
+			return err
+		}
 
 	case Expressions:
 		for _, e := range n {
-			Walk(v, e, path)
+			if err := Walk(v, e, path); err != nil {
+				return err
+			}
 		}
 	case *AggregateExpr:
-		Walk(v, n.Expr, path)
+		if err := Walk(v, n.Expr, path); err != nil {
+			return err
+		}
 
 	case *BinaryExpr:
-		Walk(v, n.LHS, path)
+		if err := Walk(v, n.LHS, path); err != nil {
-		Walk(v, n.RHS, path)
+			return err
+		}
+		if err := Walk(v, n.RHS, path); err != nil {
+			return err
+		}
 
 	case *Call:
-		Walk(v, n.Args, path)
+		if err := Walk(v, n.Args, path); err != nil {
+			return err
+		}
 
 	case *ParenExpr:
-		Walk(v, n.Expr, path)
+		if err := Walk(v, n.Expr, path); err != nil {
+			return err
+		}
 
 	case *UnaryExpr:
-		Walk(v, n.Expr, path)
+		if err := Walk(v, n.Expr, path); err != nil {
+			return err
+		}
 
 	case *MatrixSelector, *NumberLiteral, *StringLiteral, *VectorSelector:
 		// nothing to do
@@ -296,21 +319,23 @@ func Walk(v Visitor, node Node, path []Node) {
 		panic(fmt.Errorf("promql.Walk: unhandled node type %T", node))
 	}
 
-	v.Visit(nil, nil)
+	_, err = v.Visit(nil, nil)
+	return err
 }
 
-type inspector func(Node, []Node) bool
+type inspector func(Node, []Node) error
 
-func (f inspector) Visit(node Node, path []Node) Visitor {
+func (f inspector) Visit(node Node, path []Node) (Visitor, error) {
-	if f(node, path) {
+	if err := f(node, path); err == nil {
-		return f
+		return f, nil
+	} else {
+		return nil, err
 	}
-	return nil
 }
 
 // Inspect traverses an AST in depth-first order: It starts by calling
-// f(node, path); node must not be nil. If f returns true, Inspect invokes f
+// f(node, path); node must not be nil. If f returns a nil error, Inspect invokes f
 // for all the non-nil children of node, recursively.
-func Inspect(node Node, f func(Node, []Node) bool) {
+func Inspect(node Node, f inspector) {
 	Walk(inspector(f), node, nil)
 }

promql/engine.go

@@ -451,7 +451,7 @@ func (ng *Engine) execEvalStmt(ctx context.Context, query *query, s *EvalStmt) (
 
 func (ng *Engine) populateSeries(ctx context.Context, q storage.Queryable, s *EvalStmt) (storage.Querier, error) {
 	var maxOffset time.Duration
-	Inspect(s.Expr, func(node Node, _ []Node) bool {
+	Inspect(s.Expr, func(node Node, _ []Node) error {
 		switch n := node.(type) {
 		case *VectorSelector:
 			if maxOffset < LookbackDelta {
@@ -468,7 +468,7 @@ func (ng *Engine) populateSeries(ctx context.Context, q storage.Queryable, s *Ev
 				maxOffset = n.Offset + n.Range
 			}
 		}
-		return true
+		return nil
 	})
 
 	mint := s.Start.Add(-maxOffset)
@@ -478,7 +478,7 @@ func (ng *Engine) populateSeries(ctx context.Context, q storage.Queryable, s *Ev
 		return nil, err
 	}
 
-	Inspect(s.Expr, func(node Node, path []Node) bool {
+	Inspect(s.Expr, func(node Node, path []Node) error {
 		var set storage.SeriesSet
 		params := &storage.SelectParams{
 			Step: int64(s.Interval / time.Millisecond),
@@ -491,13 +491,13 @@ func (ng *Engine) populateSeries(ctx context.Context, q storage.Queryable, s *Ev
 			set, err = querier.Select(params, n.LabelMatchers...)
 			if err != nil {
 				level.Error(ng.logger).Log("msg", "error selecting series set", "err", err)
-				return false
+				return err
 			}
 			n.series, err = expandSeriesSet(set)
 			if err != nil {
 				// TODO(fabxc): use multi-error.
 				level.Error(ng.logger).Log("msg", "error expanding series set", "err", err)
-				return false
+				return err
 			}
 
 		case *MatrixSelector:
@@ -506,15 +506,15 @@ func (ng *Engine) populateSeries(ctx context.Context, q storage.Queryable, s *Ev
 			set, err = querier.Select(params, n.LabelMatchers...)
 			if err != nil {
 				level.Error(ng.logger).Log("msg", "error selecting series set", "err", err)
-				return false
+				return err
 			}
 			n.series, err = expandSeriesSet(set)
 			if err != nil {
 				level.Error(ng.logger).Log("msg", "error expanding series set", "err", err)
-				return false
+				return err
 			}
 		}
-		return true
+		return nil
 	})
 	return querier, err
 }