From b9f84cc589518bc9c1085a2ef476be397677d08e Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Tue, 9 Jul 2024 16:51:47 +0100 Subject: [PATCH] Warn about unused comments --- cmd/pint/tests/0160_ci_comment_edit.txt | 5 +- docs/changelog.md | 7 ++ internal/checks/base_test.go | 6 +- internal/checks/promql_series.go | 144 +++++++++++++++++++++--- internal/checks/promql_series_test.go | 107 ++++++++++++++++++ 5 files changed, 253 insertions(+), 16 deletions(-) diff --git a/cmd/pint/tests/0160_ci_comment_edit.txt b/cmd/pint/tests/0160_ci_comment_edit.txt index 9b7e2c55..614fab9b 100644 --- a/cmd/pint/tests/0160_ci_comment_edit.txt +++ b/cmd/pint/tests/0160_ci_comment_edit.txt @@ -46,10 +46,13 @@ level=WARN msg="No results for Prometheus uptime metric, you might have set upti level=WARN msg="Using dummy Prometheus uptime metric results with no gaps" name=prom metric=up 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 level=WARN msg="Using dummy Prometheus uptime metric results with no gaps" name=prom metric=up -level=INFO msg="Problems found" Bug=2 +level=INFO msg="Problems found" Bug=2 Warning=1 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) 8 | expr: up == 0 +rules.yml:8 Warning: pint disable comment `promql/series(xxx)` check doesn't match any selector in this query (promql/series) + 8 | expr: up == 0 + 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) 16 | expr: up == 0 diff --git a/docs/changelog.md b/docs/changelog.md index 179857eb..160a7ddd 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,5 +1,12 @@ # Changelog +## v0.62.0 + +### Added + +- [promql/series](checks/promql/series.md) check will now generate warnings if there are `# pint disable` + or `# pint rule/set` comments that are not matching any valid query selector or Prometheus server. + ## v0.61.2 ### Fixed diff --git a/internal/checks/base_test.go b/internal/checks/base_test.go index 35a761ae..3fa41d21 100644 --- a/internal/checks/base_test.go +++ b/internal/checks/base_test.go @@ -75,7 +75,7 @@ func simpleProm(name, uri string, timeout time.Duration, required bool) *promapi "up", []*regexp.Regexp{}, []*regexp.Regexp{}, - []string{}, + []string{"mytag"}, ) } @@ -129,8 +129,10 @@ func runTests(t *testing.T, testCases []checkTest) { uri = srv.URL } + var proms []*promapi.FailoverGroup prom := tc.prometheus(uri) if prom != nil { + proms = append(proms, prom) reg := prometheus.NewRegistry() prom.StartWorkers(reg) defer prom.Close(reg) @@ -139,7 +141,7 @@ func runTests(t *testing.T, testCases []checkTest) { entries, err := parseContent(tc.content) require.NoError(t, err, "cannot parse rule content") for _, entry := range entries { - ctx := context.Background() + ctx := context.WithValue(context.Background(), promapi.AllPrometheusServers, proms) if tc.ctx != nil { ctx = tc.ctx() } diff --git a/internal/checks/promql_series.go b/internal/checks/promql_series.go index f3875d61..68ab3ad7 100644 --- a/internal/checks/promql_series.go +++ b/internal/checks/promql_series.go @@ -70,6 +70,8 @@ To fully validate your changes it's best to first deploy the rules that generate 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.` 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. [Click here](https://cloudflare.github.io/pint/checks/promql/series.html#min-age) to see supported syntax.` + 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." + 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." ) func NewSeriesCheck(prom *promapi.FailoverGroup) SeriesCheck { @@ -118,8 +120,10 @@ func (c SeriesCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Ru params := promapi.NewRelativeRange(settings.lookbackRangeDuration, settings.lookbackStepDuration) + selectors := getSelectors(expr.Query) + done := map[string]bool{} - for _, selector := range getSelectors(expr.Query) { + for _, selector := range selectors { if _, ok := done[selector.String()]; ok { continue } @@ -531,6 +535,25 @@ func (c SeriesCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Ru } } + for _, disable := range orphanedDisableComments(ctx, rule, selectors) { + problems = append(problems, Problem{ + Lines: expr.Value.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("pint %s comment `%s` check doesn't match any selector in this query", comments.DisableComment, disable.Match), + Details: SeriesCheckUnusedDisableComment, + Severity: Warning, + }) + } + for _, ruleSet := range orphanedRuleSetComments(c.Reporter(), rule, selectors) { + problems = append(problems, Problem{ + Lines: expr.Value.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("pint %s comment `%s` doesn't match any label matcher in this query", comments.RuleSetComment, ruleSet.Value), + Details: SeriesCheckUnusedRuleSetComment, + Severity: Warning, + }) + } + return problems } @@ -612,12 +635,7 @@ func (c SeriesCheck) instantSeriesCount(ctx context.Context, query string) (int, func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelector) (minAge time.Duration, problems []Problem) { minAge = time.Hour * 2 - bareSelector := stripLabels(selector) - prefixes := []string{ - c.Reporter() + " min-age ", - fmt.Sprintf("%s(%s) min-age ", c.Reporter(), bareSelector.String()), - fmt.Sprintf("%s(%s) min-age ", c.Reporter(), selector.String()), - } + prefixes := ruleSetMinAgePrefixes(c.Reporter(), selector) for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) { for _, prefix := range prefixes { if !strings.HasPrefix(ruleSet.Value, prefix) { @@ -643,12 +661,7 @@ func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelec } func (c SeriesCheck) isLabelValueIgnored(rule parser.Rule, selector promParser.VectorSelector, labelName string) bool { - bareSelector := stripLabels(selector) - values := []string{ - fmt.Sprintf("%s ignore/label-value %s", c.Reporter(), labelName), - fmt.Sprintf("%s(%s) ignore/label-value %s", c.Reporter(), bareSelector.String(), labelName), - fmt.Sprintf("%s(%s) ignore/label-value %s", c.Reporter(), selector.String(), labelName), - } + values := ruleSetIgnoreLabelValValues(c.Reporter(), labelName, selector) for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) { for _, val := range values { if ruleSet.Value == val { @@ -750,6 +763,111 @@ func isDisabled(rule parser.Rule, selector promParser.VectorSelector) bool { return false } +func orphanedDisableComments(ctx context.Context, rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.Disable) { + var promNames, promTags []string + if val := ctx.Value(promapi.AllPrometheusServers); val != nil { + for _, server := range val.([]*promapi.FailoverGroup) { + promNames = append(promNames, server.Name()) + promTags = append(promTags, server.Tags()...) + } + } + + for _, disable := range comments.Only[comments.Disable](rule.Comments, comments.DisableType) { + match := strings.TrimSuffix(strings.TrimPrefix(disable.Match, SeriesCheckName+"("), ")") + // Skip matching tags. + if strings.HasPrefix(match, "+") && slices.Contains(promTags, strings.TrimPrefix(match, "+")) { + continue + } + // Skip matching Prometheus servers. + if slices.Contains(promNames, match) { + continue + } + var wasUsed bool + if !strings.HasPrefix(disable.Match, SeriesCheckName+"(") || !strings.HasSuffix(disable.Match, ")") { + continue + } + for _, selector := range selectors { + // try full string or name match first + if match == selector.String() || match == selector.Name { + wasUsed = true + goto NEXT + } + // then try matchers + m, err := promParser.ParseMetricSelector(match) + if err != nil { + continue + } + var isMismatch bool + for _, l := range m { + var isMatch bool + for _, s := range selector.LabelMatchers { + if s.Type == l.Type && s.Name == l.Name && s.Value == l.Value { + isMatch = true + break + } + } + if !isMatch { + isMismatch = true + break + } + } + if !isMismatch { + wasUsed = true + } + } + NEXT: + if !wasUsed { + orhpaned = append(orhpaned, disable) + } + } + return orhpaned +} + +func ruleSetIgnoreLabelValValues(reporter, labelName string, selector promParser.VectorSelector) []string { + bareSelector := stripLabels(selector) + return []string{ + fmt.Sprintf("%s ignore/label-value %s", reporter, labelName), + fmt.Sprintf("%s(%s) ignore/label-value %s", reporter, bareSelector.String(), labelName), + fmt.Sprintf("%s(%s) ignore/label-value %s", reporter, selector.String(), labelName), + } +} + +func ruleSetMinAgePrefixes(reporter string, selector promParser.VectorSelector) []string { + bareSelector := stripLabels(selector) + return []string{ + reporter + " min-age ", + fmt.Sprintf("%s(%s) min-age ", reporter, bareSelector.String()), + fmt.Sprintf("%s(%s) min-age ", reporter, selector.String()), + } +} + +func orphanedRuleSetComments(reporter string, rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.RuleSet) { + for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) { + var used bool + for _, selector := range selectors { + for _, lm := range selector.LabelMatchers { + for _, val := range ruleSetIgnoreLabelValValues(reporter, lm.Name, selector) { + if ruleSet.Value == val { + used = true + goto NEXT + } + } + for _, val := range ruleSetMinAgePrefixes(reporter, selector) { + if strings.HasPrefix(ruleSet.Value, val) { + used = true + goto NEXT + } + } + } + } + NEXT: + if !used { + orhpaned = append(orhpaned, ruleSet) + } + } + return orhpaned +} + func sinceDesc(t time.Time) (s string) { dur := time.Since(t) if dur > time.Hour*24 { diff --git a/internal/checks/promql_series_test.go b/internal/checks/promql_series_test.go index 4fa94b77..509827b6 100644 --- a/internal/checks/promql_series_test.go +++ b/internal/checks/promql_series_test.go @@ -9,6 +9,7 @@ import ( "github.com/prometheus/common/model" "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/promapi" ) @@ -61,6 +62,14 @@ func metricIgnored(metric, check, re string) string { return fmt.Sprintf("Metric name `%s` matches `%s` check ignore regexp `%s`.", metric, check, re) } +func unusedDisableText(m string) string { + return fmt.Sprintf("pint %s comment `%s` check doesn't match any selector in this query", comments.DisableComment, m) +} + +func unusedRuleSetText(m string) string { + return fmt.Sprintf("pint %s comment `%s` doesn't match any label matcher in this query", comments.RuleSetComment, m) +} + func TestSeriesCheck(t *testing.T) { testCases := []checkTest{ { @@ -1726,6 +1735,16 @@ func TestSeriesCheck(t *testing.T) { Details: checks.SeriesCheckCommonProblemDetails, Severity: checks.Bug, }, + { + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.SeriesCheckName, + Text: unusedRuleSetText("promql/series(bar) min-age 5d"), + Details: checks.SeriesCheckUnusedRuleSetComment, + Severity: checks.Warning, + }, } }, mocks: []*prometheusMock{ @@ -2975,6 +2994,16 @@ func TestSeriesCheck(t *testing.T) { Details: checks.SeriesCheckCommonProblemDetails, Severity: checks.Bug, }, + { + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.SeriesCheckName, + Text: unusedDisableText(`promql/series({job="foo"})`), + Details: checks.SeriesCheckUnusedDisableComment, + Severity: checks.Warning, + }, } }, mocks: []*prometheusMock{ @@ -3009,6 +3038,16 @@ func TestSeriesCheck(t *testing.T) { Details: checks.SeriesCheckCommonProblemDetails, Severity: checks.Bug, }, + { + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.SeriesCheckName, + Text: unusedDisableText(`promql/series(notfound{job=foo})`), + Details: checks.SeriesCheckUnusedDisableComment, + Severity: checks.Warning, + }, } }, mocks: []*prometheusMock{ @@ -3065,6 +3104,16 @@ func TestSeriesCheck(t *testing.T) { Details: checks.SeriesCheckCommonProblemDetails, Severity: checks.Bug, }, + { + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.SeriesCheckName, + Text: unusedDisableText(`promql/series(notfound{job="foo"})`), + Details: checks.SeriesCheckUnusedDisableComment, + Severity: checks.Warning, + }, } }, mocks: []*prometheusMock{ @@ -3423,6 +3472,64 @@ func TestSeriesCheck(t *testing.T) { }, }, }, + { + description: "disable comment for prometheus server", + content: ` +# pint disable promql/series(prom) +- record: my_series + expr: vector(1) +`, + checker: newSeriesCheck, + prometheus: newSimpleProm, + problems: noProblems, + }, + { + description: "disable comment for prometheus server", + content: ` +# pint disable promql/series(+mytag) +- record: my_series + expr: vector(1) +`, + checker: newSeriesCheck, + prometheus: newSimpleProm, + problems: noProblems, + }, + { + description: "disable comment for a valid series", + content: ` +# pint disable promql/series(foo) +- record: my_series + expr: count(foo) +`, + checker: newSeriesCheck, + prometheus: newSimpleProm, + problems: noProblems, + }, + { + description: "disable comment for a mismatched series", + content: ` +# pint disable promql/series(bar) +# pint disable promql/series(foo) +- record: my_series + expr: count(bar) +`, + checker: newSeriesCheck, + prometheus: newSimpleProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 5, + Last: 5, + }, + Reporter: checks.SeriesCheckName, + Text: unusedDisableText("promql/series(foo)"), + Details: checks.SeriesCheckUnusedDisableComment, + Severity: checks.Warning, + }, + } + }, + }, } runTests(t, testCases) }