feat: add new validator expressionUsesUnderscoresInLargeNumbers

Signed-off-by: Martin Chodur <[email protected]>

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Changed: Upgrade to go 1.23
 - Changed: Upgrade prometheus dependencies
 - Changed: Enable experimental functions in PromQL parser by default, to forbid them use a new validator `expressionDoesNotUseExperimentalFunctions`
+- Added: New validator `expressionDoesNotUseExperimentalFunctions` to check if the expression does not use experimental functions
+- Added: New validator `expressionUsesUnderscoresInLargeNumbers` that requires using underscores as separators in large numbers in expressions
 
 ## [3.2.0]
 - Added: Build of windows and darwin and support for arm architectures in CI

docs/default_validation.yaml

@@ -30,6 +30,7 @@ validationRules:
       - type: rateBeforeAggregation
       - type: expressionWithNoMetricName
       - type: expressionDoesNotUseExperimentalFunctions
+      - type: expressionUsesUnderscoresInLargeNumbers
       - type: expressionIsWellFormatted
         params:
           showExpectedForm: true

docs/validations.md

@@ -32,6 +32,7 @@ All the supported validations are listed here. The validations are grouped by th
       - [`expressionDoesNotUseIrate`](#expressiondoesnotuseirate)
       - [`validFunctionsOnCounters`](#validfunctionsoncounters)
       - [`rateBeforeAggregation`](#ratebeforeaggregation)
+      - [`expressionUsesUnderscoresInLargeNumbers`](#expressionusesunderscoresinlargenumbers)
       - [`expressionWithNoMetricName`](#expressionwithnometricname)
       - [`expressionIsWellFormatted`](#expressioniswellformatted)
     - [PromQL expression validators (using live Prometheus instance)](#promql-expression-validators-using-live-prometheus-instance)
@@ -318,6 +319,11 @@ Fails if the expression uses a `rate` or `increase` function on a metric that do
 Fails if aggregation function is used before the `rate` or `increase` functions.
 > Avoid common mistake of using aggregation function before the `rate` or `increase` function.
 
+#### `expressionUsesUnderscoresInLargeNumbers`
+
+Fails if the query containes numbers higher than 1000 without using underscores as separators for better readability.
+Ignores numbers in the `10e2` and duration format.
+
 #### `expressionWithNoMetricName`
 
 Fails if an expression doesn't use an explicit metric name (also if used as `__name__` label) in all its selectors(eg `up{foo="bar"}`).

examples/human_readable.html

@@ -33,6 +33,7 @@ <h2><a href="#check-alert-title">check-alert-title</a></h2>
   <h2><a href="#check-prometheus-limitations">check-prometheus-limitations</a></h2>
 	  <ul>
 		<li>All rules expression does not use any experimental PromQL functions</li>
+		<li>All rules expression uses underscores as separators in large numbers in PromQL expression. Example: 1_000_000</li>
 		<li>All rules expression does not use data older than <code>6h0m0s</code></li>
 		<li>All rules does not use any of the <code>cluster</code>,<code>locality</code>,<code>prometheus-type</code>,<code>replica</code> labels is in its expression</li>
 	  </ul>

examples/human_readable.md

@@ -24,6 +24,7 @@ Validation rules:
 
   check-prometheus-limitations
     - All rules expression does not use any experimental PromQL functions
+    - All rules expression uses underscores as separators in large numbers in PromQL expression. Example: 1_000_000
     - All rules expression does not use data older than `6h0m0s`
     - All rules does not use any of the `cluster`,`locality`,`prometheus-type`,`replica` labels is in its expression
 

examples/rules/rules.yaml

@@ -10,6 +10,14 @@ groups:
         labels:
           foo: bar
 
+      - alert: testExperimentalPromQL
+        expr: |
+          # ignore_validations: expressionCanBeEvaluated, expressionDoesNotUseExperimentalFunctions, hasLabels, hasAnyOfAnnotations, hasAnnotations
+          sort_by_label(
+            up,
+            "instance"
+          )
+
   # ignore_validations: labelHasAllowedValue
   - name: testGroup
     limit: 1000
@@ -18,11 +26,7 @@ groups:
       # Comment on the same line. ignore_validations: expressionSelectorsMatchesAnything, expressionDoesNotUseOlderDataThan
       # Comment after.
       - alert: test
-        expr: |
-          sort_by_label(
-            avg_over_time(max_over_time(up{job="prometheus"}[10h] offset 10d)[10m:10m]),
-            "instance"
-          )
+        expr: avg_over_time(max_over_time(up{job="prometheus"}[10h] offset 10d)[10m:10m])
         for: 4w
         keep_firing_for: 5m
         labels:

examples/validation.yaml

@@ -2,7 +2,7 @@
 # customDisableComment: my_disable_comment
 
 prometheus:
-  url: http://demo.robustperception.io:9090
+  url: https://prometheus.demo.do.prometheus.io
   bearerTokenFile: ./bearer.token
 
 validationRules:
@@ -66,6 +66,7 @@ validationRules:
     scope: All rules
     validations:
       - type: expressionDoesNotUseExperimentalFunctions
+      - type: expressionUsesUnderscoresInLargeNumbers
       - type: expressionDoesNotUseOlderDataThan
         params:
           limit: "6h"

pkg/validator/config.go

@@ -11,6 +11,7 @@ import (
 type validatorCreator func(params yaml.Node) (Validator, error)
 
 var registeredUniversalRuleValidators = map[string]validatorCreator{
+	// Labels
 	"hasLabels":            newHasLabels,
 	"doesNotHaveLabels":    newDoesNotHaveLabels,
 	"hasAnyOfLabels":       newHasAnyOfLabels,
@@ -19,6 +20,7 @@ var registeredUniversalRuleValidators = map[string]validatorCreator{
 	"nonEmptyLabels":       newNonEmptyLabels,
 	"exclusiveLabels":      newExclusiveLabels,
 
+	// Expressions
 	"expressionIsValidPromQL":                        newExpressionIsValidPromQL,
 	"validFunctionsOnCounters":                       newValidFunctionsOnCounters,
 	"rateBeforeAggregation":                          newRateBeforeAggregation,
@@ -33,11 +35,15 @@ var registeredUniversalRuleValidators = map[string]validatorCreator{
 	"expressionSelectorsMatchesAnything":             newExpressionSelectorsMatchesAnything,
 	"expressionWithNoMetricName":                     newExpressionWithNoMetricName,
 	"expressionIsWellFormatted":                      newExpressionIsWellFormatted,
-	"expressionIsValidLogQL":                         newExpressionIsValidLogQL,
+	"expressionUsesUnderscoresInLargeNumbers":        newExpressionUsesUnderscoresInLargeNumbers,
 	"expressionDoesNotUseExperimentalFunctions":      newExpressionDoesNotUseExperimentalFunctions,
-	"logQlExpressionUsesRangeAggregation":            newLogQLExpressionUsesRangeAggregation,
-	"logQlExpressionUsesFiltersFirst":                newlogQlExpressionUsesFiltersFirst,
 
+	// LogQL
+	"expressionIsValidLogQL":              newExpressionIsValidLogQL,
+	"logQlExpressionUsesRangeAggregation": newLogQLExpressionUsesRangeAggregation,
+	"logQlExpressionUsesFiltersFirst":     newlogQlExpressionUsesFiltersFirst,
+
+	// Other
 	"hasSourceTenantsForMetrics": newHasSourceTenantsForMetrics,
 }
 

pkg/validator/promql_expression.go

@@ -616,3 +616,41 @@ func (h expressionDoesNotUseExperimentalFunctions) Validate(_ unmarshaler.RuleGr
 	}
 	return []error{}
 }
+
+func newExpressionUsesUnderscoresInLargeNumbers(paramsConfig yaml.Node) (Validator, error) {
+	params := struct{}{}
+	if err := paramsConfig.Decode(&params); err != nil {
+		return nil, err
+	}
+	return &expressionUsesUnderscoresInLargeNumbers{}, nil
+}
+
+type expressionUsesUnderscoresInLargeNumbers struct{}
+
+func (h expressionUsesUnderscoresInLargeNumbers) String() string {
+	return "expression uses underscores as separators in large numbers in PromQL expression. Example: 1_000_000"
+}
+
+var numberRegexp = regexp.MustCompile(`^\d+$`)
+
+func (h expressionUsesUnderscoresInLargeNumbers) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {
+	promQl, err := parser.ParseExpr(rule.Expr)
+	if err != nil {
+		return []error{fmt.Errorf("failed to parse expression `%s`: %w", rule.Expr, err)}
+	}
+	invalidNumbers := []string{}
+	parser.Inspect(promQl, func(n parser.Node, _ []parser.Node) error {
+		if number, ok := n.(*parser.NumberLiteral); ok {
+			numberStr := rule.Expr[number.PosRange.Start:number.PosRange.End]
+			// Ignore numbers that use 10e2 notation and duration notation (1m, 1h, etc.) and numbers that are less than 1000 where the underscore is not needed
+			if numberRegexp.MatchString(numberStr) && number.Val >= 1000 && !strings.Contains(numberStr, "_") {
+				invalidNumbers = append(invalidNumbers, number.String())
+			}
+		}
+		return nil
+	})
+	if len(invalidNumbers) > 0 {
+		return []error{fmt.Errorf("expression should use _ in large numbers (example: 1_000_0000) for better readability in these numbers: %s", strings.Join(invalidNumbers, ", "))}
+	}
+	return []error{}
+}

pkg/validator/validator_test.go

@@ -311,6 +311,12 @@ var testCases = []struct {
 	// expressionDoesNotUseExperimentalFunctions
 	{name: "expressionDoesNotUseExperimentalFunctions_valid", validator: expressionDoesNotUseExperimentalFunctions{}, rule: rulefmt.Rule{Expr: `sort_desc(up)`}, expectedErrors: 0},
 	{name: "expressionDoesNotUseExperimentalFunctions_invalid", validator: expressionDoesNotUseExperimentalFunctions{}, rule: rulefmt.Rule{Expr: `sort_by_label(up, "instance")`}, expectedErrors: 1},
+
+	// expressionUsesUnderscoresInLargeNumbers
+	{name: "expressionUsesUnderscoresInLargeNumbers_valid", validator: expressionUsesUnderscoresInLargeNumbers{}, rule: rulefmt.Rule{Expr: `vector(time())  > 100`}, expectedErrors: 0},
+	{name: "expressionUsesUnderscoresInLargeNumbers_valid_duration", validator: expressionUsesUnderscoresInLargeNumbers{}, rule: rulefmt.Rule{Expr: `vector(time())  > 100h`}, expectedErrors: 0},
+	{name: "expressionUsesUnderscoresInLargeNumbers_valid_exp", validator: expressionUsesUnderscoresInLargeNumbers{}, rule: rulefmt.Rule{Expr: `vector(time())  > 10e12`}, expectedErrors: 0},
+	{name: "expressionUsesUnderscoresInLargeNumbers_invalid", validator: expressionUsesUnderscoresInLargeNumbers{}, rule: rulefmt.Rule{Expr: `time() > 1000`}, expectedErrors: 1},
 }
 
 func Test(t *testing.T) {