Skip to content

Commit d17c718

Browse files
prymitiveprymitive
authored
Merge pull request #1345 from cloudflare/tempaltes
More accurate template reports
2 parents 88fe80c + a911ab5 commit d17c718

File tree

8 files changed

+86
-54
lines changed

8 files changed

+86
-54
lines changed

‎cmd/pint/tests/0003_lint_workdir.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ Information: use humanize filters for the results (alerts/template)
9292
^^^^^^^^^^^^^^^^ `rate()` will produce results that are hard to read for humans.
9393
| [...]
9494
61 | summary: 'error rate: {{ $value }}'
95-
^^^^^^^^^^^^^^^^^^^^^^^^ Use one of humanize template functions to make the result more readable.
95+
^^^^^^^ Use one of humanize template functions to make the result more readable.
9696

9797
level=INFO msg="Problems found" Fatal=1 Warning=12 Information=1
9898
level=ERROR msg="Execution completed with error(s)" err="found 1 problem(s) with severity Bug or higher"

‎cmd/pint/tests/0076_ci_group_errors.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ Bug: template uses non-existent label (alerts/template)
261261
^^ Query is using aggregation with `by(foo)`, only labels included inside `by(...)` will be present on the results.
262262
| [...]
263263
30 | instance: 'sum on {{ $labels.instance }} is {{ $value }}'
264-
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Template is using `instance` label but the query results won't have this label.
264+
^^^^^^^^^ Template is using `instance` label but the query results won't have this label.
265265

266266
Bug: required label not set (rule/label)
267267
---> rules.yml:25-30 -> `template`

‎cmd/pint/tests/0087_dedup.txt‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,24 @@ Bug: template uses non-existent label (alerts/template)
1010
5 | expr: sum(up{job="bar"}) / sum(foo) / sum(bar)
1111
^^^ Query is using aggregation that removes all labels.
1212
| [...]
13-
12 | summary: "Server {{ $labels.instance }} in cluster {{ $labels.cluster }} has gone down"
14-
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Template is using `instance` label but the query results won't have this label.
13+
13 | dashboard: "https://grafana.example.com/dashboard?var-cluster={{ $labels.cluster }}&var-instance={{ $labels.cluster }}"
14+
^^^^^^^^ Template is using `cluster` label but the query results won't have this label.
1515

1616
Bug: template uses non-existent label (alerts/template)
1717
---> rules/01.yml:4-13 -> `foo`
1818
5 | expr: sum(up{job="bar"}) / sum(foo) / sum(bar)
1919
^^^ Query is using aggregation that removes all labels.
2020
| [...]
2121
12 | summary: "Server {{ $labels.instance }} in cluster {{ $labels.cluster }} has gone down"
22-
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Template is using `cluster` label but the query results won't have this label.
22+
^^^^^^^^ Template is using `cluster` label but the query results won't have this label.
2323

2424
Bug: template uses non-existent label (alerts/template)
2525
---> rules/01.yml:4-13 -> `foo`
2626
5 | expr: sum(up{job="bar"}) / sum(foo) / sum(bar)
2727
^^^ Query is using aggregation that removes all labels.
2828
| [...]
29-
13 | dashboard: "https://grafana.example.com/dashboard?var-cluster={{ $labels.cluster }}&var-instance={{ $labels.cluster }}"
30-
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Template is using `cluster` label but the query results won't have this label.
29+
12 | summary: "Server {{ $labels.instance }} in cluster {{ $labels.cluster }} has gone down"
30+
^^^^^^^^^ Template is using `instance` label but the query results won't have this label.
3131

3232
Warning: always firing alert (alerts/comparison)
3333
---> rules/01.yml:5 -> `foo`

‎cmd/pint/tests/0090_lint_min_severity_info.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ Information: use humanize filters for the results (alerts/template)
1111
^^^^^^^^^^^^^^^^ `rate()` will produce results that are hard to read for humans.
1212
| [...]
1313
7 | summary: 'error rate: {{ $value }}'
14-
^^^^^^^^^^^^^^^^^^^^^^^^ Use one of humanize template functions to make the result more readable.
14+
^^^^^^^ Use one of humanize template functions to make the result more readable.
1515

1616
level=INFO msg="Problems found" Information=1
1717
-- rules/0001.yml --

‎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.71.5
4+
5+
### Fixed
6+
7+
- [alerts/template](checks/alerts/template.md) check will now output more accurate comments.
8+
39
## v0.71.4
410

511
### Fixed

‎internal/checks/alerts_external_labels.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,8 @@ func checkExternalLabels(name, text string, externalLabels map[string]string) (l
130130
externalLabelsAliases := aliases.varAliases(".ExternalLabels")
131131
for _, v := range vars {
132132
for _, a := range externalLabelsAliases {
133-
if len(v) > 1 && v[0] == a {
134-
name := v[1]
133+
if len(v.value) > 1 && v.value[0] == a {
134+
name := v.value[1]
135135
if _, ok = done[name]; ok {
136136
continue
137137
}

‎internal/checks/alerts_template.go‎

Lines changed: 69 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ var (
3434
"{{$externalURL := .ExternalURL}}",
3535
"{{$value := .Value}}",
3636
}
37+
templateDefsLen = len(strings.Join(templateDefs, ""))
3738

3839
templateFuncMap = textTemplate.FuncMap{
3940
"query": dummyFuncMap,
@@ -180,44 +181,59 @@ func (c TemplateCheck) Check(ctx context.Context, _ discovery.Path, rule parser.
180181
},
181182
})
182183
}
183-
184184
problems = append(problems, c.checkQueryLabels(rule, annotation, src)...)
185-
186-
if hasValue(annotation.Key.Value, annotation.Value.Value) && !hasHumanize(annotation.Key.Value, annotation.Value.Value) {
187-
problems = append(problems, c.checkHumanizeIsNeeded(rule.AlertingRule.Expr, annotation.Value)...)
188-
}
185+
problems = append(problems, c.checkHumanizeIsNeeded(rule.AlertingRule.Expr, annotation)...)
189186
}
190187
}
191188

192189
return problems
193190
}
194191

195-
func (c TemplateCheck) checkHumanizeIsNeeded(expr parser.PromQLExpr, ann *parser.YamlNode) (problems []Problem) {
192+
func (c TemplateCheck) checkHumanizeIsNeeded(expr parser.PromQLExpr, ann *parser.YamlKeyValue) (problems []Problem) {
193+
if !hasValue(ann.Key.Value, ann.Value.Value) {
194+
return problems
195+
}
196+
if hasHumanize(ann.Key.Value, ann.Value.Value) {
197+
return problems
198+
}
199+
vars, aliases, ok := findTemplateVariables(ann.Key.Value, ann.Value.Value)
200+
if !ok {
201+
return problems
202+
}
196203
for _, call := range utils.HasOuterRate(expr.Query) {
204+
dgs := []diags.Diagnostic{
205+
{
206+
Message: fmt.Sprintf("`%s()` will produce results that are hard to read for humans.", call.Func.Name),
207+
Pos: expr.Value.Pos,
208+
FirstColumn: int(call.PosRange.Start) + 1,
209+
LastColumn: int(call.PosRange.End),
210+
},
211+
}
212+
labelsAliases := aliases.varAliases(".Value")
213+
for _, v := range vars {
214+
for _, a := range labelsAliases {
215+
if v.value[0] == a {
216+
dgs = append(dgs, diags.Diagnostic{
217+
Message: "Use one of humanize template functions to make the result more readable.",
218+
Pos: ann.Value.Pos,
219+
FirstColumn: v.column,
220+
LastColumn: v.column + len(v.value[0]),
221+
})
222+
}
223+
}
224+
}
225+
197226
problems = append(problems, Problem{
198227
Anchor: AnchorAfter,
199228
Lines: diags.LineRange{
200-
First: min(expr.Value.Lines.First, ann.Lines.First),
201-
Last: max(expr.Value.Lines.Last, ann.Lines.Last),
229+
First: min(expr.Value.Lines.First, ann.Value.Lines.First),
230+
Last: max(expr.Value.Lines.Last, ann.Value.Lines.Last),
202231
},
203-
Reporter: c.Reporter(),
204-
Summary: "use humanize filters for the results",
205-
Details: TemplateCheckReferenceDetails,
206-
Diagnostics: []diags.Diagnostic{
207-
{
208-
Message: fmt.Sprintf("`%s()` will produce results that are hard to read for humans.", call.Func.Name),
209-
Pos: expr.Value.Pos,
210-
FirstColumn: int(call.PosRange.Start) + 1,
211-
LastColumn: int(call.PosRange.End),
212-
},
213-
{
214-
Message: "Use one of humanize template functions to make the result more readable.",
215-
Pos: ann.Pos,
216-
FirstColumn: 1, // FIXME use $value offset
217-
LastColumn: len(ann.Value), // FIXME use $value offset
218-
},
219-
},
220-
Severity: Information,
232+
Reporter: c.Reporter(),
233+
Summary: "use humanize filters for the results",
234+
Details: TemplateCheckReferenceDetails,
235+
Diagnostics: dgs,
236+
Severity: Information,
221237
})
222238
}
223239
return problems
@@ -293,7 +309,7 @@ func checkForValueInLabels(name, text string) (msgs []string) {
293309
func containsAliasedNode(am aliasMap, node parse.Node, alias string) (string, bool) {
294310
valAliases := am.varAliases(alias)
295311
for _, vars := range getVariables(node) {
296-
for _, v := range vars {
312+
for _, v := range vars.value {
297313
for _, a := range valAliases {
298314
if v == a {
299315
return v, true
@@ -389,10 +405,10 @@ func getAliases(node parse.Node, aliases *aliasMap) {
389405
for _, k := range getVariables(arg) {
390406
for _, d := range n.Pipe.Decl {
391407
for _, v := range getVariables(d) {
392-
if _, ok := aliases.aliases[k[0]]; !ok {
393-
aliases.aliases[k[0]] = map[string]struct{}{}
408+
if _, ok := aliases.aliases[k.value[0]]; !ok {
409+
aliases.aliases[k.value[0]] = map[string]struct{}{}
394410
}
395-
aliases.aliases[k[0]][v[0]] = struct{}{}
411+
aliases.aliases[k.value[0]][v.value[0]] = struct{}{}
396412
}
397413
}
398414
}
@@ -402,7 +418,12 @@ func getAliases(node parse.Node, aliases *aliasMap) {
402418
}
403419
}
404420

405-
func getVariables(node parse.Node) (vars [][]string) {
421+
type tmplVar struct {
422+
value []string
423+
column int
424+
}
425+
426+
func getVariables(node parse.Node) (vars []tmplVar) {
406427
switch n := node.(type) {
407428
case *parse.ActionNode:
408429
if len(n.Pipe.Decl) == 0 && len(n.Pipe.Cmds) > 0 {
@@ -414,15 +435,20 @@ func getVariables(node parse.Node) (vars [][]string) {
414435
}
415436
case *parse.FieldNode:
416437
n.Ident[0] = "." + n.Ident[0]
417-
vars = append(vars, n.Ident)
438+
vars = append(vars, tmplVar{
439+
value: n.Ident,
440+
column: int(n.Pos) + 1 - templateDefsLen,
441+
})
418442
case *parse.VariableNode:
419-
vars = append(vars, n.Ident)
443+
vars = append(vars, tmplVar{
444+
value: n.Ident,
445+
column: int(n.Pos) + 1 - templateDefsLen,
446+
})
420447
}
421-
422448
return vars
423449
}
424450

425-
func findTemplateVariables(name, text string) (vars [][]string, aliases aliasMap, ok bool) {
451+
func findTemplateVariables(name, text string) (vars []tmplVar, aliases aliasMap, ok bool) {
426452
t, err := textTemplate.
427453
New(name).
428454
Funcs(templateFuncMap).
@@ -452,16 +478,16 @@ func (c TemplateCheck) checkQueryLabels(rule parser.Rule, label *parser.YamlKeyV
452478
labelsAliases := aliases.varAliases(".Labels")
453479
for _, v := range vars {
454480
for _, a := range labelsAliases {
455-
if len(v) > 1 && v[0] == a {
456-
if _, ok := done[v[1]]; ok {
481+
if len(v.value) > 1 && v.value[0] == a {
482+
if _, ok := done[v.value[1]]; ok {
457483
continue
458484
}
459485
for _, s := range src {
460486
if s.IsDead {
461487
continue
462488
}
463-
if !s.CanHaveLabel(v[1]) {
464-
er := s.LabelExcludeReason(v[1])
489+
if !s.CanHaveLabel(v.value[1]) {
490+
er := s.LabelExcludeReason(v.value[1])
465491
problems = append(problems, Problem{
466492
Anchor: AnchorAfter,
467493
Lines: rule.Lines,
@@ -471,10 +497,10 @@ func (c TemplateCheck) checkQueryLabels(rule parser.Rule, label *parser.YamlKeyV
471497
Severity: Bug,
472498
Diagnostics: []diags.Diagnostic{
473499
{
474-
Message: fmt.Sprintf("Template is using `%s` label but the query results won't have this label.", v[1]),
500+
Message: fmt.Sprintf("Template is using `%s` label but the query results won't have this label.", v.value[1]),
475501
Pos: label.Value.Pos,
476-
FirstColumn: 1,
477-
LastColumn: len(label.Value.Value),
502+
FirstColumn: v.column,
503+
LastColumn: v.column + len(v.value[1]),
478504
},
479505
{
480506
Message: er.Reason,
@@ -488,7 +514,7 @@ func (c TemplateCheck) checkQueryLabels(rule parser.Rule, label *parser.YamlKeyV
488514
}
489515
}
490516
NEXT:
491-
done[v[1]] = struct{}{}
517+
done[v.value[1]] = struct{}{}
492518
}
493519
}
494520
}

‎internal/diags/position.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ type PositionRange struct {
3535

3636
type PositionRanges []PositionRange
3737

38-
func (prs PositionRanges) Len() (l int) { // FIXME remove
38+
func (prs PositionRanges) Len() (l int) {
3939
for _, pr := range prs {
4040
l += pr.LastColumn - pr.FirstColumn + 1
4141
}

0 commit comments

Comments
 (0)