Skip to content

Commit 2d9f589

Browse files
prymitiveprymitive
authored
Merge pull request #1528 from cloudflare/func_args
Check arguments of functions used in suggestions
2 parents 693c59b + 321b244 commit 2d9f589

7 files changed

Lines changed: 374 additions & 79 deletions

File tree

‎docs/changelog.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# Changelog
22

3+
## v0.74.8
4+
5+
### Fixed
6+
7+
- Fixed incorrect suggestions from [query/cost](checks/query/cost.md) for function calls with different arguments.
8+
39
## v0.74.7
410

511
### Fixed

‎internal/checks/query_cost.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"log/slog"
77
"math"
88
"net/url"
9+
"slices"
910
"strings"
1011
"time"
1112

@@ -456,6 +457,7 @@ func (c CostCheck) isSuggestionFor(src, potential utils.Source, join *utils.Join
456457
for i := len(src.Operations); i > 0; i-- {
457458
ops := src.Operations[:i]
458459
if c.equalOperations(ops, potential.Operations) {
460+
slog.Debug("Equal operations", slog.Any("query", ops), slog.Any("suggestion", potential.Operations))
459461
if c.metricName(ops) != c.metricName(potential.Operations) {
460462
goto NEXT
461463
}
@@ -503,9 +505,13 @@ func (c CostCheck) equalOperations(a, b utils.SourceOperations) bool {
503505
return false
504506
}
505507
for i := range a {
508+
slog.Debug("Compare operations", slog.Int("idx", i), slog.Any("a", a[i]), slog.Any("b", b[i]))
506509
if c.normalizeFuncName(a[i].Operation) != c.normalizeFuncName(b[i].Operation) {
507510
return false
508511
}
512+
if !slices.Equal(a[i].Arguments, b[i].Arguments) {
513+
return false
514+
}
509515
}
510516
return true
511517
}

‎internal/checks/query_cost_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1588,6 +1588,54 @@ func TestCostCheck(t *testing.T) {
15881588
},
15891589
},
15901590
},
1591+
{
1592+
description: "suggest recording rule / histogram_fraction",
1593+
content: `- record: sum:foo:rate5m
1594+
expr: metric / on() histogram_fraction(0, 0.2, metric)
1595+
`,
1596+
checker: func(prom *promapi.FailoverGroup) checks.RuleChecker {
1597+
return checks.NewCostCheck(prom, 100, 100, 0, 0, "check comment", checks.Warning)
1598+
},
1599+
prometheus: newSimpleProm,
1600+
entries: mustParseContent(`
1601+
1602+
- record: metric:fraction
1603+
expr: histogram_fraction(0, 0.1, metric)
1604+
`),
1605+
mocks: []*prometheusMock{
1606+
{
1607+
conds: []requestCondition{
1608+
requireQueryPath,
1609+
formCond{key: "query", value: "count(metric / on() histogram_fraction(0, 0.2, metric))"},
1610+
},
1611+
resp: vectorResponse{
1612+
samples: []*model.Sample{
1613+
generateSample(map[string]string{}),
1614+
},
1615+
stats: promapi.QueryStats{
1616+
Samples: promapi.QuerySamples{
1617+
TotalQueryableSamples: 50,
1618+
PeakSamples: 50,
1619+
},
1620+
Timings: promapi.QueryTimings{
1621+
EvalTotalTime: 10,
1622+
},
1623+
},
1624+
},
1625+
},
1626+
{
1627+
conds: []requestCondition{
1628+
requireQueryPath,
1629+
formCond{key: "query", value: checks.BytesPerSampleQuery},
1630+
},
1631+
resp: vectorResponse{
1632+
samples: []*model.Sample{
1633+
generateSampleWithValue(map[string]string{}, 2048),
1634+
},
1635+
},
1636+
},
1637+
},
1638+
},
15911639
}
15921640

15931641
runTests(t, testCases)

‎internal/checks/query_cost_test.snap‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -717,3 +717,8 @@
717717
[]
718718

719719
---
720+
721+
[TestCostCheck/suggest_recording_rule_/_histogram_fraction - 1]
722+
[]
723+
724+
---

‎internal/parser/utils/source.go‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -89,14 +89,19 @@ type ReturnInfo struct {
8989
type SourceOperation struct {
9090
Node promParser.Node
9191
Operation string
92+
Arguments []string
9293
}
9394

9495
// Used for test snapshots.
9596
func (so SourceOperation) MarshalYAML() (any, error) {
96-
return map[string]any{
97+
y := map[string]any{
9798
"op": so.Operation,
9899
"node": fmt.Sprintf("[%T] %s", so.Node, so.Node.String()),
99-
}, nil
100+
}
101+
if so.Arguments != nil {
102+
y["args"] = so.Arguments
103+
}
104+
return y, nil
100105
}
101106

102107
type SourceOperations []SourceOperation
@@ -335,6 +340,7 @@ func walkNode(expr string, node promParser.Node) (src []Source) {
335340
s.Operations = append(s.Operations, SourceOperation{
336341
Operation: "",
337342
Node: n,
343+
Arguments: nil,
338344
})
339345
s.guaranteeLabel(
340346
"Query will only return series where these labels are present.",
@@ -401,12 +407,19 @@ func GetQueryFragment(expr string, pos posrange.PositionRange) string {
401407

402408
func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
403409
s := newSource()
410+
411+
var args []string
412+
if n.Param != nil {
413+
args = append(args, n.Param.String())
414+
}
415+
404416
switch n.Op {
405417
case promParser.SUM:
406418
for _, s = range parseAggregation(expr, n) {
407419
s.Operations = append(s.Operations, SourceOperation{
408420
Operation: "sum",
409421
Node: n,
422+
Arguments: args,
410423
})
411424
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
412425
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -418,6 +431,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
418431
s.Operations = append(s.Operations, SourceOperation{
419432
Operation: "min",
420433
Node: n,
434+
Arguments: args,
421435
})
422436
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
423437
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -429,6 +443,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
429443
s.Operations = append(s.Operations, SourceOperation{
430444
Operation: "max",
431445
Node: n,
446+
Arguments: args,
432447
})
433448
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
434449
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -440,6 +455,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
440455
s.Operations = append(s.Operations, SourceOperation{
441456
Operation: "avg",
442457
Node: n,
458+
Arguments: args,
443459
})
444460
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
445461
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -451,6 +467,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
451467
s.Operations = append(s.Operations, SourceOperation{
452468
Operation: "group",
453469
Node: n,
470+
Arguments: args,
454471
})
455472
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
456473
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -462,6 +479,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
462479
s.Operations = append(s.Operations, SourceOperation{
463480
Operation: "stddev",
464481
Node: n,
482+
Arguments: args,
465483
})
466484
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
467485
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -473,6 +491,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
473491
s.Operations = append(s.Operations, SourceOperation{
474492
Operation: "stdvar",
475493
Node: n,
494+
Arguments: args,
476495
})
477496
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
478497
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -484,6 +503,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
484503
s.Operations = append(s.Operations, SourceOperation{
485504
Operation: "count",
486505
Node: n,
506+
Arguments: args,
487507
})
488508
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
489509
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -495,6 +515,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
495515
s.Operations = append(s.Operations, SourceOperation{
496516
Operation: "count_values",
497517
Node: n,
518+
Arguments: args,
498519
})
499520
// Param is the label to store the count value in.
500521
s.guaranteeLabel(
@@ -512,6 +533,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
512533
s.Operations = append(s.Operations, SourceOperation{
513534
Operation: "quantile",
514535
Node: n,
536+
Arguments: args,
515537
})
516538
if n.Without || !slices.Contains(n.Grouping, labels.MetricName) {
517539
s.excludeLabel("Aggregation removes metric name.", n.PosRange, labels.MetricName)
@@ -527,6 +549,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
527549
s.Operations = append(s.Operations, SourceOperation{
528550
Operation: "topk",
529551
Node: n,
552+
Arguments: args,
530553
})
531554
src = append(src, s)
532555
}
@@ -539,6 +562,7 @@ func walkAggregation(expr string, n *promParser.AggregateExpr) (src []Source) {
539562
s.Operations = append(s.Operations, SourceOperation{
540563
Operation: "bottomk",
541564
Node: n,
565+
Arguments: args,
542566
})
543567
src = append(src, s)
544568
}
@@ -810,6 +834,9 @@ If you're hoping to get instance specific labels this way and alert when some ta
810834
}
811835

812836
func parseCall(expr string, n *promParser.Call) (src []Source) {
837+
var args []string
838+
var exprs []promParser.Expr
839+
813840
var vt promParser.ValueType
814841
for i, e := range n.Args {
815842
if i >= len(n.Func.ArgTypes) {
@@ -820,16 +847,22 @@ func parseCall(expr string, n *promParser.Call) (src []Source) {
820847

821848
switch vt {
822849
case promParser.ValueTypeVector, promParser.ValueTypeMatrix:
823-
for _, es := range walkNode(expr, e) {
824-
es.Type = FuncSource
825-
es.Operations = append(es.Operations, SourceOperation{
826-
Operation: n.Func.Name,
827-
Node: n,
828-
})
829-
es.Position = e.PositionRange()
830-
src = append(src, parsePromQLFunc(es, expr, n))
831-
}
850+
exprs = append(exprs, e)
832851
case promParser.ValueTypeNone, promParser.ValueTypeScalar, promParser.ValueTypeString:
852+
args = append(args, e.String())
853+
}
854+
}
855+
856+
for _, e := range exprs {
857+
for _, es := range walkNode(expr, e) {
858+
es.Type = FuncSource
859+
es.Operations = append(es.Operations, SourceOperation{
860+
Operation: n.Func.Name,
861+
Node: n,
862+
Arguments: args,
863+
})
864+
es.Position = e.PositionRange()
865+
src = append(src, parsePromQLFunc(es, expr, n))
833866
}
834867
}
835868

@@ -841,6 +874,7 @@ func parseCall(expr string, n *promParser.Call) (src []Source) {
841874
{
842875
Operation: n.Func.Name,
843876
Node: n,
877+
Arguments: args,
844878
},
845879
},
846880
Position: n.PosRange,

‎internal/parser/utils/source_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ count by (dc) (
288288
) > 5`,
289289
`topk(10, prometheus_build_info*prometheus_ready)`,
290290
`bottomk(10, prometheus_build_info*prometheus_ready)`,
291+
`histogram_fraction(0, 0.1, metric)`,
291292
}
292293

293294
type Snapshot struct {

0 commit comments

Comments
 (0)