Fus-digital-cuckoo (#93)

* feat: add new validation expressionDoesNotUseClassicHistogramBucketOperations

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

* fix: release notes in CI

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

* fix: release notes CI

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

---------

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

.github/workflows/release.yaml

@@ -3,7 +3,7 @@ name: goreleaser
 on:
   push:
     tags:
-      - '*'
+      - "*"
 
 permissions:
   contents: write
@@ -20,17 +20,19 @@ jobs:
         uses: actions/setup-go@v5
         with:
           go-version: "1.23"
-
       - name: Login to Docker Hub
         uses: docker/login-action@v3
         with:
           username: ${{ secrets.DOCKERHUB_USERNAME }}
           password: ${{ secrets.DOCKERHUB_TOKEN }}
+
+      - name: ReleaseNotes
+        run: make release_notes.md
       - name: Run GoReleaser
         uses: goreleaser/goreleaser-action@v6
         with:
           distribution: goreleaser
           version: latest
-          args: release --clean
+          args: release --clean --release-notes release_notes.md
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

.github/workflows/test-release.yaml

@@ -20,7 +20,9 @@ jobs:
         with:
           go-version: "1.23"
 
+      - name: ReleaseNotes
+        run: make release_notes.md
       - name: Test
         uses: goreleaser/goreleaser-action@v6
         with:
-          args: release --snapshot --clean
+          args: release --snapshot --clean --release-notes release_notes.md

.gitignore

@@ -10,4 +10,5 @@ vendor/
 *_cache.json
 promruval
 dist/
-tmp
+release_notes.md
+

.goreleaser.yml

@@ -19,6 +19,9 @@ builds:
       - goos: darwin
         goarch: "386"
 
+release:
+  draft: true
+
 source:
   enabled: true
 

CHANGELOG.md

@@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+- Added: New validation `expressionDoesNotUseClassicHistogramBucketOperations` to avoid queries fragile because of the classic histogram bucket operations.
+  See the docs for more info [expressionDoesNotUseClassicHistogramBucketOperations](docs/validations.md#expressiondoesnotuseclassichistogrambucketoperations)
 
 ## [3.4.0] - 2024-10-30
 - Fixed: :warning: Ignore white spaces around rule names in the `disabled_validation_rules` annotation CSV format (Thanks @jmichalek13 !)

Makefile

@@ -1,25 +1,14 @@
 #!/usr/bin/make -f
 SRC_DIR	:= $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))
-TMP_DIR ?= $(SRC_DIR)/tmp
-TMP_BIN_DIR ?= $(TMP_DIR)/bin
 CACHE_FILE := .promruval_cache.json
 
 PROMRUVAL_BIN := ./promruval
+RELEASE_NOTES := release_notes.md
 
 all: clean deps lint build test e2e-test test-release
 
-$(TMP_DIR):
-	mkdir -p $(TMP_DIR)
-
-$(TMP_BIN_DIR):
-	mkdir -p $(TMP_BIN_DIR)
-
-RELEASE_NOTES ?= $(TMP_DIR)/release_notes
-$(RELEASE_NOTES): $(TMP_DIR)
-	@echo "Generating release notes to $(RELEASE_NOTES) ..."
-	@csplit -q -n1 --suppress-matched -f $(TMP_DIR)/release-notes-part CHANGELOG.md '/## \[\s*v.*\]/' {1}
-	@mv $(TMP_DIR)/release-notes-part1 $(RELEASE_NOTES)
-	@rm $(TMP_DIR)/release-notes-part*
+$(RELEASE_NOTES):
+	@cat CHANGELOG.md | grep -E -A 1000 -m 1 "## \[[0-9]" | grep -E -B 1000 -m 2 "## \[[0-9]" | head -n-1 | tail -n+2 > $(RELEASE_NOTES)
 
 lint:
 	golangci-lint run
@@ -56,11 +45,11 @@ docker: build
 
 .PHONY: clean
 clean:
-	rm -rf dist $(TMP_DIR) $(PROMRUVAL_BIN) $(CACHE_FILE)
+	rm -rf dist $(RELEASE_NOTES) $(PROMRUVAL_BIN) $(CACHE_FILE)
 
 .PHONY: deps
 deps:
 	go mod tidy && go mod verify
 
-test-release:
-	goreleaser release --snapshot --clean
+test-release: release_notes.md
+	goreleaser release --snapshot --clean --release-notes release_notes.md

docs/validations.md

@@ -35,6 +35,7 @@ All the supported validations are listed here. The validations are grouped by th
       - [`expressionUsesUnderscoresInLargeNumbers`](#expressionusesunderscoresinlargenumbers)
       - [`expressionWithNoMetricName`](#expressionwithnometricname)
       - [`expressionIsWellFormatted`](#expressioniswellformatted)
+      - [`expressionDoesNotUseClassicHistogramBucketOperations`](#expressiondoesnotuseclassichistogrambucketoperations)
     - [PromQL expression validators (using live Prometheus instance)](#promql-expression-validators-using-live-prometheus-instance)
       - [`expressionCanBeEvaluated`](#expressioncanbeevaluated)
       - [`expressionUsesExistingLabels`](#expressionusesexistinglabels)
@@ -343,6 +344,14 @@ params:
   skipExpressionsWithComments: true # Optional, will skip the expressions with comments
 ```
 
+#### `expressionDoesNotUseClassicHistogramBucketOperations`
+
+Fails if the expression does any binary operation between bucket metrics of a classical histogram.
+
+> There are situations when the classic histogram is not atomic (for example remote write), this it may result in unexpected results.
+> This calculation is often used to calculate SLOs a a difference between the `+Inf` bucket and one of the buckets which is the SLO threshold.
+> To avoid this issue, it's recommended to calculate such differences before sending the data over the remote write for example.
+
 ### PromQL expression validators (using live Prometheus instance)
 
 All these validations require the ` prometheus` sectiong in the config to be set.

examples/human_readable.html

@@ -55,6 +55,7 @@ <h2><a href="#check-groups">check-groups</a></h2>
   <h2><a href="#check-formatting">check-formatting</a></h2>
 	  <ul>
 		<li>All rules expression is well formatted as would <code>promtool promql format</code> do or similar online tool such as https://o11y.tools/promqlparser/</li>
+		<li>All rules expression does not do any binary operations between histogram buckets, it can be dangerous because of inconsistency in the data if sent over remote write for example</li>
 	  </ul>
 
   <h2><a href="#check-recording-rules">check-recording-rules</a></h2>

examples/human_readable.md

@@ -40,6 +40,7 @@ Validation rules:
 
   check-formatting
     - All rules expression is well formatted as would `promtool promql format` do or similar online tool such as https://o11y.tools/promqlparser/
+    - All rules expression does not do any binary operations between histogram buckets, it can be dangerous because of inconsistency in the data if sent over remote write for example
 
   check-recording-rules
     - Recording rule Recorded metric name does not match regexp: ^foo_bar$

examples/validation.yaml

@@ -104,6 +104,7 @@ validationRules:
         params:
           showExpectedForm: true
           skipExpressionsWithComments: true
+      - type: expressionDoesNotUseClassicHistogramBucketOperations
 
   - name: check-recording-rules
     scope: Recording rule

pkg/validator/config.go

@@ -21,22 +21,23 @@ var registeredUniversalRuleValidators = map[string]validatorCreator{
 	"exclusiveLabels":      newExclusiveLabels,
 
 	// Expressions
-	"expressionIsValidPromQL":                        newExpressionIsValidPromQL,
-	"validFunctionsOnCounters":                       newValidFunctionsOnCounters,
-	"rateBeforeAggregation":                          newRateBeforeAggregation,
-	"expressionDoesNotUseLabels":                     newExpressionDoesNotUseLabels,
-	"expressionUsesOnlyAllowedLabelsForMetricRegexp": newExpressionUseOnlyWhitelistedLabelsForMetric,
-	"expressionDoesNotUseOlderDataThan":              newExpressionDoesNotUseOlderDataThan,
-	"expressionDoesNotUseRangeShorterThan":           newExpressionDoesNotUseRangeShorterThan,
-	"expressionDoesNotUseMetrics":                    newExpressionDoesNotUseMetrics,
-	"expressionDoesNotUseIrate":                      newExpressionDoesNotUseIrate,
-	"expressionCanBeEvaluated":                       newExpressionCanBeEvaluated,
-	"expressionUsesExistingLabels":                   newExpressionUsesExistingLabels,
-	"expressionSelectorsMatchesAnything":             newExpressionSelectorsMatchesAnything,
-	"expressionWithNoMetricName":                     newExpressionWithNoMetricName,
-	"expressionIsWellFormatted":                      newExpressionIsWellFormatted,
-	"expressionUsesUnderscoresInLargeNumbers":        newExpressionUsesUnderscoresInLargeNumbers,
-	"expressionDoesNotUseExperimentalFunctions":      newExpressionDoesNotUseExperimentalFunctions,
+	"expressionIsValidPromQL":                              newExpressionIsValidPromQL,
+	"validFunctionsOnCounters":                             newValidFunctionsOnCounters,
+	"rateBeforeAggregation":                                newRateBeforeAggregation,
+	"expressionDoesNotUseLabels":                           newExpressionDoesNotUseLabels,
+	"expressionUsesOnlyAllowedLabelsForMetricRegexp":       newExpressionUseOnlyWhitelistedLabelsForMetric,
+	"expressionDoesNotUseOlderDataThan":                    newExpressionDoesNotUseOlderDataThan,
+	"expressionDoesNotUseRangeShorterThan":                 newExpressionDoesNotUseRangeShorterThan,
+	"expressionDoesNotUseMetrics":                          newExpressionDoesNotUseMetrics,
+	"expressionDoesNotUseIrate":                            newExpressionDoesNotUseIrate,
+	"expressionCanBeEvaluated":                             newExpressionCanBeEvaluated,
+	"expressionUsesExistingLabels":                         newExpressionUsesExistingLabels,
+	"expressionSelectorsMatchesAnything":                   newExpressionSelectorsMatchesAnything,
+	"expressionWithNoMetricName":                           newExpressionWithNoMetricName,
+	"expressionIsWellFormatted":                            newExpressionIsWellFormatted,
+	"expressionUsesUnderscoresInLargeNumbers":              newExpressionUsesUnderscoresInLargeNumbers,
+	"expressionDoesNotUseExperimentalFunctions":            newExpressionDoesNotUseExperimentalFunctions,
+	"expressionDoesNotUseClassicHistogramBucketOperations": newExpressionDoesNotUseOperationsBetweenClassicHistogramBuckets,
 
 	// LogQL
 	"expressionIsValidLogQL":              newExpressionIsValidLogQL,

pkg/validator/promql_expression.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"slices"
 	"strings"
 	"time"
 
@@ -654,3 +655,55 @@ func (h expressionUsesUnderscoresInLargeNumbers) Validate(_ unmarshaler.RuleGrou
 	}
 	return []error{}
 }
+
+func newExpressionDoesNotUseOperationsBetweenClassicHistogramBuckets(paramsConfig yaml.Node) (Validator, error) {
+	params := struct{}{}
+	if err := paramsConfig.Decode(&params); err != nil {
+		return nil, err
+	}
+	return &expressionDoesNotUseOperationsBetweenClassicHistogramBuckets{}, nil
+}
+
+type expressionDoesNotUseOperationsBetweenClassicHistogramBuckets struct{}
+
+func (h expressionDoesNotUseOperationsBetweenClassicHistogramBuckets) String() string {
+	return "expression does not do any binary operations between histogram buckets, it can be dangerous because of inconsistency in the data if sent over remote write for example"
+}
+
+const (
+	histogramBucketSuffix    = "_bucket"
+	histogramBucketLabelName = "le"
+)
+
+func (h expressionDoesNotUseOperationsBetweenClassicHistogramBuckets) 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)}
+	}
+	var errs []error
+	parser.Inspect(promQl, func(n parser.Node, _ []parser.Node) error {
+		if op, ok := n.(*parser.BinaryExpr); ok {
+			lhsVectorSelectors, err := getExpressionVectorSelectors(op.LHS.String())
+			if err != nil {
+				return err
+			}
+			rhsVectorSelectors, err := getExpressionVectorSelectors(op.RHS.String())
+			if err != nil {
+				return err
+			}
+			// If there is more then one vector selector on either side of the binary operation, we can't be sure that it is a histogram bucket operation
+			if len(lhsVectorSelectors) == 1 && len(rhsVectorSelectors) == 1 {
+				lhs := lhsVectorSelectors[0]
+				rhs := rhsVectorSelectors[0]
+				lhsLabelsUsedInSelector, _ := labelsUsedInSelectorForMetric(lhs, nil)
+				rhsLabelsUsedInSelector, _ := labelsUsedInSelectorForMetric(rhs, nil)
+				// Only detect if the metric name is the same and both selectors use the le label in selector
+				if lhs.Name == rhs.Name && strings.HasSuffix(lhs.Name, histogramBucketSuffix) && slices.Contains(lhsLabelsUsedInSelector, histogramBucketLabelName) && slices.Contains(rhsLabelsUsedInSelector, "le") {
+					errs = append(errs, fmt.Errorf("expression does binary operation between histogram buckets `%s` and `%s`", lhs.Name, rhs.Name))
+				}
+			}
+		}
+		return nil
+	})
+	return errs
+}

pkg/validator/promql_expression_helpers.go

@@ -29,7 +29,7 @@ func allowedLabelsMap(l []string) map[string]struct{} {
 // Returns true in case metric name selector matches given regexp and a list used labels, false and empty list otherwise.
 func labelsUsedInSelectorForMetric(selector *parser.VectorSelector, metricRegexp *regexp.Regexp) (usedLabels []string, metricUsed bool) {
 	for _, m := range selector.LabelMatchers {
-		if m.Name == metricNameLabel && m.Type == labels.MatchEqual && metricRegexp.MatchString(m.Value) {
+		if metricRegexp != nil && m.Name == metricNameLabel && m.Type == labels.MatchEqual && metricRegexp.MatchString(m.Value) {
 			metricUsed = true
 		}
 		usedLabels = append(usedLabels, m.Name)

pkg/validator/validator_test.go

@@ -323,6 +323,11 @@ var testCases = []struct {
 	{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},
+
+	// expressionDoesNotUseOperationsBetweenClassicHistogramBuckets
+	{name: "expressionDoesNotUseOperationsBetweenClassicHistogramBuckets_valid", validator: expressionDoesNotUseOperationsBetweenClassicHistogramBuckets{}, rule: rulefmt.Rule{Expr: `foo_bucket{le="+Inf"} - bar_bucket{le="1"}`}, expectedErrors: 0},
+	{name: "expressionDoesNotUseOperationsBetweenClassicHistogramBuckets_invalid", validator: expressionDoesNotUseOperationsBetweenClassicHistogramBuckets{}, rule: rulefmt.Rule{Expr: `request_duration_seconds_bucket{le="+Inf"} - ignoring(le) request_duration_seconds_bucket{le="1"}`}, expectedErrors: 1},
+	{name: "expressionDoesNotUseOperationsBetweenClassicHistogramBuckets_complicated_valid", validator: expressionDoesNotUseOperationsBetweenClassicHistogramBuckets{}, rule: rulefmt.Rule{Expr: `(request_duration_seconds_bucket{app="foo", le="+Inf"} * up{app="foo"}) - ignoring(le) request_duration_seconds_bucket{le="1"}`}, expectedErrors: 0},
 }
 
 func Test(t *testing.T) {