From 404abe0f1c19053581ba890cb40d2a2f7c6b758e Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Thu, 7 Jun 2018 09:27:34 -0700 Subject: [PATCH] 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 Fixes #4136 --- promql/ast.go | 79 +++++++++++++++++++++++++++++++----------------- promql/engine.go | 16 +++++----- 2 files changed, 60 insertions(+), 35 deletions(-) diff --git a/promql/ast.go b/promql/ast.go index af3ed4189..0c8a3ad2a 100644 --- a/promql/ast.go +++ b/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 -// w for each of the non-nil children of node, followed by a call of -// w.Visit(nil). +// v.Visit(node, path) is not nil and the visitor returns no error, Walk is +// invoked recursively with visitor w for each of the non-nil children of node, +// 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) { - if v = v.Visit(node, path); v == nil { - return +func Walk(v Visitor, node Node, path []Node) error { + var err error + 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) - Walk(v, n.RHS, path) + if err := Walk(v, n.LHS, path); err != nil { + 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 { - if f(node, path) { - return f +func (f inspector) Visit(node Node, path []Node) (Visitor, error) { + if err := f(node, path); err == nil { + 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) } diff --git a/promql/engine.go b/promql/engine.go index 2206607c4..adf3bf00c 100644 --- a/promql/engine.go +++ b/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 }