From 1c1b174a8e98f5a97ba782b50f4eb9a09d3f9a8e Mon Sep 17 00:00:00 2001 From: Filip Petkovski Date: Wed, 6 Apr 2022 06:05:11 +0200 Subject: [PATCH] Add a --lint flag to the promtool check rules and check config commands (#10435) * Add a --lint flag to the promtool check rules and check config commands Checking rules with promtool emits warnings in the case of duplicate rules. These warnings do not result in a non-zero exit code and are difficult to spot in CI environments. Additionally, checking for duplicates is closer to a lint check rather than a syntax check. This commit adds a --lint flag to commands which include checking rules. The flag can be used to enable or disable certain linting options and cause the execution to return a non-zero exit code in case those options are not met. Signed-off-by: fpetkovski * Exit with status 3 on lint error Signed-off-by: fpetkovski --- cmd/promtool/main.go | 90 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 15 deletions(-) diff --git a/cmd/promtool/main.go b/cmd/promtool/main.go index dbdc825fe..bf85786ae 100644 --- a/cmd/promtool/main.go +++ b/cmd/promtool/main.go @@ -66,8 +66,14 @@ const ( failureExitCode = 1 // Exit code 3 is used for "one or more lint issues detected". lintErrExitCode = 3 + + lintOptionAll = "all" + lintOptionDuplicateRules = "duplicate-rules" + lintOptionNone = "none" ) +var lintOptions = []string{lintOptionAll, lintOptionDuplicateRules, lintOptionNone} + func main() { app := kingpin.New(filepath.Base(os.Args[0]), "Tooling for the Prometheus monitoring system.").UsageWriter(os.Stdout) app.Version(version.Print("promtool")) @@ -86,6 +92,10 @@ func main() { "The config files to check.", ).Required().ExistingFiles() checkConfigSyntaxOnly := checkConfigCmd.Flag("syntax-only", "Only check the config file syntax, ignoring file and content validation referenced in the config").Bool() + checkConfigLint := checkConfigCmd.Flag( + "lint", + "Linting checks to apply to the rules specified in the config. Available options are: "+strings.Join(lintOptions, ", ")+". Use --lint=none to disable linting", + ).Default(lintOptionDuplicateRules).String() checkWebConfigCmd := checkCmd.Command("web-config", "Check if the web config files are valid or not.") webConfigFiles := checkWebConfigCmd.Arg( @@ -98,6 +108,10 @@ func main() { "rule-files", "The rule files to check.", ).Required().ExistingFiles() + checkRulesLint := checkRulesCmd.Flag( + "lint", + "Linting checks to apply. Available options are: "+strings.Join(lintOptions, ", ")+". Use --lint=none to disable linting", + ).Default(lintOptionDuplicateRules).String() checkMetricsCmd := checkCmd.Command("metrics", checkMetricsUsage) checkMetricsExtended := checkCmd.Flag("extended", "Print extended information related to the cardinality of the metrics.").Bool() @@ -222,13 +236,13 @@ func main() { os.Exit(CheckSD(*sdConfigFile, *sdJobName, *sdTimeout)) case checkConfigCmd.FullCommand(): - os.Exit(CheckConfig(*agentMode, *checkConfigSyntaxOnly, *configFiles...)) + os.Exit(CheckConfig(*agentMode, *checkConfigSyntaxOnly, newLintConfig(*checkConfigLint), *configFiles...)) case checkWebConfigCmd.FullCommand(): os.Exit(CheckWebConfig(*webConfigFiles...)) case checkRulesCmd.FullCommand(): - os.Exit(CheckRules(*ruleFiles...)) + os.Exit(CheckRules(newLintConfig(*checkRulesLint), *ruleFiles...)) case checkMetricsCmd.FullCommand(): os.Exit(CheckMetrics(*checkMetricsExtended)) @@ -283,9 +297,39 @@ func main() { } } +// nolint:revive +var lintError = fmt.Errorf("lint error") + +type lintConfig struct { + all bool + duplicateRules bool +} + +func newLintConfig(stringVal string) lintConfig { + items := strings.Split(stringVal, ",") + ls := lintConfig{} + for _, setting := range items { + switch setting { + case lintOptionAll: + ls.all = true + case lintOptionDuplicateRules: + ls.duplicateRules = true + case lintOptionNone: + default: + fmt.Printf("WARNING: unknown lint option %s\n", setting) + } + } + return ls +} + +func (ls lintConfig) lintDuplicateRules() bool { + return ls.all || ls.duplicateRules +} + // CheckConfig validates configuration files. -func CheckConfig(agentMode, checkSyntaxOnly bool, files ...string) int { +func CheckConfig(agentMode, checkSyntaxOnly bool, lintSettings lintConfig, files ...string) int { failed := false + hasLintErrors := false for _, f := range files { ruleFiles, err := checkConfig(agentMode, f, checkSyntaxOnly) @@ -301,18 +345,24 @@ func CheckConfig(agentMode, checkSyntaxOnly bool, files ...string) int { fmt.Println() for _, rf := range ruleFiles { - if n, errs := checkRules(rf); len(errs) > 0 { + if n, errs := checkRules(rf, lintSettings); len(errs) > 0 { fmt.Fprintln(os.Stderr, " FAILED:") for _, err := range errs { fmt.Fprintln(os.Stderr, " ", err) } failed = true + for _, err := range errs { + hasLintErrors = hasLintErrors || errors.Is(err, lintError) + } } else { fmt.Printf(" SUCCESS: %d rules found\n", n) } fmt.Println() } } + if failed && hasLintErrors { + return lintErrExitCode + } if failed { return failureExitCode } @@ -521,28 +571,35 @@ func checkSDFile(filename string) ([]*targetgroup.Group, error) { } // CheckRules validates rule files. -func CheckRules(files ...string) int { +func CheckRules(ls lintConfig, files ...string) int { failed := false + hasLintErrors := false for _, f := range files { - if n, errs := checkRules(f); errs != nil { + if n, errs := checkRules(f, ls); errs != nil { fmt.Fprintln(os.Stderr, " FAILED:") for _, e := range errs { fmt.Fprintln(os.Stderr, e.Error()) } failed = true + for _, err := range errs { + hasLintErrors = hasLintErrors || errors.Is(err, lintError) + } } else { fmt.Printf(" SUCCESS: %d rules found\n", n) } fmt.Println() } + if failed && hasLintErrors { + return lintErrExitCode + } if failed { return failureExitCode } return successExitCode } -func checkRules(filename string) (int, []error) { +func checkRules(filename string, lintSettings lintConfig) (int, []error) { fmt.Println("Checking", filename) rgs, errs := rulefmt.ParseFile(filename) @@ -555,16 +612,19 @@ func checkRules(filename string) (int, []error) { numRules += len(rg.Rules) } - dRules := checkDuplicates(rgs.Groups) - if len(dRules) != 0 { - fmt.Printf("%d duplicate rule(s) found.\n", len(dRules)) - for _, n := range dRules { - fmt.Printf("Metric: %s\nLabel(s):\n", n.metric) - for _, l := range n.label { - fmt.Printf("\t%s: %s\n", l.Name, l.Value) + if lintSettings.lintDuplicateRules() { + dRules := checkDuplicates(rgs.Groups) + if len(dRules) != 0 { + errMessage := fmt.Sprintf("%d duplicate rule(s) found.\n", len(dRules)) + for _, n := range dRules { + errMessage += fmt.Sprintf("Metric: %s\nLabel(s):\n", n.metric) + for _, l := range n.label { + errMessage += fmt.Sprintf("\t%s: %s\n", l.Name, l.Value) + } } + errMessage += "Might cause inconsistency while recording expressions" + return 0, []error{fmt.Errorf("%w %s", lintError, errMessage)} } - fmt.Println("Might cause inconsistency while recording expressions.") } return numRules, nil