Skip to content

Commit 87d1062

Browse files
authored
Take into account vector matching in binary expression in rule prepare (#49)
* Take into account vector matching in binary expression while preparing rules
1 parent 711b0fc commit 87d1062

File tree

16 files changed

+6840
-24
lines changed

16 files changed

+6840
-24
lines changed

CHANGELOG.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@
22

33
## unreleased / master
44

5-
## v0.2.0 / 2020-06-02
5+
* [FEATURE] Add `rules check` command. It runs various [best practice](https://prometheus.io/docs/practices/rules/) checks against rules.
6+
* [ENHANCEMENT] Ensure `rules prepare` takes into account Vector Matching on PromQL Binary Expressions.
7+
8+
## v0.2.0 / 2020-03-10
69

710
* [FEATURE] Add `rules prepare` command. It allows you add a label to PromQL aggregations and lint your expressions in rule files.
8-
* [FEATURE] Add `logtool` tool. It parses Cortex query-frontend logs and formats them for easy analysis.
11+
* [FEATURE] Add `rules lint` command. It lints, rules YAML and PromQL expression formatting within the rule file
12+
* [FEATURE] Add `logtool` binary. It parses Loki/Cortex query-frontend logs and formats them for easy analysis.
913

1014
## v0.1.4 / 2020-03-10
1115

README.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,12 @@ At the end of the run, the command tells you whenever the operation was a succes
9090

9191
It is important to note that a modification can be a PromQL expression lint or a label add to your aggregation.
9292

93+
#### Rules Check
94+
95+
This commands checks rules against the recommended [best practices](https://prometheus.io/docs/practices/rules/) for rules.
96+
97+
cortextool rules check ./example_rules_one.yaml
98+
9399
## chunktool
94100

95101
This repo also contains the `chunktool`. A client meant to interact with chunks stored and indexed in cortex backends.

pkg/commands/rules.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func (r *RuleCommand) Register(app *kingpin.Application) {
152152
).StringVar(&r.RuleFilesPath)
153153

154154
// Prepare Command
155-
prepareCmd.Arg("rule-files", "The rule files to check.").Required().ExistingFilesVar(&r.RuleFilesList)
155+
prepareCmd.Arg("rule-files", "The rule files to check.").ExistingFilesVar(&r.RuleFilesList)
156156
prepareCmd.Flag("rule-files", "The rule files to check. Flag can be reused to load multiple files.").StringVar(&r.RuleFiles)
157157
prepareCmd.Flag(
158158
"rule-dirs",
@@ -165,7 +165,7 @@ func (r *RuleCommand) Register(app *kingpin.Application) {
165165
prepareCmd.Flag("label", "label to include as part of the aggregations.").Default(defaultPrepareAggregationLabel).Short('l').StringVar(&r.AggregationLabel)
166166

167167
// Lint Command
168-
lintCmd.Arg("rule-files", "The rule files to check.").Required().ExistingFilesVar(&r.RuleFilesList)
168+
lintCmd.Arg("rule-files", "The rule files to check.").ExistingFilesVar(&r.RuleFilesList)
169169
lintCmd.Flag("rule-files", "The rule files to check. Flag can be reused to load multiple files.").StringVar(&r.RuleFiles)
170170
lintCmd.Flag(
171171
"rule-dirs",

pkg/rules/rules.go

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"strings"
66

7+
promql "github.com/cortexproject/cortex/pkg/configs/legacy_promql"
78
rulefmt "github.com/cortexproject/cortex/pkg/ruler/legacy_rulefmt"
89
"github.com/prometheus/prometheus/promql/parser"
910
log "github.com/sirupsen/logrus"
@@ -29,7 +30,7 @@ func (r RuleNamespace) LintPromQLExpressions() (int, int, error) {
2930
for i, group := range r.Groups {
3031
for j, rule := range group.Rules {
3132
log.WithFields(log.Fields{"rule": getRuleName(rule)}).Debugf("linting PromQL")
32-
exp, err := parser.ParseExpr(rule.Expr)
33+
exp, err := promql.ParseExpr(rule.Expr)
3334
if err != nil {
3435
return count, mod, err
3536
}
@@ -122,34 +123,68 @@ func (r RuleNamespace) AggregateBy(label string) (int, int, error) {
122123
}
123124

124125
// exprNodeInspectorFunc returns a PromQL inspector.
125-
// It modifies most PromQL aggregations to include a given label.
126+
// It modifies most PromQL expressions to include a given label.
126127
func exprNodeInspectorFunc(rule rulefmt.Rule, label string) func(node parser.Node, path []parser.Node) error {
127128
return func(node parser.Node, path []parser.Node) error {
128-
aggregation, ok := node.(*parser.AggregateExpr)
129-
if !ok {
130-
return nil
129+
var err error
130+
switch n := node.(type) {
131+
case *parser.AggregateExpr:
132+
err = prepareAggregationExpr(n, label, getRuleName(rule))
133+
case *parser.BinaryExpr:
134+
err = prepareBinaryExpr(n, label, getRuleName(rule))
135+
default:
136+
return err
131137
}
132138

133-
// If the aggregation is about dropping labels (e.g. without), we don't want to modify
134-
// this expression. Omission as long as it is not the cluster label will include it.
135-
// TODO: We probably want to check whenever the label we're trying to include is included in the omission.
136-
if aggregation.Without {
139+
return err
140+
}
141+
}
142+
143+
func prepareAggregationExpr(e *parser.AggregateExpr, label string, ruleName string) error {
144+
// If the aggregation is about dropping labels (e.g. without), we don't want to modify
145+
// this expression. Omission as long as it is not the cluster label will include it.
146+
// TODO: We probably want to check whenever the label we're trying to include is included in the omission.
147+
if e.Without {
148+
return nil
149+
}
150+
151+
for _, lbl := range e.Grouping {
152+
// It already has the label we want to aggregate by.
153+
if lbl == label {
137154
return nil
138155
}
156+
}
139157

140-
for _, lbl := range aggregation.Grouping {
141-
if lbl == label {
142-
return nil
143-
}
144-
}
158+
log.WithFields(
159+
log.Fields{"rule": ruleName, "lbls": strings.Join(e.Grouping, ", ")},
160+
).Debugf("aggregation without '%s' label, adding.", label)
145161

146-
log.WithFields(
147-
log.Fields{"rule": getRuleName(rule), "lbls": strings.Join(aggregation.Grouping, ", ")},
148-
).Debugf("aggregation without '%s' label, adding.", label)
162+
e.Grouping = append(e.Grouping, label)
163+
return nil
164+
}
149165

150-
aggregation.Grouping = append(aggregation.Grouping, label)
166+
func prepareBinaryExpr(e *parser.BinaryExpr, label string, rule string) error {
167+
if e.VectorMatching == nil {
151168
return nil
152169
}
170+
171+
if !e.VectorMatching.On {
172+
return nil
173+
}
174+
175+
for _, lbl := range e.VectorMatching.MatchingLabels {
176+
// It already has the label we want to add in the expression.
177+
if lbl == label {
178+
return nil
179+
}
180+
}
181+
182+
log.WithFields(
183+
log.Fields{"rule": rule, "lbls": strings.Join(e.VectorMatching.MatchingLabels, ", ")},
184+
).Debugf("binary expression without '%s' label, adding.", label)
185+
186+
e.VectorMatching.MatchingLabels = append(e.VectorMatching.MatchingLabels, label)
187+
return nil
153188
}
154189

155190
// Validate each rule in the rule namespace is valid

pkg/rules/rules_test.go

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ func TestAggregateBy(t *testing.T) {
2222
count: 0, modified: 0, expect: nil,
2323
},
2424
{
25-
name: "no modifcation",
25+
name: "no modification",
2626
rn: RuleNamespace{
2727
Groups: []rulefmt.RuleGroup{{Name: "WithoutAggregation", Rules: []rulefmt.Rule{
2828
{Alert: "WithoutAggregation", Expr: "up != 1"},
@@ -76,6 +76,18 @@ func TestAggregateBy(t *testing.T) {
7676
expectedExpr: []string{`count by(cluster) (count by(gitVersion, cluster) (label_replace(kubernetes_build_info{job!~"kube-dns|coredns"}, "gitVersion", "$1", "gitVersion", "(v[0-9]*.[0-9]*.[0-9]*).*"))) > 1`},
7777
count: 1, modified: 1, expect: nil,
7878
},
79+
{
80+
name: "with vector matching in binary operations",
81+
rn: RuleNamespace{
82+
Groups: []rulefmt.RuleGroup{{Name: "BinaryExpressions", Rules: []rulefmt.Rule{
83+
{Alert: "VectorMatching", Expr: `
84+
count by(cluster, node) (sum by(node, cpu, cluster) (node_cpu_seconds_total{job="default/node-exporter"} * on(namespace, instance) group_left(node) node_namespace_pod:kube_pod_info:))
85+
`},
86+
}}},
87+
},
88+
expectedExpr: []string{`count by(cluster, node) (sum by(node, cpu, cluster) (node_cpu_seconds_total{job="default/node-exporter"} * on(namespace, instance, cluster) group_left(node) node_namespace_pod:kube_pod_info:))`},
89+
count: 1, modified: 1, expect: nil,
90+
},
7991
}
8092

8193
for _, tc := range tt {
@@ -130,7 +142,7 @@ func TestLintPromQLExpressions(t *testing.T) {
130142
expr: "it fails",
131143
expected: "it fails",
132144
count: 0, modified: 0,
133-
err: "1:4: parse error: unexpected identifier \"fails\"",
145+
err: "parse error at char 4: could not parse remaining input \"fails\"...",
134146
},
135147
}
136148

0 commit comments

Comments
 (0)