Skip to content

Commit b9f84cc

Browse files
prymitiveprymitive
committed
Warn about unused comments
1 parent df1c22b commit b9f84cc

File tree

5 files changed

+253
-16
lines changed

5 files changed

+253
-16
lines changed

‎cmd/pint/tests/0160_ci_comment_edit.txt

+4-1
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,13 @@ level=WARN msg="No results for Prometheus uptime metric, you might have set upti
4646
level=WARN msg="Using dummy Prometheus uptime metric results with no gaps" name=prom metric=up
4747
level=WARN msg="No results for Prometheus uptime metric, you might have set uptime config option to a missing metric, please check your config" name=prom metric=up
4848
level=WARN msg="Using dummy Prometheus uptime metric results with no gaps" name=prom metric=up
49-
level=INFO msg="Problems found" Bug=2
49+
level=INFO msg="Problems found" Bug=2 Warning=1
5050
rules.yml:8 Bug: `prom` Prometheus server at http://127.0.0.1:7160 didn't have any series for `up` metric in the last 1w. (promql/series)
5151
8 | expr: up == 0
5252

53+
rules.yml:8 Warning: pint disable comment `promql/series(xxx)` check doesn't match any selector in this query (promql/series)
54+
8 | expr: up == 0
55+
5356
rules.yml:16 Bug: `prom` Prometheus server at http://127.0.0.1:7160 didn't have any series for `up` metric in the last 1w. (promql/series)
5457
16 | expr: up == 0
5558

‎docs/changelog.md

+7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
# Changelog
22

3+
## v0.62.0
4+
5+
### Added
6+
7+
- [promql/series](checks/promql/series.md) check will now generate warnings if there are `# pint disable`
8+
or `# pint rule/set` comments that are not matching any valid query selector or Prometheus server.
9+
310
## v0.61.2
411

512
### Fixed

‎internal/checks/base_test.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func simpleProm(name, uri string, timeout time.Duration, required bool) *promapi
7575
"up",
7676
[]*regexp.Regexp{},
7777
[]*regexp.Regexp{},
78-
[]string{},
78+
[]string{"mytag"},
7979
)
8080
}
8181

@@ -129,8 +129,10 @@ func runTests(t *testing.T, testCases []checkTest) {
129129
uri = srv.URL
130130
}
131131

132+
var proms []*promapi.FailoverGroup
132133
prom := tc.prometheus(uri)
133134
if prom != nil {
135+
proms = append(proms, prom)
134136
reg := prometheus.NewRegistry()
135137
prom.StartWorkers(reg)
136138
defer prom.Close(reg)
@@ -139,7 +141,7 @@ func runTests(t *testing.T, testCases []checkTest) {
139141
entries, err := parseContent(tc.content)
140142
require.NoError(t, err, "cannot parse rule content")
141143
for _, entry := range entries {
142-
ctx := context.Background()
144+
ctx := context.WithValue(context.Background(), promapi.AllPrometheusServers, proms)
143145
if tc.ctx != nil {
144146
ctx = tc.ctx()
145147
}

‎internal/checks/promql_series.go

+131-13
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ To fully validate your changes it's best to first deploy the rules that generate
7070
SeriesCheckCommonProblemDetails = `[Click here](https://cloudflare.github.io/pint/checks/promql/series.html#common-problems) to see a list of common problems that might cause this.`
7171
SeriesCheckMinAgeDetails = `You have a comment that tells pint how long can a metric be missing before it warns you about it but this comment is not formatted correctly.
7272
[Click here](https://cloudflare.github.io/pint/checks/promql/series.html#min-age) to see supported syntax.`
73+
SeriesCheckUnusedDisableComment = "One of the `# pint disable promql/series` comments used in this rule doesn't have any effect and won't disable anything.\n[Click here](https://cloudflare.github.io/pint/checks/promql/series.html#how-to-disable-it) to see docs about disable comment syntax."
74+
SeriesCheckUnusedRuleSetComment = "One of the `# pint rule/set promql/series` comments used in this rule doesn't have any effect. Make sure that the comment targets labels that are used in the rule query.\n[Click here](https://cloudflare.github.io/pint/checks/promql/series.html#ignorelabel-value) for docs about comment syntax."
7375
)
7476

7577
func NewSeriesCheck(prom *promapi.FailoverGroup) SeriesCheck {
@@ -118,8 +120,10 @@ func (c SeriesCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Ru
118120

119121
params := promapi.NewRelativeRange(settings.lookbackRangeDuration, settings.lookbackStepDuration)
120122

123+
selectors := getSelectors(expr.Query)
124+
121125
done := map[string]bool{}
122-
for _, selector := range getSelectors(expr.Query) {
126+
for _, selector := range selectors {
123127
if _, ok := done[selector.String()]; ok {
124128
continue
125129
}
@@ -531,6 +535,25 @@ func (c SeriesCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Ru
531535
}
532536
}
533537

538+
for _, disable := range orphanedDisableComments(ctx, rule, selectors) {
539+
problems = append(problems, Problem{
540+
Lines: expr.Value.Lines,
541+
Reporter: c.Reporter(),
542+
Text: fmt.Sprintf("pint %s comment `%s` check doesn't match any selector in this query", comments.DisableComment, disable.Match),
543+
Details: SeriesCheckUnusedDisableComment,
544+
Severity: Warning,
545+
})
546+
}
547+
for _, ruleSet := range orphanedRuleSetComments(c.Reporter(), rule, selectors) {
548+
problems = append(problems, Problem{
549+
Lines: expr.Value.Lines,
550+
Reporter: c.Reporter(),
551+
Text: fmt.Sprintf("pint %s comment `%s` doesn't match any label matcher in this query", comments.RuleSetComment, ruleSet.Value),
552+
Details: SeriesCheckUnusedRuleSetComment,
553+
Severity: Warning,
554+
})
555+
}
556+
534557
return problems
535558
}
536559

@@ -612,12 +635,7 @@ func (c SeriesCheck) instantSeriesCount(ctx context.Context, query string) (int,
612635

613636
func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelector) (minAge time.Duration, problems []Problem) {
614637
minAge = time.Hour * 2
615-
bareSelector := stripLabels(selector)
616-
prefixes := []string{
617-
c.Reporter() + " min-age ",
618-
fmt.Sprintf("%s(%s) min-age ", c.Reporter(), bareSelector.String()),
619-
fmt.Sprintf("%s(%s) min-age ", c.Reporter(), selector.String()),
620-
}
638+
prefixes := ruleSetMinAgePrefixes(c.Reporter(), selector)
621639
for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) {
622640
for _, prefix := range prefixes {
623641
if !strings.HasPrefix(ruleSet.Value, prefix) {
@@ -643,12 +661,7 @@ func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelec
643661
}
644662

645663
func (c SeriesCheck) isLabelValueIgnored(rule parser.Rule, selector promParser.VectorSelector, labelName string) bool {
646-
bareSelector := stripLabels(selector)
647-
values := []string{
648-
fmt.Sprintf("%s ignore/label-value %s", c.Reporter(), labelName),
649-
fmt.Sprintf("%s(%s) ignore/label-value %s", c.Reporter(), bareSelector.String(), labelName),
650-
fmt.Sprintf("%s(%s) ignore/label-value %s", c.Reporter(), selector.String(), labelName),
651-
}
664+
values := ruleSetIgnoreLabelValValues(c.Reporter(), labelName, selector)
652665
for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) {
653666
for _, val := range values {
654667
if ruleSet.Value == val {
@@ -750,6 +763,111 @@ func isDisabled(rule parser.Rule, selector promParser.VectorSelector) bool {
750763
return false
751764
}
752765

766+
func orphanedDisableComments(ctx context.Context, rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.Disable) {
767+
var promNames, promTags []string
768+
if val := ctx.Value(promapi.AllPrometheusServers); val != nil {
769+
for _, server := range val.([]*promapi.FailoverGroup) {
770+
promNames = append(promNames, server.Name())
771+
promTags = append(promTags, server.Tags()...)
772+
}
773+
}
774+
775+
for _, disable := range comments.Only[comments.Disable](rule.Comments, comments.DisableType) {
776+
match := strings.TrimSuffix(strings.TrimPrefix(disable.Match, SeriesCheckName+"("), ")")
777+
// Skip matching tags.
778+
if strings.HasPrefix(match, "+") && slices.Contains(promTags, strings.TrimPrefix(match, "+")) {
779+
continue
780+
}
781+
// Skip matching Prometheus servers.
782+
if slices.Contains(promNames, match) {
783+
continue
784+
}
785+
var wasUsed bool
786+
if !strings.HasPrefix(disable.Match, SeriesCheckName+"(") || !strings.HasSuffix(disable.Match, ")") {
787+
continue
788+
}
789+
for _, selector := range selectors {
790+
// try full string or name match first
791+
if match == selector.String() || match == selector.Name {
792+
wasUsed = true
793+
goto NEXT
794+
}
795+
// then try matchers
796+
m, err := promParser.ParseMetricSelector(match)
797+
if err != nil {
798+
continue
799+
}
800+
var isMismatch bool
801+
for _, l := range m {
802+
var isMatch bool
803+
for _, s := range selector.LabelMatchers {
804+
if s.Type == l.Type && s.Name == l.Name && s.Value == l.Value {
805+
isMatch = true
806+
break
807+
}
808+
}
809+
if !isMatch {
810+
isMismatch = true
811+
break
812+
}
813+
}
814+
if !isMismatch {
815+
wasUsed = true
816+
}
817+
}
818+
NEXT:
819+
if !wasUsed {
820+
orhpaned = append(orhpaned, disable)
821+
}
822+
}
823+
return orhpaned
824+
}
825+
826+
func ruleSetIgnoreLabelValValues(reporter, labelName string, selector promParser.VectorSelector) []string {
827+
bareSelector := stripLabels(selector)
828+
return []string{
829+
fmt.Sprintf("%s ignore/label-value %s", reporter, labelName),
830+
fmt.Sprintf("%s(%s) ignore/label-value %s", reporter, bareSelector.String(), labelName),
831+
fmt.Sprintf("%s(%s) ignore/label-value %s", reporter, selector.String(), labelName),
832+
}
833+
}
834+
835+
func ruleSetMinAgePrefixes(reporter string, selector promParser.VectorSelector) []string {
836+
bareSelector := stripLabels(selector)
837+
return []string{
838+
reporter + " min-age ",
839+
fmt.Sprintf("%s(%s) min-age ", reporter, bareSelector.String()),
840+
fmt.Sprintf("%s(%s) min-age ", reporter, selector.String()),
841+
}
842+
}
843+
844+
func orphanedRuleSetComments(reporter string, rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.RuleSet) {
845+
for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) {
846+
var used bool
847+
for _, selector := range selectors {
848+
for _, lm := range selector.LabelMatchers {
849+
for _, val := range ruleSetIgnoreLabelValValues(reporter, lm.Name, selector) {
850+
if ruleSet.Value == val {
851+
used = true
852+
goto NEXT
853+
}
854+
}
855+
for _, val := range ruleSetMinAgePrefixes(reporter, selector) {
856+
if strings.HasPrefix(ruleSet.Value, val) {
857+
used = true
858+
goto NEXT
859+
}
860+
}
861+
}
862+
}
863+
NEXT:
864+
if !used {
865+
orhpaned = append(orhpaned, ruleSet)
866+
}
867+
}
868+
return orhpaned
869+
}
870+
753871
func sinceDesc(t time.Time) (s string) {
754872
dur := time.Since(t)
755873
if dur > time.Hour*24 {

‎internal/checks/promql_series_test.go

+107
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/prometheus/common/model"
1010

1111
"github.com/cloudflare/pint/internal/checks"
12+
"github.com/cloudflare/pint/internal/comments"
1213
"github.com/cloudflare/pint/internal/parser"
1314
"github.com/cloudflare/pint/internal/promapi"
1415
)
@@ -61,6 +62,14 @@ func metricIgnored(metric, check, re string) string {
6162
return fmt.Sprintf("Metric name `%s` matches `%s` check ignore regexp `%s`.", metric, check, re)
6263
}
6364

65+
func unusedDisableText(m string) string {
66+
return fmt.Sprintf("pint %s comment `%s` check doesn't match any selector in this query", comments.DisableComment, m)
67+
}
68+
69+
func unusedRuleSetText(m string) string {
70+
return fmt.Sprintf("pint %s comment `%s` doesn't match any label matcher in this query", comments.RuleSetComment, m)
71+
}
72+
6473
func TestSeriesCheck(t *testing.T) {
6574
testCases := []checkTest{
6675
{
@@ -1726,6 +1735,16 @@ func TestSeriesCheck(t *testing.T) {
17261735
Details: checks.SeriesCheckCommonProblemDetails,
17271736
Severity: checks.Bug,
17281737
},
1738+
{
1739+
Lines: parser.LineRange{
1740+
First: 4,
1741+
Last: 4,
1742+
},
1743+
Reporter: checks.SeriesCheckName,
1744+
Text: unusedRuleSetText("promql/series(bar) min-age 5d"),
1745+
Details: checks.SeriesCheckUnusedRuleSetComment,
1746+
Severity: checks.Warning,
1747+
},
17291748
}
17301749
},
17311750
mocks: []*prometheusMock{
@@ -2975,6 +2994,16 @@ func TestSeriesCheck(t *testing.T) {
29752994
Details: checks.SeriesCheckCommonProblemDetails,
29762995
Severity: checks.Bug,
29772996
},
2997+
{
2998+
Lines: parser.LineRange{
2999+
First: 4,
3000+
Last: 4,
3001+
},
3002+
Reporter: checks.SeriesCheckName,
3003+
Text: unusedDisableText(`promql/series({job="foo"})`),
3004+
Details: checks.SeriesCheckUnusedDisableComment,
3005+
Severity: checks.Warning,
3006+
},
29783007
}
29793008
},
29803009
mocks: []*prometheusMock{
@@ -3009,6 +3038,16 @@ func TestSeriesCheck(t *testing.T) {
30093038
Details: checks.SeriesCheckCommonProblemDetails,
30103039
Severity: checks.Bug,
30113040
},
3041+
{
3042+
Lines: parser.LineRange{
3043+
First: 4,
3044+
Last: 4,
3045+
},
3046+
Reporter: checks.SeriesCheckName,
3047+
Text: unusedDisableText(`promql/series(notfound{job=foo})`),
3048+
Details: checks.SeriesCheckUnusedDisableComment,
3049+
Severity: checks.Warning,
3050+
},
30123051
}
30133052
},
30143053
mocks: []*prometheusMock{
@@ -3065,6 +3104,16 @@ func TestSeriesCheck(t *testing.T) {
30653104
Details: checks.SeriesCheckCommonProblemDetails,
30663105
Severity: checks.Bug,
30673106
},
3107+
{
3108+
Lines: parser.LineRange{
3109+
First: 4,
3110+
Last: 4,
3111+
},
3112+
Reporter: checks.SeriesCheckName,
3113+
Text: unusedDisableText(`promql/series(notfound{job="foo"})`),
3114+
Details: checks.SeriesCheckUnusedDisableComment,
3115+
Severity: checks.Warning,
3116+
},
30683117
}
30693118
},
30703119
mocks: []*prometheusMock{
@@ -3423,6 +3472,64 @@ func TestSeriesCheck(t *testing.T) {
34233472
},
34243473
},
34253474
},
3475+
{
3476+
description: "disable comment for prometheus server",
3477+
content: `
3478+
# pint disable promql/series(prom)
3479+
- record: my_series
3480+
expr: vector(1)
3481+
`,
3482+
checker: newSeriesCheck,
3483+
prometheus: newSimpleProm,
3484+
problems: noProblems,
3485+
},
3486+
{
3487+
description: "disable comment for prometheus server",
3488+
content: `
3489+
# pint disable promql/series(+mytag)
3490+
- record: my_series
3491+
expr: vector(1)
3492+
`,
3493+
checker: newSeriesCheck,
3494+
prometheus: newSimpleProm,
3495+
problems: noProblems,
3496+
},
3497+
{
3498+
description: "disable comment for a valid series",
3499+
content: `
3500+
# pint disable promql/series(foo)
3501+
- record: my_series
3502+
expr: count(foo)
3503+
`,
3504+
checker: newSeriesCheck,
3505+
prometheus: newSimpleProm,
3506+
problems: noProblems,
3507+
},
3508+
{
3509+
description: "disable comment for a mismatched series",
3510+
content: `
3511+
# pint disable promql/series(bar)
3512+
# pint disable promql/series(foo)
3513+
- record: my_series
3514+
expr: count(bar)
3515+
`,
3516+
checker: newSeriesCheck,
3517+
prometheus: newSimpleProm,
3518+
problems: func(_ string) []checks.Problem {
3519+
return []checks.Problem{
3520+
{
3521+
Lines: parser.LineRange{
3522+
First: 5,
3523+
Last: 5,
3524+
},
3525+
Reporter: checks.SeriesCheckName,
3526+
Text: unusedDisableText("promql/series(foo)"),
3527+
Details: checks.SeriesCheckUnusedDisableComment,
3528+
Severity: checks.Warning,
3529+
},
3530+
}
3531+
},
3532+
},
34263533
}
34273534
runTests(t, testCases)
34283535
}

0 commit comments

Comments
 (0)