Merge pull request #905 from cloudflare/vector

Ignore vector selectors with fallback values

docs/changelog.md

@@ -11,6 +11,16 @@
 
 - `pint watch` command now has two sub-commands: `pint watch glob` and `pint watch rule_files`.
   See [watch mode](index.md#watch-mode) docs for details.
+- [promql/series](checks/promql/series.md) check will now ignore missing metrics if the
+  query uses a fallback with `or vector(1)`. This will reduce the number of reported
+  missing metrics for queries where the intention is to accept and ignore missing
+  metrics.
+  Example of a rule that will no longer report problems:
+
+  ```yaml
+  - alert: Foo
+    expr: sum(my_metric or vector(0)) > 1
+  ```
 
 ### Fixed
 

docs/checks/promql/series.md

@@ -21,6 +21,16 @@ to determine why, by checking if:
 - `my_metric` has any series with `foo` label
 - `my_metric` has any series matching `foo="bar"`
 
+Metrics that are wrapped in `... or vector(0)` won't be checked, since
+the intention of adding `or vector(0)` is to provide a fallback value
+when there are no matching time series.
+Example:
+
+```yaml
+- alert: Foo
+  expr: sum(my_metric or vector(0)) > 1
+```
+
 ## Common problems
 
 If you see this check complaining about some metric it's might due to a number
@@ -75,7 +85,7 @@ problems like label mismatch.
 - Service exporting your metric is not working or no longer being scraped.
 - You are querying wrong Prometheus server.
 - You are trying to filter a metric that exists using a label key that is
-    never present on that metric.
+  never present on that metric.
 - You are using label value as a filter, but that value is never present.
 
 If that's the case you need to fix you query. Make sure your metric is present
@@ -357,6 +367,6 @@ You can disable this check until given time by adding a comment to it. Example:
 ```
 
 Where `$TIMESTAMP` is either use [RFC3339](https://www.rfc-editor.org/rfc/rfc3339)
-formatted  or `YYYY-MM-DD`.
-Adding this comment will disable `promql/series` *until* `$TIMESTAMP`, after that
+formatted or `YYYY-MM-DD`.
+Adding this comment will disable `promql/series` _until_ `$TIMESTAMP`, after that
 check will be re-enabled.

internal/checks/promql_series.go

@@ -14,6 +14,7 @@ import (
 	"github.com/cloudflare/pint/internal/discovery"
 	"github.com/cloudflare/pint/internal/output"
 	"github.com/cloudflare/pint/internal/parser"
+	"github.com/cloudflare/pint/internal/parser/utils"
 	"github.com/cloudflare/pint/internal/promapi"
 
 	"github.com/prometheus/common/model"
@@ -676,19 +677,32 @@ func (c SeriesCheck) textAndSeverity(settings *PromqlSeriesSettings, name, text
 }
 
 func getSelectors(n *parser.PromQLNode) (selectors []promParser.VectorSelector) {
-	if node, ok := n.Node.(*promParser.VectorSelector); ok {
+	tree := utils.Tree(n.Node, nil)
+OUTER:
+	for _, vs := range utils.WalkDown[*promParser.VectorSelector](&tree) {
+		for _, bin := range utils.WalkUp[*promParser.BinaryExpr](vs.Parent) {
+			if binExp := bin.Expr.(*promParser.BinaryExpr); binExp.Op != promParser.LOR {
+				continue
+			}
+			for _, vec := range utils.WalkDown[*promParser.Call](bin) {
+				if vecCall := vec.Expr.(*promParser.Call); vecCall.Func.Name == "vector" {
+					// vector seletor is under (...) OR vector()
+					// ignore it
+					slog.Debug(
+						"Metric uses vector() fallback value, ignore",
+						slog.String("selector", (vs.Expr.String())),
+					)
+					continue OUTER
+				}
+			}
+		}
 		// copy node without offset
 		nc := promParser.VectorSelector{
-			Name:          node.Name,
-			LabelMatchers: node.LabelMatchers,
+			Name:          vs.Expr.(*promParser.VectorSelector).Name,
+			LabelMatchers: vs.Expr.(*promParser.VectorSelector).LabelMatchers,
 		}
 		selectors = append(selectors, nc)
 	}
-
-	for _, child := range n.Children {
-		selectors = append(selectors, getSelectors(child)...)
-	}
-
 	return selectors
 }
 

internal/checks/promql_series_test.go

@@ -3326,6 +3326,41 @@ func TestSeriesCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "metric with fallback / 1",
+			content:     "- record: foo\n  expr: sum(sometimes{foo!=\"bar\"} or vector(0))\n",
+			checker:     newSeriesCheck,
+			prometheus:  newSimpleProm,
+			problems:    noProblems,
+		},
+		{
+			description: "metric with fallback / 2",
+			content: `
+- alert: foo
+  expr: |
+    (sum(sometimes{foo!=\"bar\"} or vector(0)))
+    or
+    (bob > 10)
+    or
+    vector(1)
+`,
+			checker:    newSeriesCheck,
+			prometheus: newSimpleProm,
+			problems:   noProblems,
+		},
+		{
+			description: "metric with fallback / 3",
+			content: `
+- alert: foo
+  expr: |
+    (sum(sometimes{foo!=\"bar\"} or vector(0)))
+    or
+    ((bob > 10) or sum(foo) or vector(1))
+`,
+			checker:    newSeriesCheck,
+			prometheus: newSimpleProm,
+			problems:   noProblems,
+		},
 	}
 	runTests(t, testCases)
 }

internal/parser/utils/tree.go

@@ -0,0 +1,61 @@
+package utils
+
+import "github.com/prometheus/prometheus/promql/parser"
+
+// Node is used to turn the parsed PromQL query expression into a tree.
+// This allows us to walk the tree up & down and look for either parents
+// or children of specific type. Which is useful if you, for example,
+// want to check if all vector selectors are wrapped inside function
+// calls etc.
+type Node struct {
+	Parent   *Node
+	Expr     parser.Node
+	children []Node
+}
+
+// Tree takes a parsed PromQL node and turns it into a Node
+// instance with parent and children populated.
+func Tree(expr parser.Node, parent *Node) Node {
+	n := Node{
+		Parent: parent,
+		Expr:   expr,
+	}
+	for _, child := range parser.Children(expr) {
+		n.children = append(n.children, Tree(child, &n))
+	}
+	return n
+}
+
+// WalkUp allows to iterate a promQLNode node looking for
+// parents of specific type.
+// Prometheus parser returns interfaces which makes it more difficult
+// to figure out what kind of node we're dealing with, hence this
+// helper takes a type parameter it tries to cast.
+// It starts by checking the node passed to it and then walks
+// up by visiting all parent nodes.
+func WalkUp[T parser.Node](node *Node) (nodes []*Node) {
+	if node == nil {
+		return nodes
+	}
+	if _, ok := node.Expr.(T); ok {
+		nodes = append(nodes, node)
+	}
+	if node.Parent != nil {
+		nodes = append(nodes, WalkUp[T](node.Parent)...)
+	}
+	return nodes
+}
+
+// WalkDown works just like findParents but it walks the tree
+// down, visiting all children.
+// It also starts by checking the node passed to it before walking
+// down the tree.
+func WalkDown[T parser.Node](node *Node) (nodes []*Node) {
+	if _, ok := node.Expr.(T); ok {
+		nodes = append(nodes, node)
+	}
+	for _, child := range node.children {
+		nodes = append(nodes, WalkDown[T](&child)...)
+	}
+	return nodes
+}