From 606441b1d859448ffd74b4a6f282da6ac2131681 Mon Sep 17 00:00:00 2001 From: Martin Chodur Date: Thu, 31 Oct 2024 00:10:48 +0100 Subject: [PATCH] Fus-digital-cuckoo (#93) * feat: add new validation expressionDoesNotUseClassicHistogramBucketOperations Signed-off-by: Martin Chodur * fix: release notes in CI Signed-off-by: Martin Chodur * fix: release notes CI Signed-off-by: Martin Chodur --------- Signed-off-by: Martin Chodur --- .github/workflows/release.yaml | 8 ++-- .github/workflows/test-release.yaml | 4 +- .gitignore | 3 +- .goreleaser.yml | 3 ++ CHANGELOG.md | 2 + Makefile | 23 +++------- docs/validations.md | 9 ++++ examples/human_readable.html | 1 + examples/human_readable.md | 1 + examples/validation.yaml | 1 + pkg/validator/config.go | 33 +++++++------- pkg/validator/promql_expression.go | 53 ++++++++++++++++++++++ pkg/validator/promql_expression_helpers.go | 2 +- pkg/validator/validator_test.go | 5 ++ 14 files changed, 109 insertions(+), 39 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index ee8f9ec..765c3ce 100644 --- a/.github/workflows/release.yaml +++ b/.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 }} diff --git a/.github/workflows/test-release.yaml b/.github/workflows/test-release.yaml index 84c0544..efdcccf 100644 --- a/.github/workflows/test-release.yaml +++ b/.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 diff --git a/.gitignore b/.gitignore index 7ae80e1..c30becf 100644 --- a/.gitignore +++ b/.gitignore @@ -10,4 +10,5 @@ vendor/ *_cache.json promruval dist/ -tmp +release_notes.md + diff --git a/.goreleaser.yml b/.goreleaser.yml index 843dab5..bb4b9c6 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -19,6 +19,9 @@ builds: - goos: darwin goarch: "386" +release: + draft: true + source: enabled: true diff --git a/CHANGELOG.md b/CHANGELOG.md index c060e15..af28dab 100644 --- a/CHANGELOG.md +++ b/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 !) diff --git a/Makefile b/Makefile index 54c6678..1bb31a1 100644 --- a/Makefile +++ b/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 diff --git a/docs/validations.md b/docs/validations.md index ad4f621..1f44395 100644 --- a/docs/validations.md +++ b/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. diff --git a/examples/human_readable.html b/examples/human_readable.html index 20cad90..fd200b5 100644 --- a/examples/human_readable.html +++ b/examples/human_readable.html @@ -55,6 +55,7 @@

check-groups

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

diff --git a/examples/human_readable.md b/examples/human_readable.md index ef0bf0d..31a221a 100644 --- a/examples/human_readable.md +++ b/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$ diff --git a/examples/validation.yaml b/examples/validation.yaml index f31348c..7216b54 100644 --- a/examples/validation.yaml +++ b/examples/validation.yaml @@ -104,6 +104,7 @@ validationRules: params: showExpectedForm: true skipExpressionsWithComments: true + - type: expressionDoesNotUseClassicHistogramBucketOperations - name: check-recording-rules scope: Recording rule diff --git a/pkg/validator/config.go b/pkg/validator/config.go index c6cbf25..76b472f 100644 --- a/pkg/validator/config.go +++ b/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, diff --git a/pkg/validator/promql_expression.go b/pkg/validator/promql_expression.go index d60e2d8..c7caff8 100644 --- a/pkg/validator/promql_expression.go +++ b/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(¶ms); 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 +} diff --git a/pkg/validator/promql_expression_helpers.go b/pkg/validator/promql_expression_helpers.go index 7b6f994..ced96ac 100644 --- a/pkg/validator/promql_expression_helpers.go +++ b/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) diff --git a/pkg/validator/validator_test.go b/pkg/validator/validator_test.go index 6ba3fb1..2b40d1d 100644 --- a/pkg/validator/validator_test.go +++ b/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) {