Skip to content

Commit

Permalink
Refactor promql parsing code
Browse files Browse the repository at this point in the history
  • Loading branch information
@prymitive
prymitive committed Mar 18, 2024
1 parent d6dc933 commit a2a588b
Show file tree
Hide file tree
Showing 20 changed files with 158 additions and 383 deletions.
4 changes: 2 additions & 2 deletions internal/checks/alerts_comparison.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (c ComparisonCheck) Check(_ context.Context, _ string, rule parser.Rule, _
}
}

if n := hasComparision(expr.Node); n != nil {
if n := hasComparision(expr.Expr); n != nil {
if n.ReturnBool && hasComparision(n.LHS) == nil && hasComparision(n.RHS) == nil {
problems = append(problems, Problem{
Lines: rule.AlertingRule.Expr.Value.Lines,
Expand Down Expand Up @@ -130,7 +130,7 @@ func isAbsent(node promParser.Node) bool {
}

func hasAbsent(n *parser.PromQLNode) bool {
if isAbsent(n.Node) {
if isAbsent(n.Expr) {
return true
}
for _, child := range n.Children {
Expand Down
4 changes: 2 additions & 2 deletions internal/checks/alerts_template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ func absentLabels(f utils.PromQLFragment) []string {
}

func binaryExprs(node *parser.PromQLNode) (be []*promParser.BinaryExpr) {
if n, ok := node.Node.(*promParser.BinaryExpr); ok {
if n, ok := node.Expr.(*promParser.BinaryExpr); ok {
be = append(be, n)
}

Expand All @@ -663,7 +663,7 @@ func binaryExprs(node *parser.PromQLNode) (be []*promParser.BinaryExpr) {
}

func calls(node *parser.PromQLNode, name string) (cl []*promParser.Call) {
if n, ok := node.Node.(*promParser.Call); ok && n.Func.Name == name {
if n, ok := node.Expr.(*promParser.Call); ok && n.Func.Name == name {
cl = append(cl, n)
}

Expand Down
12 changes: 6 additions & 6 deletions internal/checks/promql_aggregation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (c AggregationCheck) Check(_ context.Context, _ string, rule parser.Rule, _
}

func (c AggregationCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem) {
if n, ok := node.Node.(*promParser.AggregateExpr); ok {
if n, ok := node.Expr.(*promParser.AggregateExpr); ok {
switch n.Op {
case promParser.SUM:
case promParser.MIN:
Expand Down Expand Up @@ -131,14 +131,14 @@ func (c AggregationCheck) checkNode(node *parser.PromQLNode) (problems []exprPro
if n.Without {
if found && c.keep {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("`%s` label is required and should be preserved when aggregating `%s` rules, remove %s from `without()`.", c.label, c.nameRegex.anchored, c.label),
})
}

if !found && !c.keep {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("`%s` label should be removed when aggregating `%s` rules, use `without(%s, ...)`.", c.label, c.nameRegex.anchored, c.label),
})
}
Expand All @@ -151,14 +151,14 @@ func (c AggregationCheck) checkNode(node *parser.PromQLNode) (problems []exprPro
} else {
if found && !c.keep {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("`%s` label should be removed when aggregating `%s` rules, remove %s from `by()`.", c.label, c.nameRegex.anchored, c.label),
})
}

if !found && c.keep {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("`%s` label is required and should be preserved when aggregating `%s` rules, use `by(%s, ...)`.", c.label, c.nameRegex.anchored, c.label),
})
}
Expand All @@ -172,7 +172,7 @@ func (c AggregationCheck) checkNode(node *parser.PromQLNode) (problems []exprPro
}

NEXT:
if n, ok := node.Node.(*promParser.BinaryExpr); ok && n.VectorMatching != nil {
if n, ok := node.Expr.(*promParser.BinaryExpr); ok && n.VectorMatching != nil {
switch n.VectorMatching.Card {
case promParser.CardOneToOne:
// sum() + sum()
Expand Down
10 changes: 4 additions & 6 deletions internal/checks/promql_counter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (

"github.com/cloudflare/pint/internal/discovery"
"github.com/cloudflare/pint/internal/parser"
"github.com/cloudflare/pint/internal/parser/utils"
"github.com/cloudflare/pint/internal/promapi"
)

Expand Down Expand Up @@ -58,31 +57,30 @@ func (c CounterCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ [

done := map[string]struct{}{}

tree := utils.Tree(expr.Query.Node, nil)
LOOP:
for _, vs := range utils.WalkDownExpr[*promParser.VectorSelector](&tree) {
for _, vs := range parser.WalkDownExpr[*promParser.VectorSelector](expr.Query) {
if vs.Parent == nil {
// This might be a counter but there's no parent so we have something like `expr: foo`.
// We're only testing for the existence of foo in alerts OR copying it via recording rules.
continue LOOP
}

for _, call := range utils.WalkUpExpr[*promParser.Call](vs.Parent) {
for _, call := range parser.WalkUpExpr[*promParser.Call](vs.Parent) {
if fn := call.Expr.(*promParser.Call); c.isSafeFunc(fn.Func.Name) {
// This might be a counter but it's wrapped in one of the functions that make it
// safe to use.
continue LOOP
}
}

for _, aggr := range utils.WalkUpExpr[*promParser.AggregateExpr](vs.Parent) {
for _, aggr := range parser.WalkUpExpr[*promParser.AggregateExpr](vs.Parent) {
if ag := aggr.Expr.(*promParser.AggregateExpr); ag.Op == promParser.COUNT || ag.Op == promParser.GROUP {
// This might be a counter but it's wrapped in count() or group() call so it's safe to use.
continue LOOP
}
}

for _, binSide := range utils.WalkUpParent[*promParser.BinaryExpr](vs) {
for _, binSide := range parser.WalkUpParent[*promParser.BinaryExpr](vs) {
if binExp := binSide.Parent.Expr.(*promParser.BinaryExpr); binExp.Op == promParser.LUNLESS {
// We're inside a binary expression with `foo unless bar`.
// Check which side we're at, if it's the RHS then it's safe to use counters directly.
Expand Down
2 changes: 1 addition & 1 deletion internal/checks/promql_fragile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (c FragileCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem
}
if len(series) >= 2 {
p := exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: "Aggregation using `without()` can be fragile when used inside binary expression because both sides must have identical sets of labels to produce any results, adding or removing labels to metrics used here can easily break the query, consider aggregating using `by()` to ensure consistent labels.",
severity: Warning,
}
Expand Down
4 changes: 2 additions & 2 deletions internal/checks/promql_range_query.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ func (c RangeQueryCheck) Check(ctx context.Context, _ string, rule parser.Rule,
}

func (c RangeQueryCheck) checkNode(ctx context.Context, node *parser.PromQLNode, retention time.Duration, uri string) (problems []exprProblem) {
if n, ok := node.Node.(*promParser.MatrixSelector); ok {
if n, ok := node.Expr.(*promParser.MatrixSelector); ok {
if n.Range > retention {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("`%s` selector is trying to query Prometheus for %s worth of metrics, but %s is configured to only keep %s of metrics history.",
node.Expr, model.Duration(n.Range), promText(c.prom.Name(), uri), model.Duration(retention)),
severity: Warning,
Expand Down
6 changes: 3 additions & 3 deletions internal/checks/promql_rate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,15 @@ func (c RateCheck) Check(ctx context.Context, _ string, rule parser.Rule, entrie
}

func (c RateCheck) checkNode(ctx context.Context, node *parser.PromQLNode, entries []discovery.Entry, cfg *promapi.ConfigResult, done *completedList) (problems []exprProblem) {
if n, ok := node.Node.(*promParser.Call); ok && (n.Func.Name == "rate" || n.Func.Name == "irate" || n.Func.Name == "deriv") {
if n, ok := node.Expr.(*promParser.Call); ok && (n.Func.Name == "rate" || n.Func.Name == "irate" || n.Func.Name == "deriv") {
for _, arg := range n.Args {
m, ok := arg.(*promParser.MatrixSelector)
if !ok {
continue
}
if m.Range < cfg.Config.Global.ScrapeInterval*time.Duration(c.minIntervals) {
p := exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("Duration for `%s()` must be at least %d x scrape_interval, %s is using `%s` scrape_interval.",
n.Func.Name, c.minIntervals, promText(c.prom.Name(), cfg.URI), output.HumanizeDuration(cfg.Config.Global.ScrapeInterval)),
details: RateCheckDetails,
Expand Down Expand Up @@ -163,7 +163,7 @@ func (c RateCheck) checkNode(ctx context.Context, node *parser.PromQLNode, entri
for _, m := range metadata.Metadata {
if m.Type == v1.MetricTypeCounter {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("`rate(sum(counter))` chain detected, `%s` is called here on results of `%s`, calling `rate()` on `sum()` results will return bogus results, always `sum(rate(counter))`, never `rate(sum(counter))`.",
node.Expr, sm),
severity: Bug,
Expand Down
8 changes: 3 additions & 5 deletions internal/checks/promql_series.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ 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"
Expand Down Expand Up @@ -677,14 +676,13 @@ func (c SeriesCheck) textAndSeverity(settings *PromqlSeriesSettings, name, text
}

func getSelectors(n *parser.PromQLNode) (selectors []promParser.VectorSelector) {
tree := utils.Tree(n.Node, nil)
LOOP:
for _, vs := range utils.WalkDownExpr[*promParser.VectorSelector](&tree) {
for _, bin := range utils.WalkUpExpr[*promParser.BinaryExpr](vs.Parent) {
for _, vs := range parser.WalkDownExpr[*promParser.VectorSelector](n) {
for _, bin := range parser.WalkUpExpr[*promParser.BinaryExpr](vs.Parent) {
if binExp := bin.Expr.(*promParser.BinaryExpr); binExp.Op != promParser.LOR {
continue
}
for _, vec := range utils.WalkDownExpr[*promParser.Call](bin) {
for _, vec := range parser.WalkDownExpr[*promParser.Call](bin) {
if vecCall := vec.Expr.(*promParser.Call); vecCall.Func.Name == "vector" {
// vector seletor is under (...) OR vector()
// ignore it
Expand Down
24 changes: 12 additions & 12 deletions internal/checks/promql_vector_matching.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (c VectorMatchingCheck) Check(ctx context.Context, _ string, rule parser.Ru
}

func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLNode) (problems []exprProblem) {
if n, ok := utils.RemoveConditions(node.Node.String()).(*promParser.BinaryExpr); ok &&
if n, ok := utils.RemoveConditions(node.Expr.String()).(*promParser.BinaryExpr); ok &&
n.VectorMatching != nil &&
n.Op != promParser.LOR &&
n.Op != promParser.LUNLESS {
Expand All @@ -82,7 +82,7 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
if err != nil {
text, severity := textAndSeverityFromError(err, c.Reporter(), c.prom.Name(), Bug)
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: text,
severity: severity,
})
Expand Down Expand Up @@ -125,7 +125,7 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
for k, lv := range lhsMatchers {
if rv, ok := rhsMatchers[k]; ok && rv != lv {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("The left hand side uses `{%s=%q}` while the right hand side uses `{%s=%q}`, this will never match.", k, lv, k, rv),
details: VectorMatchingCheckDetails,
severity: Bug,
Expand All @@ -139,7 +139,7 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
if err != nil {
text, severity := textAndSeverityFromError(err, c.Reporter(), c.prom.Name(), Bug)
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: text,
severity: severity,
})
Expand All @@ -153,7 +153,7 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
if err != nil {
text, severity := textAndSeverityFromError(err, c.Reporter(), c.prom.Name(), Bug)
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: text,
severity: severity,
})
Expand All @@ -167,23 +167,23 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
for _, name := range n.VectorMatching.MatchingLabels {
if !leftLabels.hasName(name) && rightLabels.hasName(name) {
problems = append(problems, exprProblem{
expr: node.Expr,
text: fmt.Sprintf("Using `on(%s)` won't produce any results because the left hand side of the query doesn't have this label: `%s`.", name, node.Node.(*promParser.BinaryExpr).LHS),
expr: node.Expr.String(),
text: fmt.Sprintf("Using `on(%s)` won't produce any results because the left hand side of the query doesn't have this label: `%s`.", name, node.Expr.(*promParser.BinaryExpr).LHS),
details: VectorMatchingCheckDetails,
severity: Bug,
})
}
if leftLabels.hasName(name) && !rightLabels.hasName(name) {
problems = append(problems, exprProblem{
expr: node.Expr,
text: fmt.Sprintf("Using `on(%s)` won't produce any results because the right hand side of the query doesn't have this label: `%s`.", name, node.Node.(*promParser.BinaryExpr).RHS),
expr: node.Expr.String(),
text: fmt.Sprintf("Using `on(%s)` won't produce any results because the right hand side of the query doesn't have this label: `%s`.", name, node.Expr.(*promParser.BinaryExpr).RHS),
details: VectorMatchingCheckDetails,
severity: Bug,
})
}
if !leftLabels.hasName(name) && !rightLabels.hasName(name) {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("Using `on(%s)` won't produce any results because both sides of the query don't have this label.", name),
details: VectorMatchingCheckDetails,
severity: Bug,
Expand All @@ -194,14 +194,14 @@ func (c VectorMatchingCheck) checkNode(ctx context.Context, node *parser.PromQLN
l, r := leftLabels.getFirstNonOverlap(rightLabels)
if len(n.VectorMatching.MatchingLabels) == 0 {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("This query will never return anything because the right and the left hand side have different labels: `%s` != `%s`.", l, r),
details: VectorMatchingCheckDetails,
severity: Bug,
})
} else {
problems = append(problems, exprProblem{
expr: node.Expr,
expr: node.Expr.String(),
text: fmt.Sprintf("Using `ignoring(%s)` won't produce any results because both sides of the query have different labels: `%s` != `%s`.", strings.Join(n.VectorMatching.MatchingLabels, ","), l, r),
details: VectorMatchingCheckDetails,
severity: Bug,
Expand Down
Loading

0 comments on commit a2a588b

Please sign in to comment.