diff --git a/internal/checks/alerts_annotation_test.go b/internal/checks/alerts_annotation_test.go index cca9a6ef..b5caffca 100644 --- a/internal/checks/alerts_annotation_test.go +++ b/internal/checks/alerts_annotation_test.go @@ -11,115 +11,157 @@ func TestAnnotationCheck(t *testing.T) { { description: "ignores recording rules", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: noProblems, }, { description: "doesn't ignore rules with syntax errors", content: "- alert: foo\n expr: sum(foo) without(\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "alert: foo", - Lines: []int{1, 2}, - Reporter: "alerts/annotation", - Text: "severity annotation is required", - Severity: checks.Warning, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "alert: foo", + Lines: []int{1, 2}, + Reporter: checks.AnnotationCheckName, + Text: "severity annotation is required", + Severity: checks.Warning, + }, + } }, }, { description: "no annotations / required", content: "- alert: foo\n expr: sum(foo)\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "alert: foo", - Lines: []int{1, 2}, - Reporter: "alerts/annotation", - Text: "severity annotation is required", - Severity: checks.Warning, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "alert: foo", + Lines: []int{1, 2}, + Reporter: checks.AnnotationCheckName, + Text: "severity annotation is required", + Severity: checks.Warning, + }, + } }, }, { description: "no annotations / not required", content: "- alert: foo\n expr: sum(foo)\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: noProblems, }, { description: "missing annotation / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: bar\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "annotations:", - Lines: []int{3, 4}, - Reporter: "alerts/annotation", - Text: "severity annotation is required", - Severity: checks.Warning, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "annotations:", + Lines: []int{3, 4}, + Reporter: checks.AnnotationCheckName, + Text: "severity annotation is required", + Severity: checks.Warning, + }, + } }, }, { description: "missing annotation / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: bar\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: noProblems, }, { description: "wrong annotation value / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: bar\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "severity: bar", - Lines: []int{4}, - Reporter: "alerts/annotation", - Text: `severity annotation value must match "^critical$"`, - Severity: checks.Warning, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "severity: bar", + Lines: []int{4}, + Reporter: checks.AnnotationCheckName, + Text: `severity annotation value must match "^critical$"`, + Severity: checks.Warning, + }, + } }, }, { description: "wrong annotation value / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: bar\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "severity: bar", - Lines: []int{4}, - Reporter: "alerts/annotation", - Text: `severity annotation value must match "^critical$"`, - Severity: checks.Warning, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "severity: bar", + Lines: []int{4}, + Reporter: checks.AnnotationCheckName, + Text: `severity annotation value must match "^critical$"`, + Severity: checks.Warning, + }, + } }, }, { description: "valid annotation / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: info\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical|info|debug"), true, checks.Warning), + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical|info|debug"), true, checks.Warning) + }, + problems: noProblems, }, { description: "valid annotation / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: info\n", - checker: checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical|info|debug"), false, checks.Warning), + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("severity", checks.MustTemplatedRegexp("critical|info|debug"), false, checks.Warning) + }, + problems: noProblems, }, { description: "templated annotation value / passing", content: "- alert: foo\n expr: sum(foo)\n for: 5m\n annotations:\n for: 5m\n", - checker: checks.NewAnnotationCheck("for", checks.MustTemplatedRegexp("{{ $for }}"), true, checks.Bug), + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("for", checks.MustTemplatedRegexp("{{ $for }}"), true, checks.Bug) + }, + problems: noProblems, }, { description: "templated annotation value / passing", content: "- alert: foo\n expr: sum(foo)\n for: 5m\n annotations:\n for: 4m\n", - checker: checks.NewAnnotationCheck("for", checks.MustTemplatedRegexp("{{ $for }}"), true, checks.Bug), - problems: []checks.Problem{ - { - Fragment: "for: 4m", - Lines: []int{5}, - Reporter: "alerts/annotation", - Text: `for annotation value must match "^{{ $for }}$"`, - Severity: checks.Bug, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewAnnotationCheck("for", checks.MustTemplatedRegexp("{{ $for }}"), true, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "for: 4m", + Lines: []int{5}, + Reporter: checks.AnnotationCheckName, + Text: `for annotation value must match "^{{ $for }}$"`, + Severity: checks.Bug, + }, + } }, }, } diff --git a/internal/checks/alerts_comparison_test.go b/internal/checks/alerts_comparison_test.go index 572a4d0e..5a3a2a2b 100644 --- a/internal/checks/alerts_comparison_test.go +++ b/internal/checks/alerts_comparison_test.go @@ -6,45 +6,56 @@ import ( "github.com/cloudflare/pint/internal/checks" ) +func newComparisonCheck(_ string) checks.RuleChecker { + return checks.NewComparisonCheck() +} + func TestComparisonCheck(t *testing.T) { testCases := []checkTest{ { description: "ignores recording rules", content: "- record: foo\n expr: up == 0\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "ignores rules with syntax errors", content: "- alert: Foo Is Down\n expr: sum(\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "alert expr with > condition", content: "- alert: Foo Is Down\n for: 10m\n expr: up{job=\"foo\"} > 0\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "alert expr with >= condition", content: "- alert: Foo Is Down\n for: 10m\n expr: up{job=\"foo\"} >= 1\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "alert expr with == condition", content: "- alert: Foo Is Down\n for: 10m\n expr: up{job=\"foo\"} == 1\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "alert expr without any condition", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"}\n", - checker: checks.NewComparisonCheck(), - problems: []checks.Problem{ - { - Fragment: `up{job="foo"}`, - Lines: []int{2}, - Reporter: "alerts/comparison", - Text: "alert query doesn't have any condition, it will always fire if the metric exists", - Severity: checks.Warning, - }, + checker: newComparisonCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `up{job="foo"}`, + Lines: []int{2}, + Reporter: checks.ComparisonCheckName, + Text: "alert query doesn't have any condition, it will always fire if the metric exists", + Severity: checks.Warning, + }, + } }, }, { @@ -55,7 +66,8 @@ func TestComparisonCheck(t *testing.T) { AND ON (instance) (rate(node_netstat_Udp_RcvbufErrors[5m])+rate(node_netstat_Udp6_RcvbufErrors[5m])) > 200 `, - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "deep level without comparison", @@ -65,45 +77,52 @@ func TestComparisonCheck(t *testing.T) { AND ON (instance) rate(node_netstat_Udp_RcvbufErrors[5m])+rate(node_netstat_Udp6_RcvbufErrors[5m]) `, - checker: checks.NewComparisonCheck(), - problems: []checks.Problem{ - { - Fragment: `quantile_over_time(0.7,(irate(udp_packets_drops[2m]))[10m:2m]) AND ON (instance) rate(node_netstat_Udp_RcvbufErrors[5m])+rate(node_netstat_Udp6_RcvbufErrors[5m])`, - Lines: []int{3}, - Reporter: "alerts/comparison", - Text: "alert query doesn't have any condition, it will always fire if the metric exists", - Severity: checks.Warning, - }, + checker: newComparisonCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `quantile_over_time(0.7,(irate(udp_packets_drops[2m]))[10m:2m]) AND ON (instance) rate(node_netstat_Udp_RcvbufErrors[5m])+rate(node_netstat_Udp6_RcvbufErrors[5m])`, + Lines: []int{3}, + Reporter: checks.ComparisonCheckName, + Text: "alert query doesn't have any condition, it will always fire if the metric exists", + Severity: checks.Warning, + }, + } }, }, { description: "alert unless condition", content: "- alert: Foo Is Down\n for: 10m\n expr: foo unless bar\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "alert expr with bool", content: "- alert: Error rate is high\n expr: rate(error_count[5m]) > bool 5\n", - checker: checks.NewComparisonCheck(), - problems: []checks.Problem{ - { - Fragment: "rate(error_count[5m]) > bool 5", - Lines: []int{2}, - Reporter: "alerts/comparison", - Text: "alert query uses bool modifier for comparison, this means it will always return a result and the alert will always fire", - Severity: checks.Bug, - }, + checker: newComparisonCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(error_count[5m]) > bool 5", + Lines: []int{2}, + Reporter: checks.ComparisonCheckName, + Text: "alert query uses bool modifier for comparison, this means it will always return a result and the alert will always fire", + Severity: checks.Bug, + }, + } }, }, { description: "alert expr with bool and condition", content: "- alert: Error rate is high\n expr: rate(error_count[5m]) > bool 5 == 1\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, { description: "alert on absent", content: "- alert: Foo Is Missing\n expr: absent(foo)\n", - checker: checks.NewComparisonCheck(), + checker: newComparisonCheck, + problems: noProblems, }, } diff --git a/internal/checks/alerts_count_test.go b/internal/checks/alerts_count_test.go index b7df5ad0..682109c0 100644 --- a/internal/checks/alerts_count_test.go +++ b/internal/checks/alerts_count_test.go @@ -2,159 +2,237 @@ package checks_test import ( "fmt" - "net/http" - "net/http/httptest" "testing" "time" + "github.com/prometheus/common/model" + "github.com/cloudflare/pint/internal/checks" ) -func TestAlertsCheck(t *testing.T) { - content := "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n" - - now := time.Now() - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - err := r.ParseForm() - if err != nil { - t.Fatal(err) - } - query := r.Form.Get("query") - if query != `up{job="foo"} == 0` && - query != `{__name__="up", job="foo"} == 0` && - query != `{__name__=~"(up|foo)", job="foo"} == 0` { - t.Fatalf("Prometheus got invalid query: %s", query) - } +func newAlertsCheck(uri string) checks.RuleChecker { + return checks.NewAlertsCheck(simpleProm("prom", uri, time.Second*5, true), time.Hour*24, time.Minute, time.Minute*5) +} - switch r.URL.Path { - case "/empty/api/v1/query_range": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"success","data":{"resultType":"matrix","result":[]}}`)) - case "/alerts/api/v1/query_range": - w.WriteHeader(200) - out := fmt.Sprintf(`{ - "status":"success", - "data":{ - "resultType":"matrix", - "result":[ - {"metric":{"instance":"1"},"values":[ - [%d,"0"], - [%d,"0"], - [%d,"0"], - [%d,"0"], - [%d,"0"] - ]}, - {"metric":{"instance":"2"},"values":[ - [%d,"0"], - [%d,"0"], - [%d,"0"], - [%d,"0"], - [%d,"0"] - ]} - ] - } - }`, - now.AddDate(0, 0, -1).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*2).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*60).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*61).Unix(), +func alertsText(name, uri string, count int, since string) string { + return fmt.Sprintf(`prometheus %q at %s would trigger %d alert(s) in the last %s`, name, uri, count, since) +} - now.AddDate(0, 0, -1).Add(time.Minute*6).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*12).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*18).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*24).Unix(), - now.AddDate(0, 0, -1).Add(time.Minute*30).Unix(), - ) - _, _ = w.Write([]byte(out)) - default: - w.WriteHeader(400) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"error","errorType":"bad_data","error":"unhandled path"}`)) - } - })) - defer srv.Close() +func TestAlertsCheck(t *testing.T) { + content := "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n" testCases := []checkTest{ { description: "ignores recording rules", content: "- record: foo\n expr: up == 0\n", - checker: checks.NewAlertsCheck(simpleProm("prom", "http://localhost", time.Second*5, true), time.Hour*24, time.Minute, time.Minute*5), + checker: newAlertsCheck, + problems: noProblems, }, { description: "ignores rules with syntax errors", content: "- alert: Foo Is Down\n expr: sum(\n", - checker: checks.NewAlertsCheck(simpleProm("prom", "http://localhost", time.Second*5, true), time.Hour*24, time.Minute, time.Minute*5), + checker: newAlertsCheck, + problems: noProblems, }, { description: "bad request", content: content, - checker: checks.NewAlertsCheck(simpleProm("prom", srv.URL+"/400/", time.Second*5, true), time.Hour*24, time.Minute, time.Minute*5), - problems: []checks.Problem{ + checker: newAlertsCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `up{job="foo"} == 0`, + Lines: []int{2}, + Reporter: "alerts/count", + Text: checkErrorBadData("prom", uri, "bad_data: bad input data"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `up{job="foo"} == 0`, - Lines: []int{2}, - Reporter: "alerts/count", - Text: fmt.Sprintf(`prometheus "prom" at %s/400/ failed with: bad_data: unhandled path`, srv.URL), - Severity: checks.Bug, + conds: []requestCondition{ + requireRangeQueryPath, + formCond{key: "query", value: `up{job="foo"} == 0`}, + }, + resp: respondWithBadData(), }, }, }, { - description: "connection refused", + description: "connection refused / upstream not required / warning", content: content, - checker: checks.NewAlertsCheck(simpleProm("prom", "http://127.0.0.1", time.Second*5, false), time.Hour*24, time.Minute, time.Minute*5), - problems: []checks.Problem{ - { - Fragment: `up{job="foo"} == 0`, - Lines: []int{2}, - Reporter: "alerts/count", - Text: `cound't run "alerts/count" checks due to prometheus "prom" at http://127.0.0.1 connection error: Post "http://127.0.0.1/api/v1/query_range": dial tcp 127.0.0.1:80: connect: connection refused`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewAlertsCheck(simpleProm("prom", "http://127.0.0.1:1111", time.Second*5, false), time.Hour*24, time.Minute, time.Minute*5) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `up{job="foo"} == 0`, + Lines: []int{2}, + Reporter: "alerts/count", + Text: checkErrorUnableToRun(checks.AlertsCheckName, "prom", "http://127.0.0.1:1111", `Post "http://127.0.0.1:1111/api/v1/query_range": dial tcp 127.0.0.1:1111: connect: connection refused`), + Severity: checks.Warning, + }, + } }, }, { description: "empty response", content: content, - checker: checks.NewAlertsCheck(simpleProm("prom", srv.URL+"/empty/", time.Second*5, true), time.Hour*24, time.Minute, time.Minute*5), - problems: []checks.Problem{ + checker: newAlertsCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `up{job="foo"} == 0`, + Lines: []int{2}, + Reporter: "alerts/count", + Text: alertsText("prom", uri, 0, "1d"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `up{job="foo"} == 0`, - Lines: []int{2}, - Reporter: "alerts/count", - Text: fmt.Sprintf(`prometheus "prom" at %s/empty/ would trigger 0 alert(s) in the last 1d`, srv.URL), - Severity: checks.Information, + conds: []requestCondition{ + requireRangeQueryPath, + formCond{key: "query", value: `up{job="foo"} == 0`}, + }, + resp: respondWithEmptyMatrix(), }, }, }, { description: "multiple alerts", content: content, - checker: checks.NewAlertsCheck(simpleProm("prom", srv.URL+"/alerts/", time.Second*5, true), time.Hour*24, time.Minute, time.Minute*5), - problems: []checks.Problem{ + checker: newAlertsCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `up{job="foo"} == 0`, + Lines: []int{2}, + Reporter: "alerts/count", + Text: alertsText("prom", uri, 7, "1d"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `up{job="foo"} == 0`, - Lines: []int{2}, - Reporter: "alerts/count", - Text: fmt.Sprintf(`prometheus "prom" at %s/alerts/ would trigger 7 alert(s) in the last 1d`, srv.URL), - Severity: checks.Information, + conds: []requestCondition{ + requireRangeQueryPath, + formCond{key: "query", value: `up{job="foo"} == 0`}, + }, + resp: matrixResponse{ + samples: []*model.SampleStream{ + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*24), + time.Now().Add(time.Hour*24).Add(time.Minute*6), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*23), + time.Now().Add(time.Hour*23).Add(time.Minute*6), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*22), + time.Now().Add(time.Hour*22).Add(time.Minute), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*21), + time.Now().Add(time.Hour*21).Add(time.Minute*16), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*20), + time.Now().Add(time.Hour*20).Add(time.Minute*36), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*19), + time.Now().Add(time.Hour*19).Add(time.Minute*36), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*18), + time.Now().Add(time.Hour*18).Add(time.Hour*2), + time.Minute, + ), + }, + }, }, }, }, { description: "for: 10m", content: "- alert: Foo Is Down\n for: 10m\n expr: up{job=\"foo\"} == 0\n", - checker: checks.NewAlertsCheck(simpleProm("prom", srv.URL+"/alerts/", time.Second*5, true), time.Hour*24, time.Minute*6, time.Minute*10), - problems: []checks.Problem{ + checker: newAlertsCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `up{job="foo"} == 0`, + Lines: []int{2, 3}, + Reporter: "alerts/count", + Text: alertsText("prom", uri, 2, "1d"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `up{job="foo"} == 0`, - Lines: []int{2, 3}, - Reporter: "alerts/count", - Text: fmt.Sprintf(`prometheus "prom" at %s/alerts/ would trigger 1 alert(s) in the last 1d`, srv.URL), - Severity: checks.Information, + conds: []requestCondition{ + requireRangeQueryPath, + formCond{key: "query", value: `up{job="foo"} == 0`}, + }, + resp: matrixResponse{ + samples: []*model.SampleStream{ + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*24), + time.Now().Add(time.Hour*24).Add(time.Minute*6), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*23), + time.Now().Add(time.Hour*23).Add(time.Minute*6), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*22), + time.Now().Add(time.Hour*22).Add(time.Minute), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*21), + time.Now().Add(time.Hour*21).Add(time.Minute*16), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*20), + time.Now().Add(time.Hour*20).Add(time.Minute*9).Add(time.Second*59), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*18), + time.Now().Add(time.Hour*18).Add(time.Hour*2), + time.Minute, + ), + }, + }, }, }, }, @@ -164,14 +242,46 @@ func TestAlertsCheck(t *testing.T) { - alert: foo expr: '{__name__="up", job="foo"} == 0' `, - checker: checks.NewAlertsCheck(simpleProm("prom", srv.URL+"/alerts/", time.Second*5, true), time.Hour*24, time.Minute*6, time.Minute*10), - problems: []checks.Problem{ + checker: newAlertsCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `{__name__="up", job="foo"} == 0`, + Lines: []int{3}, + Reporter: "alerts/count", + Text: alertsText("prom", uri, 3, "1d"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `{__name__="up", job="foo"} == 0`, - Lines: []int{3}, - Reporter: "alerts/count", - Text: fmt.Sprintf(`prometheus "prom" at %s/alerts/ would trigger 3 alert(s) in the last 1d`, srv.URL), - Severity: checks.Information, + conds: []requestCondition{ + requireRangeQueryPath, + formCond{key: "query", value: `{__name__="up", job="foo"} == 0`}, + }, + resp: matrixResponse{ + samples: []*model.SampleStream{ + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*24), + time.Now().Add(time.Hour*24).Add(time.Minute*6), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*23), + time.Now().Add(time.Hour*23).Add(time.Minute*6), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*22), + time.Now().Add(time.Hour*22).Add(time.Minute), + time.Minute, + ), + }, + }, }, }, }, @@ -181,14 +291,46 @@ func TestAlertsCheck(t *testing.T) { - alert: foo expr: '{__name__=~"(up|foo)", job="foo"} == 0' `, - checker: checks.NewAlertsCheck(simpleProm("prom", srv.URL+"/alerts/", time.Second*5, true), time.Hour*24, time.Minute*6, time.Minute*10), - problems: []checks.Problem{ + checker: newAlertsCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `{__name__=~"(up|foo)", job="foo"} == 0`, + Lines: []int{3}, + Reporter: "alerts/count", + Text: alertsText("prom", uri, 3, "1d"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `{__name__=~"(up|foo)", job="foo"} == 0`, - Lines: []int{3}, - Reporter: "alerts/count", - Text: fmt.Sprintf(`prometheus "prom" at %s/alerts/ would trigger 3 alert(s) in the last 1d`, srv.URL), - Severity: checks.Information, + conds: []requestCondition{ + requireRangeQueryPath, + formCond{key: "query", value: `{__name__=~"(up|foo)", job="foo"} == 0`}, + }, + resp: matrixResponse{ + samples: []*model.SampleStream{ + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*21), + time.Now().Add(time.Hour*21).Add(time.Minute*16), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*20), + time.Now().Add(time.Hour*20).Add(time.Minute*9).Add(time.Second*59), + time.Minute, + ), + generateSampleStream( + map[string]string{"job": "foo"}, + time.Now().Add(time.Hour*18), + time.Now().Add(time.Hour*18).Add(time.Hour*2), + time.Minute, + ), + }, + }, }, }, }, diff --git a/internal/checks/alerts_for_test.go b/internal/checks/alerts_for_test.go index 09854556..6da5bc6b 100644 --- a/internal/checks/alerts_for_test.go +++ b/internal/checks/alerts_for_test.go @@ -6,58 +6,70 @@ import ( "github.com/cloudflare/pint/internal/checks" ) +func newAlertsForCheck(_ string) checks.RuleChecker { + return checks.NewAlertsForCheck() +} + func TestAlertsForCheck(t *testing.T) { testCases := []checkTest{ { description: "ignores rules with syntax errors", content: "- alert: foo\n expr: sum(foo) without(\n", - checker: checks.NewAlertsForCheck(), + checker: newAlertsForCheck, + problems: noProblems, }, { description: "ignores recording rules", content: "- record: foo\n expr: sum(foo) without(job)\n", - checker: checks.NewAlertsForCheck(), + checker: newAlertsForCheck, + problems: noProblems, }, { description: "invalid for value", content: "- alert: foo\n expr: foo\n for: abc\n", - checker: checks.NewAlertsForCheck(), - problems: []checks.Problem{ - { - Fragment: "abc", - Lines: []int{3}, - Reporter: "alerts/for", - Text: `invalid duration: not a valid duration string: "abc"`, - Severity: checks.Bug, - }, + checker: newAlertsForCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "abc", + Lines: []int{3}, + Reporter: "alerts/for", + Text: `invalid duration: not a valid duration string: "abc"`, + Severity: checks.Bug, + }, + } }, }, { description: "negative for value", content: "- alert: foo\n expr: foo\n for: -5m\n", - checker: checks.NewAlertsForCheck(), - problems: []checks.Problem{ - { - Fragment: "-5m", - Lines: []int{3}, - Reporter: "alerts/for", - Text: `invalid duration: not a valid duration string: "-5m"`, - Severity: checks.Bug, - }, + checker: newAlertsForCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "-5m", + Lines: []int{3}, + Reporter: "alerts/for", + Text: `invalid duration: not a valid duration string: "-5m"`, + Severity: checks.Bug, + }, + } }, }, { description: "default for value", content: "- alert: foo\n expr: foo\n for: 0h\n", - checker: checks.NewAlertsForCheck(), - problems: []checks.Problem{ - { - Fragment: "0h", - Lines: []int{3}, - Reporter: "alerts/for", - Text: `"0h" is the default value of "for", consider removing this line`, - Severity: checks.Information, - }, + checker: newAlertsForCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "0h", + Lines: []int{3}, + Reporter: "alerts/for", + Text: `"0h" is the default value of "for", consider removing this line`, + Severity: checks.Information, + }, + } }, }, } diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go index 3943158d..8d3af187 100644 --- a/internal/checks/alerts_template_test.go +++ b/internal/checks/alerts_template_test.go @@ -6,308 +6,355 @@ import ( "github.com/cloudflare/pint/internal/checks" ) +func newTemplateCheck(_ string) checks.RuleChecker { + return checks.NewTemplateCheck() +} + func TestTemplateCheck(t *testing.T) { testCases := []checkTest{ { description: "skips recording rule", content: "- record: foo\n expr: sum(foo)\n", - checker: checks.NewTemplateCheck(), + checker: newTemplateCheck, + problems: noProblems, }, { description: "invalid syntax in annotations", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n annotations:\n summary: 'Instance {{ $label.instance }} down'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: Instance {{ $label.instance }} down`, - Lines: []int{4}, - Reporter: "alerts/template", - Text: "template parse error: undefined variable \"$label\"", - Severity: checks.Fatal, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: Instance {{ $label.instance }} down`, + Lines: []int{4}, + Reporter: checks.TemplateCheckName, + Text: "template parse error: undefined variable \"$label\"", + Severity: checks.Fatal, + }, + } }, }, { description: "invalid function in annotations", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n annotations:\n summary: '{{ $value | xxx }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ $value | xxx }}`, - Lines: []int{4}, - Reporter: "alerts/template", - Text: "template parse error: function \"xxx\" not defined", - Severity: checks.Fatal, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ $value | xxx }}`, + Lines: []int{4}, + Reporter: checks.TemplateCheckName, + Text: "template parse error: function \"xxx\" not defined", + Severity: checks.Fatal, + }, + } }, }, { description: "valid syntax in annotations", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n annotations:\n summary: 'Instance {{ $labels.instance }} down'\n", - checker: checks.NewTemplateCheck(), + checker: newTemplateCheck, + problems: noProblems, }, { description: "invalid syntax in labels", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n labels:\n summary: 'Instance {{ $label.instance }} down'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: Instance {{ $label.instance }} down`, - Lines: []int{4}, - Reporter: "alerts/template", - Text: "template parse error: undefined variable \"$label\"", - Severity: checks.Fatal, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: Instance {{ $label.instance }} down`, + Lines: []int{4}, + Reporter: checks.TemplateCheckName, + Text: "template parse error: undefined variable \"$label\"", + Severity: checks.Fatal, + }, + } }, }, { description: "invalid function in annotations", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n labels:\n summary: '{{ $value | xxx }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ $value | xxx }}`, - Lines: []int{4}, - Reporter: "alerts/template", - Text: "template parse error: function \"xxx\" not defined", - Severity: checks.Fatal, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ $value | xxx }}`, + Lines: []int{4}, + Reporter: checks.TemplateCheckName, + Text: "template parse error: function \"xxx\" not defined", + Severity: checks.Fatal, + }, + } }, }, { description: "valid syntax in labels", content: "- alert: Foo Is Down\n expr: up{job=\"foo\"} == 0\n labels:\n summary: 'Instance {{ $labels.instance }} down'\n", - checker: checks.NewTemplateCheck(), + checker: newTemplateCheck, + problems: noProblems, }, { description: "{{ $value}} in label key", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n '{{ $value}}': bar\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "{{ $value}}: bar", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using $value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "{{ $value}}: bar", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using $value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ $value }} in label key", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n '{{ $value }}': bar\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "{{ $value }}: bar", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using $value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "{{ $value }}: bar", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using $value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{$value}} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: '{{$value}}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: {{$value}}", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using $value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: {{$value}}", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using $value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{$value}} in multiple labels", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: '{{ .Value }}'\n baz: '{{$value}}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "foo: {{ .Value }}", - Lines: []int{4}, - Reporter: "alerts/template", - Text: "using .Value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, - { - Fragment: "baz: {{$value}}", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using $value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "foo: {{ .Value }}", + Lines: []int{4}, + Reporter: checks.TemplateCheckName, + Text: "using .Value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + { + Fragment: "baz: {{$value}}", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using $value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ $value }} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: |\n foo is {{ $value | humanizePercentage }}%\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: foo is {{ $value | humanizePercentage }}%\n", - Lines: []int{5, 6}, - Reporter: "alerts/template", - Text: "using $value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: foo is {{ $value | humanizePercentage }}%\n", + Lines: []int{5, 6}, + Reporter: checks.TemplateCheckName, + Text: "using $value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ $value }} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: |\n foo is {{$value|humanizePercentage}}%\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: foo is {{$value|humanizePercentage}}%\n", - Lines: []int{5, 6}, - Reporter: "alerts/template", - Text: "using $value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: foo is {{$value|humanizePercentage}}%\n", + Lines: []int{5, 6}, + Reporter: checks.TemplateCheckName, + Text: "using $value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ .Value }} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: 'value {{ .Value }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: value {{ .Value }}", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using .Value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: value {{ .Value }}", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using .Value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ .Value|humanize }} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: '{{ .Value|humanize }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: {{ .Value|humanize }}", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using .Value in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: {{ .Value|humanize }}", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using .Value in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ $foo := $value }} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: '{{ $foo := $value }}{{ $foo }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: {{ $foo := $value }}{{ $foo }}", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using $foo in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: {{ $foo := $value }}{{ $foo }}", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using $foo in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "{{ $foo := .Value }} in label value", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n baz: '{{ $foo := .Value }}{{ $foo }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "baz: {{ $foo := .Value }}{{ $foo }}", - Lines: []int{5}, - Reporter: "alerts/template", - Text: "using $foo in labels will generate a new alert on every value change, move it to annotations", - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "baz: {{ $foo := .Value }}{{ $foo }}", + Lines: []int{5}, + Reporter: checks.TemplateCheckName, + Text: "using $foo in labels will generate a new alert on every value change, move it to annotations", + Severity: checks.Bug, + }, + } }, }, { description: "annotation label missing from metrics (by)", content: "- alert: Foo Is Down\n expr: sum(foo) > 0\n annotations:\n summary: '{{ $labels.job }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ $labels.job }}`, - Lines: []int{2, 4}, - Reporter: "alerts/template", - Text: `template is using "job" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ $labels.job }}`, + Lines: []int{2, 4}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { description: "annotation label missing from metrics (by)", content: "- alert: Foo Is Down\n expr: sum(foo) > 0\n annotations:\n summary: '{{ .Labels.job }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ .Labels.job }}`, - Lines: []int{2, 4}, - Reporter: "alerts/template", - Text: `template is using "job" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ .Labels.job }}`, + Lines: []int{2, 4}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { description: "annotation label missing from metrics (without)", content: "- alert: Foo Is Down\n expr: sum(foo) without(job) > 0\n annotations:\n summary: '{{ $labels.job }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ $labels.job }}`, - Lines: []int{2, 4}, - Reporter: "alerts/template", - Text: `template is using "job" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ $labels.job }}`, + Lines: []int{2, 4}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { description: "annotation label missing from metrics (without)", content: "- alert: Foo Is Down\n expr: sum(foo) without(job) > 0\n annotations:\n summary: '{{ .Labels.job }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ .Labels.job }}`, - Lines: []int{2, 4}, - Reporter: "alerts/template", - Text: `template is using "job" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ .Labels.job }}`, + Lines: []int{2, 4}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { description: "annotation label missing from metrics (or)", content: "- alert: Foo Is Down\n expr: sum(foo) by(job) or sum(bar)\n annotations:\n summary: '{{ .Labels.job }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ .Labels.job }}`, - Lines: []int{2, 4}, - Reporter: "alerts/template", - Text: `template is using "job" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ .Labels.job }}`, + Lines: []int{2, 4}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { description: "annotation label missing from metrics (1+)", content: "- alert: Foo Is Down\n expr: 1 + sum(foo) by(job) + sum(foo) by(notjob)\n annotations:\n summary: '{{ .Labels.job }}'\n", - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ .Labels.job }}`, - Lines: []int{2, 4}, - Reporter: "alerts/template", - Text: `template is using "job" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ .Labels.job }}`, + Lines: []int{2, 4}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { @@ -319,15 +366,17 @@ func TestTemplateCheck(t *testing.T) { summary: '{{ $labels.instance }} on {{ .Labels.foo }} is down' help: '{{ $labels.ixtance }}' `, - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `help: {{ $labels.ixtance }}`, - Lines: []int{3, 6}, - Reporter: "alerts/template", - Text: `template is using "ixtance" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `help: {{ $labels.ixtance }}`, + Lines: []int{3, 6}, + Reporter: checks.TemplateCheckName, + Text: `template is using "ixtance" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { @@ -338,7 +387,8 @@ func TestTemplateCheck(t *testing.T) { annotations: summary: '{{ $labels.instance }} on {{ .Labels.job }} is missing' `, - checker: checks.NewTemplateCheck(), + checker: newTemplateCheck, + problems: noProblems, }, { description: "annotation label missing from metrics (absent)", @@ -351,36 +401,38 @@ func TestTemplateCheck(t *testing.T) { summary: '{{ $labels.instance }} on {{ .Labels.foo }} is missing' help: '{{ $labels.xxx }}' `, - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: "instance: {{ $labels.instance }}", - Lines: []int{3, 5}, - Reporter: "alerts/template", - Text: `template is using "instance" label but absent() is not passing it`, - Severity: checks.Bug, - }, - { - Fragment: `summary: {{ $labels.instance }} on {{ .Labels.foo }} is missing`, - Lines: []int{3, 7}, - Reporter: "alerts/template", - Text: `template is using "instance" label but absent() is not passing it`, - Severity: checks.Bug, - }, - { - Fragment: `summary: {{ $labels.instance }} on {{ .Labels.foo }} is missing`, - Lines: []int{3, 7}, - Reporter: "alerts/template", - Text: `template is using "foo" label but absent() is not passing it`, - Severity: checks.Bug, - }, - { - Fragment: "help: {{ $labels.xxx }}", - Lines: []int{3, 8}, - Reporter: "alerts/template", - Text: `template is using "xxx" label but absent() is not passing it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "instance: {{ $labels.instance }}", + Lines: []int{3, 5}, + Reporter: checks.TemplateCheckName, + Text: `template is using "instance" label but absent() is not passing it`, + Severity: checks.Bug, + }, + { + Fragment: `summary: {{ $labels.instance }} on {{ .Labels.foo }} is missing`, + Lines: []int{3, 7}, + Reporter: checks.TemplateCheckName, + Text: `template is using "instance" label but absent() is not passing it`, + Severity: checks.Bug, + }, + { + Fragment: `summary: {{ $labels.instance }} on {{ .Labels.foo }} is missing`, + Lines: []int{3, 7}, + Reporter: checks.TemplateCheckName, + Text: `template is using "foo" label but absent() is not passing it`, + Severity: checks.Bug, + }, + { + Fragment: "help: {{ $labels.xxx }}", + Lines: []int{3, 8}, + Reporter: checks.TemplateCheckName, + Text: `template is using "xxx" label but absent() is not passing it`, + Severity: checks.Bug, + }, + } }, }, { @@ -391,7 +443,8 @@ func TestTemplateCheck(t *testing.T) { annotations: summary: '{{ $labels.instance }} on {{ .Labels.job }} is missing' `, - checker: checks.NewTemplateCheck(), + checker: newTemplateCheck, + problems: noProblems, }, { description: "annotation label missing from metrics (absent(sum))", @@ -401,15 +454,17 @@ func TestTemplateCheck(t *testing.T) { annotations: summary: '{{ $labels.instance }} on {{ .Labels.job }} is missing' `, - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ $labels.instance }} on {{ .Labels.job }} is missing`, - Lines: []int{3, 5}, - Reporter: "alerts/template", - Text: `template is using "instance" label but the query removes it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ $labels.instance }} on {{ .Labels.job }} is missing`, + Lines: []int{3, 5}, + Reporter: checks.TemplateCheckName, + Text: `template is using "instance" label but the query removes it`, + Severity: checks.Bug, + }, + } }, }, { @@ -420,15 +475,17 @@ func TestTemplateCheck(t *testing.T) { annotations: summary: '{{ .Labels.job }} is missing' `, - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ .Labels.job }} is missing`, - Lines: []int{3, 5}, - Reporter: "alerts/template", - Text: `template is using "job" label but absent() is not passing it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ .Labels.job }} is missing`, + Lines: []int{3, 5}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but absent() is not passing it`, + Severity: checks.Bug, + }, + } }, }, { @@ -439,22 +496,24 @@ func TestTemplateCheck(t *testing.T) { annotations: summary: '{{ .Labels.job }} / {{$labels.job}} is missing' `, - checker: checks.NewTemplateCheck(), - problems: []checks.Problem{ - { - Fragment: `summary: {{ .Labels.job }} / {{$labels.job}} is missing`, - Lines: []int{3, 5}, - Reporter: "alerts/template", - Text: `template is using "job" label but absent() is not passing it`, - Severity: checks.Bug, - }, - { - Fragment: `summary: {{ .Labels.job }} / {{$labels.job}} is missing`, - Lines: []int{3, 5}, - Reporter: "alerts/template", - Text: `template is using "job" label but absent() is not passing it`, - Severity: checks.Bug, - }, + checker: newTemplateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `summary: {{ .Labels.job }} / {{$labels.job}} is missing`, + Lines: []int{3, 5}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but absent() is not passing it`, + Severity: checks.Bug, + }, + { + Fragment: `summary: {{ .Labels.job }} / {{$labels.job}} is missing`, + Lines: []int{3, 5}, + Reporter: checks.TemplateCheckName, + Text: `template is using "job" label but absent() is not passing it`, + Severity: checks.Bug, + }, + } }, }, } diff --git a/internal/checks/base_test.go b/internal/checks/base_test.go index c2fee12f..f3c19293 100644 --- a/internal/checks/base_test.go +++ b/internal/checks/base_test.go @@ -2,76 +2,23 @@ package checks_test import ( "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "sync" "testing" "time" - "github.com/google/go-cmp/cmp" - "github.com/rs/zerolog" + v1 "github.com/prometheus/client_golang/api/prometheus/v1" + "github.com/prometheus/common/model" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/promapi" ) -type checkTest struct { - description string - content string - checker checks.RuleChecker - problems []checks.Problem -} - -func runTests(t *testing.T, testCases []checkTest, opts ...cmp.Option) { - zerolog.SetGlobalLevel(zerolog.FatalLevel) - - p := parser.NewParser() - ctx := context.Background() - for _, tc := range testCases { - // original test - t.Run(tc.description, func(t *testing.T) { - rules, err := p.Parse([]byte(tc.content)) - if err != nil { - t.Fatal(err) - } - for _, rule := range rules { - problems := tc.checker.Check(ctx, rule) - if diff := cmp.Diff(tc.problems, problems, opts...); diff != "" { - t.Fatalf("Check() returned wrong problem list (-want +got):\n%s", diff) - } - } - }) - - // broken alerting rule to test check against rules with syntax error - t.Run(tc.description+" (bogus alerting rule)", func(t *testing.T) { - rules, err := p.Parse([]byte(` -- alert: foo - expr: 'foo{}{} > 0' - annotations: - summary: '{{ $labels.job }} is incorrect' -`)) - if err != nil { - t.Fatal(err) - } - for _, rule := range rules { - _ = tc.checker.Check(ctx, rule) - } - }) - - // broken recording rule to test check against rules with syntax error - t.Run(tc.description+" (bogus recording rule)", func(t *testing.T) { - rules, err := p.Parse([]byte(` -- record: foo - expr: 'foo{}{}' -`)) - if err != nil { - t.Fatal(err) - } - for _, rule := range rules { - _ = tc.checker.Check(ctx, rule) - } - }) - } -} - func TestParseSeverity(t *testing.T) { type testCaseT struct { input string @@ -117,3 +64,322 @@ func simpleProm(name, uri string, timeout time.Duration, required bool) *promapi required, ) } + +type newCheckFn func(string) checks.RuleChecker + +type problemsFn func(string) []checks.Problem + +type checkTest struct { + description string + content string + checker newCheckFn + problems problemsFn + mocks []*prometheusMock +} + +func runTests(t *testing.T, testCases []checkTest) { + p := parser.NewParser() + brokenRules, err := p.Parse([]byte(` +- alert: foo + expr: 'foo{}{} > 0' + annotations: + summary: '{{ $labels.job }} is incorrect' + +- record: foo + expr: 'foo{}{}' +`)) + require.NoError(t, err, "failed to parse broken test rules") + + ctx := context.Background() + for _, tc := range testCases { + // original test + t.Run(tc.description, func(t *testing.T) { + var uri string + if len(tc.mocks) > 0 { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for i := range tc.mocks { + if tc.mocks[i].maybeApply(w, r) { + return + } + } + t.Errorf("no matching response for %s request", r.URL.Path) + t.FailNow() + })) + defer srv.Close() + uri = srv.URL + } + + rules, err := p.Parse([]byte(tc.content)) + require.NoError(t, err, "cannot parse rule content") + for _, rule := range rules { + problems := tc.checker(uri).Check(ctx, rule) + require.Equal(t, tc.problems(uri), problems) + } + + // verify that all mocks were used + for _, mock := range tc.mocks { + require.True(t, mock.wasUsed(), "unused mock in %s: %s", tc.description, mock.conds) + } + }) + + // broken rules to test check against rules with syntax error + t.Run(tc.description+" (bogus rules)", func(t *testing.T) { + for _, rule := range brokenRules { + _ = tc.checker("").Check(ctx, rule) + } + }) + } +} + +func noProblems(uri string) []checks.Problem { + return nil +} + +type requestCondition interface { + isMatch(*http.Request) bool +} + +type responseWriter interface { + respond(w http.ResponseWriter) +} + +type prometheusMock struct { + conds []requestCondition + resp responseWriter + used bool + mu sync.Mutex +} + +func (pm *prometheusMock) maybeApply(w http.ResponseWriter, r *http.Request) bool { + for _, cond := range pm.conds { + if !cond.isMatch(r) { + return false + } + } + pm.markUsed() + pm.resp.respond(w) + return true +} + +func (pm *prometheusMock) markUsed() { + pm.mu.Lock() + defer pm.mu.Unlock() + pm.used = true +} + +func (pm *prometheusMock) wasUsed() bool { + pm.mu.Lock() + defer pm.mu.Unlock() + return pm.used +} + +type requestPathCond struct { + path string +} + +func (rpc requestPathCond) isMatch(r *http.Request) bool { + return r.URL.Path == rpc.path +} + +type formCond struct { + key string + value string +} + +func (fc formCond) isMatch(r *http.Request) bool { + err := r.ParseForm() + if err != nil { + return false + } + return r.Form.Get(fc.key) == fc.value +} + +var ( + requireConfigPath = requestPathCond{path: "/api/v1/status/config"} + requireQueryPath = requestPathCond{path: "/api/v1/query"} + requireRangeQueryPath = requestPathCond{path: "/api/v1/query_range"} +) + +type promError struct { + code int + errorType v1.ErrorType + err string +} + +func (pe promError) respond(w http.ResponseWriter) { + w.WriteHeader(pe.code) + w.Header().Set("Content-Type", "application/json") + perr := struct { + Status string `json:"status"` + ErrorType v1.ErrorType `json:"errorType"` + Error string `json:"error"` + }{ + Status: "error", + ErrorType: pe.errorType, + Error: pe.err, + } + d, err := json.MarshalIndent(perr, "", " ") + if err != nil { + panic(err) + } + _, _ = w.Write(d) +} + +type vectorResponse struct { + samples model.Vector +} + +func (vr vectorResponse) respond(w http.ResponseWriter) { + w.WriteHeader(200) + w.Header().Set("Content-Type", "application/json") + result := struct { + Status string `json:"status"` + Data struct { + ResultType string `json:"resultType"` + Result model.Vector `json:"result"` + } `json:"data"` + }{ + Status: "success", + Data: struct { + ResultType string `json:"resultType"` + Result model.Vector `json:"result"` + }{ + ResultType: "vector", + Result: vr.samples, + }, + } + d, err := json.MarshalIndent(result, "", " ") + if err != nil { + panic(err) + } + _, _ = w.Write(d) +} + +type matrixResponse struct { + samples []*model.SampleStream +} + +func (mr matrixResponse) respond(w http.ResponseWriter) { + w.WriteHeader(200) + w.Header().Set("Content-Type", "application/json") + result := struct { + Status string `json:"status"` + Data struct { + ResultType string `json:"resultType"` + Result []*model.SampleStream `json:"result"` + } `json:"data"` + }{ + Status: "success", + Data: struct { + ResultType string `json:"resultType"` + Result []*model.SampleStream `json:"result"` + }{ + ResultType: "matrix", + Result: mr.samples, + }, + } + d, err := json.MarshalIndent(result, "", " ") + if err != nil { + panic(err) + } + _, _ = w.Write(d) +} + +type configResponse struct { + yaml string +} + +func (cr configResponse) respond(w http.ResponseWriter) { + w.WriteHeader(200) + w.Header().Set("Content-Type", "application/json") + result := struct { + Status string `json:"status"` + Data v1.ConfigResult `json:"data"` + }{ + Status: "success", + Data: v1.ConfigResult{YAML: cr.yaml}, + } + d, err := json.MarshalIndent(result, "", " ") + if err != nil { + panic(err) + } + _, _ = w.Write(d) +} + +type sleepResponse struct { + sleep time.Duration +} + +func (sr sleepResponse) respond(w http.ResponseWriter) { + time.Sleep(sr.sleep) +} + +var ( + respondWithBadData = func() responseWriter { + return promError{code: 400, errorType: v1.ErrBadData, err: "bad input data"} + } + respondWithInternalError = func() responseWriter { + return promError{code: 500, errorType: v1.ErrServer, err: "internal error"} + } + respondWithEmptyVector = func() responseWriter { + return vectorResponse{samples: model.Vector{}} + } + respondWithEmptyMatrix = func() responseWriter { + return matrixResponse{samples: []*model.SampleStream{}} + } + respondWithSingleInstantVector = func() responseWriter { + return vectorResponse{ + samples: []*model.Sample{generateSample(map[string]string{})}, + } + } + respondWithSingleRangeVector1W = func() responseWriter { + return matrixResponse{ + samples: []*model.SampleStream{ + generateSampleStream( + map[string]string{}, + time.Now().Add(time.Hour*24*-7), + time.Now(), + time.Minute*5, + ), + }, + } + } +) + +func generateSample(labels map[string]string) *model.Sample { + metric := model.Metric{} + for k, v := range labels { + metric[model.LabelName(k)] = model.LabelValue(v) + } + return &model.Sample{ + Metric: metric, + Value: model.SampleValue(1), + Timestamp: model.TimeFromUnix(time.Now().Unix()), + } +} + +func generateSampleStream(labels map[string]string, from, until time.Time, step time.Duration) (s *model.SampleStream) { + metric := model.Metric{} + for k, v := range labels { + metric[model.LabelName(k)] = model.LabelValue(v) + } + s = &model.SampleStream{ + Metric: metric, + } + for from.Before(until) { + s.Values = append(s.Values, model.SamplePair{ + Timestamp: model.TimeFromUnix(from.Unix()), + Value: 1, + }) + from = from.Add(step) + } + return +} + +func checkErrorBadData(name, uri, err string) string { + return fmt.Sprintf(`prometheus %q at %s failed with: %s`, name, uri, err) +} + +func checkErrorUnableToRun(c, name, uri, err string) string { + return fmt.Sprintf(`cound't run %q checks due to prometheus %q at %s connection error: %s`, c, name, uri, err) +} diff --git a/internal/checks/check_test.go b/internal/checks/check_test.go deleted file mode 100644 index f25624b4..00000000 --- a/internal/checks/check_test.go +++ /dev/null @@ -1,286 +0,0 @@ -package checks_test - -import ( - "context" - "encoding/json" - "fmt" - "net/http" - "net/http/httptest" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - v1 "github.com/prometheus/client_golang/api/prometheus/v1" - "github.com/prometheus/common/model" - "github.com/stretchr/testify/require" - - "github.com/cloudflare/pint/internal/checks" - "github.com/cloudflare/pint/internal/parser" -) - -type newCheckFn func(string) checks.RuleChecker - -type problemsFn func(string) []checks.Problem - -type checkTestT struct { - description string - content string - checker newCheckFn - problems problemsFn - mocks []prometheusMock -} - -func runTestsT(t *testing.T, testCases []checkTestT, opts ...cmp.Option) { - p := parser.NewParser() - brokenRules, err := p.Parse([]byte(` -- alert: foo - expr: 'foo{}{} > 0' - annotations: - summary: '{{ $labels.job }} is incorrect' - -- record: foo - expr: 'foo{}{}' -`)) - require.NoError(t, err, "failed to parse broken test rules") - - ctx := context.Background() - for _, tc := range testCases { - // original test - t.Run(tc.description, func(t *testing.T) { - var uri string - if len(tc.mocks) > 0 { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - for i := range tc.mocks { - if tc.mocks[i].maybeApply(w, r) { - tc.mocks[i].wasUsed = true - return - } - } - t.Errorf("no matching response for %s request", r.URL.Path) - t.FailNow() - })) - defer srv.Close() - uri = srv.URL - } - - rules, err := p.Parse([]byte(tc.content)) - require.NoError(t, err, "cannot parse rule content") - for _, rule := range rules { - problems := tc.checker(uri).Check(ctx, rule) - require.Equal(t, tc.problems(uri), problems) - } - - // verify that all mocks were used - for _, mock := range tc.mocks { - require.True(t, mock.wasUsed, "unused mock in %s: %s", tc.description, mock.conds) - } - }) - - // broken rules to test check against rules with syntax error - t.Run(tc.description+" (bogus rules)", func(t *testing.T) { - for _, rule := range brokenRules { - _ = tc.checker("").Check(ctx, rule) - } - }) - } -} - -func noProblems(uri string) []checks.Problem { - return nil -} - -type requestCondition interface { - isMatch(*http.Request) bool -} - -type responseWriter interface { - respond(w http.ResponseWriter) -} - -type prometheusMock struct { - conds []requestCondition - resp responseWriter - wasUsed bool -} - -func (pm *prometheusMock) maybeApply(w http.ResponseWriter, r *http.Request) bool { - for _, cond := range pm.conds { - if !cond.isMatch(r) { - return false - } - } - pm.wasUsed = true - pm.resp.respond(w) - return true -} - -type requestPathCond struct { - path string -} - -func (rpc requestPathCond) isMatch(r *http.Request) bool { - return r.URL.Path == rpc.path -} - -type formCond struct { - key string - value string -} - -func (fc formCond) isMatch(r *http.Request) bool { - err := r.ParseForm() - if err != nil { - return false - } - return r.Form.Get(fc.key) == fc.value -} - -var ( - // requireConfigPath = requestPathCond{path: "/api/v1/config"} - requireQueryPath = requestPathCond{path: "/api/v1/query"} - requireRangeQueryPath = requestPathCond{path: "/api/v1/query_range"} -) - -type promError struct { - code int - errorType v1.ErrorType - err string -} - -func (pe promError) respond(w http.ResponseWriter) { - w.WriteHeader(pe.code) - w.Header().Set("Content-Type", "application/json") - perr := struct { - Status string `json:"status"` - ErrorType v1.ErrorType `json:"errorType"` - Error string `json:"error"` - }{ - Status: "error", - ErrorType: pe.errorType, - Error: pe.err, - } - d, err := json.MarshalIndent(perr, "", " ") - if err != nil { - panic(err) - } - _, _ = w.Write(d) -} - -type vectorResponse struct { - samples model.Vector -} - -func (vr vectorResponse) respond(w http.ResponseWriter) { - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - result := struct { - Status string `json:"status"` - Data struct { - ResultType string `json:"resultType"` - Result model.Vector `json:"result"` - } `json:"data"` - }{ - Status: "success", - Data: struct { - ResultType string `json:"resultType"` - Result model.Vector `json:"result"` - }{ - ResultType: "vector", - Result: vr.samples, - }, - } - d, err := json.MarshalIndent(result, "", " ") - if err != nil { - panic(err) - } - _, _ = w.Write(d) -} - -type matrixResponse struct { - samples []*model.SampleStream -} - -func (mr matrixResponse) respond(w http.ResponseWriter) { - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - result := struct { - Status string `json:"status"` - Data struct { - ResultType string `json:"resultType"` - Result []*model.SampleStream `json:"result"` - } `json:"data"` - }{ - Status: "success", - Data: struct { - ResultType string `json:"resultType"` - Result []*model.SampleStream `json:"result"` - }{ - ResultType: "matrix", - Result: mr.samples, - }, - } - d, err := json.MarshalIndent(result, "", " ") - if err != nil { - panic(err) - } - _, _ = w.Write(d) -} - -var ( - respondWithBadData = promError{code: 400, errorType: v1.ErrBadData, err: "bad input data"} - respondWithInternalError = promError{code: 500, errorType: v1.ErrServer, err: "internal error"} - respondWithEmptyVector = vectorResponse{samples: model.Vector{}} - respondWithEmptyMatrix = matrixResponse{samples: []*model.SampleStream{}} - respondWithSingleInstantVector = vectorResponse{ - samples: generateVector(map[string]string{}), - } - respondWithSingleRangeVector1W = matrixResponse{ - samples: []*model.SampleStream{ - generateSampleStream( - map[string]string{}, - time.Now().Add(time.Hour*24*-7), - time.Now(), - time.Minute*5, - ), - }, - } -) - -func generateVector(labels map[string]string) (v model.Vector) { - metric := model.Metric{} - for k, v := range labels { - metric[model.LabelName(k)] = model.LabelValue(v) - } - v = append(v, &model.Sample{ - Metric: metric, - Value: model.SampleValue(1), - Timestamp: model.TimeFromUnix(time.Now().Unix()), - }) - return -} - -func generateSampleStream(labels map[string]string, from, until time.Time, step time.Duration) (s *model.SampleStream) { - metric := model.Metric{} - for k, v := range labels { - metric[model.LabelName(k)] = model.LabelValue(v) - } - s = &model.SampleStream{ - Metric: metric, - } - for from.Before(until) { - s.Values = append(s.Values, model.SamplePair{ - Timestamp: model.TimeFromUnix(from.Unix()), - Value: 1, - }) - from = from.Add(step) - } - return -} - -func checkErrorBadData(name, uri, err string) string { - return fmt.Sprintf(`prometheus %q at %s failed with: %s`, name, uri, err) -} - -func checkErrorUnableToRun(c, name, uri, err string) string { - return fmt.Sprintf(`cound't run %q checks due to prometheus %q at %s connection error: %s`, c, name, uri, err) -} diff --git a/internal/checks/promql_aggregation_test.go b/internal/checks/promql_aggregation_test.go index 43d4a510..9249ae81 100644 --- a/internal/checks/promql_aggregation_test.go +++ b/internal/checks/promql_aggregation_test.go @@ -11,450 +11,605 @@ func TestAggregationCheck(t *testing.T) { { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "name must match / recording", content: "- record: foo\n expr: sum(foo) without(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp("bar"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp("bar"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "name must match /alerting", content: "- alert: foo\n expr: sum(foo) without(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp("bar"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp("bar"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "uses label from labels map / recording", content: "- record: foo\n expr: sum(foo) without(job)\n labels:\n job: foo\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "uses label from labels map / alerting", content: "- alert: foo\n expr: sum(foo) without(job)\n labels:\n job: foo\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "must keep job label / warning", content: "- record: foo\n expr: sum(foo) without(instance, job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo) without(instance, job)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) without(instance, job)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "must keep job label / bug", content: "- record: foo\n expr: sum(foo) without(instance, job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Bug), - problems: []checks.Problem{ - { - Fragment: "sum(foo) without(instance, job)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Bug, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Bug) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) without(instance, job)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Bug, + }, + } }, }, { description: "must strip job label", content: "- record: foo\n expr: sum(foo) without(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo) without(instance)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label should be removed when aggregating "^.+$" rules, use without(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) without(instance)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label should be removed when aggregating "^.+$" rules, use without(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "must strip job label / being stripped", content: "- record: foo\n expr: sum(foo) without(instance,job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning) + }, + problems: noProblems, }, { description: "must strip job label / empty without", content: "- record: foo\n expr: sum(foo)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning) + }, + problems: noProblems, }, { description: "nested rule must keep job label", content: "- record: foo\n expr: sum(sum(foo) without(job)) by(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum without(job) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum without(job) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "passing most outer aggregation should stop further strip checks", content: "- record: foo\n expr: sum(sum(foo) without(foo)) without(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning) + }, + problems: noProblems, }, { description: "passing most outer aggregation should stop further checks", content: "- record: foo\n expr: sum(sum(foo) without(foo)) without(bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(sum(foo) without(foo)) without(bar)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `instance label should be removed when aggregating "^.+$" rules, use without(instance, ...)`, - Severity: checks.Warning, - }, - { - Fragment: "sum without(foo) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `instance label should be removed when aggregating "^.+$" rules, use without(instance, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(sum(foo) without(foo)) without(bar)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `instance label should be removed when aggregating "^.+$" rules, use without(instance, ...)`, + Severity: checks.Warning, + }, + { + Fragment: "sum without(foo) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `instance label should be removed when aggregating "^.+$" rules, use without(instance, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "passing most outer aggregation should continue further keep checks", content: "- record: foo\n expr: sum(sum(foo) without(job)) without(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum without(job) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum without(job) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "Right hand side of AND is ignored", content: "- record: foo\n expr: foo AND on(instance) max(bar) without()\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "Left hand side of AND is checked", content: "- record: foo\n expr: max (foo) without(job) AND on(instance) bar\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "max without(job) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "max without(job) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "Right hand side of group_left() is ignored", content: "- record: foo\n expr: sum without(id) (foo) / on(type) group_left() sum without(job) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "Left hand side of group_left() is checked", content: "- record: foo\n expr: sum without(job) (foo) / on(type) group_left() sum without(job) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum without(job) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum without(job) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "Left hand side of group_right() is ignored", content: "- record: foo\n expr: sum without(job) (foo) / on(type) group_right() sum without(id) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "Right hand side of group_right() is checked", content: "- record: foo\n expr: sum without(job) (foo) / on(type) group_right() sum without(job) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum without(job) (bar)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum without(job) (bar)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "nested count", content: "- record: foo\n expr: count(count(bar) without ())\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning) + }, + problems: noProblems, }, { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "name must match", content: "- record: foo\n expr: sum(foo) without(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp("bar"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp("bar"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "uses label from labels map", content: "- record: foo\n expr: sum(foo) by(instance)\n labels:\n job: foo\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "must keep job label / warning", content: "- record: foo\n expr: sum(foo) by(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo) by(instance)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) by(instance)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "must keep job label / bug", content: "- record: foo\n expr: sum(foo) by(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Bug), - problems: []checks.Problem{ - { - Fragment: "sum(foo) by(instance)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Bug, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Bug) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) by(instance)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Bug, + }, + } }, }, { description: "must strip job label", content: "- record: foo\n expr: sum(foo) by(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo) by(job)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label should be removed when aggregating "^.+$" rules, remove job from by()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) by(job)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label should be removed when aggregating "^.+$" rules, remove job from by()`, + Severity: checks.Warning, + }, + } }, }, { description: "must strip job label / being stripped", content: "- record: foo\n expr: sum(foo) by(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning) + }, + problems: noProblems, }, { description: "nested rule must keep job label", content: "- record: foo\n expr: sum(sum(foo) by(instance)) by(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum by(instance) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum by(instance) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "Right hand side of AND is ignored", content: "- record: foo\n expr: foo AND on(instance) max(bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "Left hand side of AND is checked", content: "- record: foo\n expr: max (foo) by(instance) AND on(instance) bar\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "max by(instance) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "max by(instance) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "Right hand side of group_left() is ignored", content: "- record: foo\n expr: sum by(job) (foo) / on(type) group_left() sum by(type) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "Left hand side of group_left() is checked", content: "- record: foo\n expr: sum by(type) (foo) / on(type) group_left() sum by(job) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum by(type) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum by(type) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "Left hand side of group_right() is ignored", content: "- record: foo\n expr: sum by(type) (foo) / on(type) group_right() sum by(job) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: noProblems, }, { description: "Right hand side of group_right() is checked", content: "- record: foo\n expr: sum by(job) (foo) / on(type) group_right() sum by(type) (bar)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum by(type) (bar)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum by(type) (bar)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "nested count", content: "- record: foo\n expr: count(count(bar) by (instance))\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning) + }, + problems: noProblems, }, { description: "nested count AND nested count", content: "- record: foo\n expr: count(count(bar) by (instance)) AND count(count(bar) by (instance))\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning), + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning) + }, + problems: noProblems, }, { description: "nested by(without())", content: "- record: foo\n expr: sum(sum(foo) by(instance)) without(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(sum(foo) by(instance)) without(job)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, - { - Fragment: "sum by(instance) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(sum(foo) by(instance)) without(job)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + { + Fragment: "sum by(instance) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "nested by(without())", content: "- record: foo\n expr: sum(sum(foo) by(instance,job)) without(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(sum(foo) by(instance,job)) without(job)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(sum(foo) by(instance,job)) without(job)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, remove job from without()`, + Severity: checks.Warning, + }, + } }, }, { description: "nested by(without())", content: "- record: foo\n expr: sum(sum(foo) by(instance)) without(instance)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum by(instance) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum by(instance) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "nested by(without())", content: "- record: foo\n expr: sum(sum(foo) by(instance)) without(job)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(sum(foo) by(instance)) without(job)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `instance label should be removed when aggregating "^.+$" rules, use without(instance, ...)`, - Severity: checks.Warning, - }, - { - Fragment: "sum by(instance) (foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `instance label should be removed when aggregating "^.+$" rules, remove instance from by()`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "instance", false, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(sum(foo) by(instance)) without(job)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `instance label should be removed when aggregating "^.+$" rules, use without(instance, ...)`, + Severity: checks.Warning, + }, + { + Fragment: "sum by(instance) (foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `instance label should be removed when aggregating "^.+$" rules, remove instance from by()`, + Severity: checks.Warning, + }, + } }, }, { description: "must keep job label / sum()", content: "- record: foo\n expr: sum(foo)\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "must keep job label / sum() by()", content: "- record: foo\n expr: sum(foo) by()\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo) by()", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", true, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) by()", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...)`, + Severity: checks.Warning, + }, + } }, }, { description: "must strip job label / sum() without()", content: "- record: foo\n expr: sum(foo) without()\n", - checker: checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "sum(foo) without()", - Lines: []int{2}, - Reporter: "promql/aggregate", - Text: `job label should be removed when aggregating "^.+$" rules, use without(job, ...)`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewAggregationCheck(checks.MustTemplatedRegexp(".+"), "job", false, checks.Warning) + }, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) without()", + Lines: []int{2}, + Reporter: checks.AggregationCheckName, + Text: `job label should be removed when aggregating "^.+$" rules, use without(job, ...)`, + Severity: checks.Warning, + }, + } }, }, } diff --git a/internal/checks/promql_fragile_test.go b/internal/checks/promql_fragile_test.go index a634ea46..6a305394 100644 --- a/internal/checks/promql_fragile_test.go +++ b/internal/checks/promql_fragile_test.go @@ -6,6 +6,10 @@ import ( "github.com/cloudflare/pint/internal/checks" ) +func newFragileCheck(_ string) checks.RuleChecker { + return checks.NewFragileCheck() +} + func TestFragileCheck(t *testing.T) { text := "aggregation using without() can be fragile when used inside binary expression because both sides must have identical sets of labels to produce any results, adding or removing labels to metrics used here can easily break the query, consider aggregating using by() to ensure consistent labels" @@ -13,107 +17,124 @@ func TestFragileCheck(t *testing.T) { { description: "ignores syntax errors", content: "- record: foo\n expr: up ==\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, { description: "ignores simple comparison", content: "- record: foo\n expr: up == 0\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, { description: "ignores simple division", content: "- record: foo\n expr: foo / bar\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, { description: "ignores unless", content: "- record: foo\n expr: foo unless sum(bar) without(job)\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, { description: "ignores safe division", content: "- record: foo\n expr: foo / sum(bar)\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, { description: "warns about fragile division", content: "- record: foo\n expr: foo / sum(bar) without(job)\n", - checker: checks.NewFragileCheck(), - problems: []checks.Problem{ - { - Fragment: `foo / sum(bar) without(job)`, - Lines: []int{2}, - Reporter: "promql/fragile", - Text: text, - Severity: checks.Warning, - }, + checker: newFragileCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `foo / sum(bar) without(job)`, + Lines: []int{2}, + Reporter: "promql/fragile", + Text: text, + Severity: checks.Warning, + }, + } }, }, { description: "warns about fragile sum", content: "- record: foo\n expr: sum(foo) without(job) + sum(bar) without(job)\n", - checker: checks.NewFragileCheck(), - problems: []checks.Problem{ - { - Fragment: `sum(foo) without(job) + sum(bar) without(job)`, - Lines: []int{2}, - Reporter: "promql/fragile", - Text: text, - Severity: checks.Warning, - }, + checker: newFragileCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `sum(foo) without(job) + sum(bar) without(job)`, + Lines: []int{2}, + Reporter: "promql/fragile", + Text: text, + Severity: checks.Warning, + }, + } }, }, { description: "warns about fragile sum inside a condition", content: "- alert: foo\n expr: (sum(foo) without(job) + sum(bar) without(job)) > 1\n", - checker: checks.NewFragileCheck(), - problems: []checks.Problem{ - { - Fragment: `(sum(foo) without(job) + sum(bar) without(job)) > 1`, - Lines: []int{2}, - Reporter: "promql/fragile", - Text: text, - Severity: checks.Warning, - }, + checker: newFragileCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `(sum(foo) without(job) + sum(bar) without(job)) > 1`, + Lines: []int{2}, + Reporter: "promql/fragile", + Text: text, + Severity: checks.Warning, + }, + } }, }, { description: "warns about fragile division inside a condition", content: "- alert: foo\n expr: (foo / sum(bar) without(job)) > 1\n", - checker: checks.NewFragileCheck(), - problems: []checks.Problem{ - { - Fragment: `foo / sum without(job) (bar)`, - Lines: []int{2}, - Reporter: "promql/fragile", - Text: text, - Severity: checks.Warning, - }, + checker: newFragileCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `foo / sum without(job) (bar)`, + Lines: []int{2}, + Reporter: "promql/fragile", + Text: text, + Severity: checks.Warning, + }, + } }, }, { description: "warns about fragile sum inside a complex rule", content: "- alert: foo\n expr: (sum(foo) without(job) + sum(bar)) > 1 unless sum(bob) without(job) < 10\n", - checker: checks.NewFragileCheck(), - problems: []checks.Problem{ - { - Fragment: `(sum without(job) (foo) + sum(bar)) > 1`, - Lines: []int{2}, - Reporter: "promql/fragile", - Text: text, - Severity: checks.Warning, - }, + checker: newFragileCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `(sum without(job) (foo) + sum(bar)) > 1`, + Lines: []int{2}, + Reporter: "promql/fragile", + Text: text, + Severity: checks.Warning, + }, + } }, }, { description: "ignores safe division", content: "- record: foo\n expr: sum(foo) + sum(bar)\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, { description: "ignores division if source metric is the same", content: "- record: foo\n expr: sum(foo) without(bar) + sum(foo) without(bar)\n", - checker: checks.NewFragileCheck(), + checker: newFragileCheck, + problems: noProblems, }, } diff --git a/internal/checks/promql_rate_test.go b/internal/checks/promql_rate_test.go index ca15b309..51ab6498 100644 --- a/internal/checks/promql_rate_test.go +++ b/internal/checks/promql_rate_test.go @@ -2,108 +2,129 @@ package checks_test import ( "fmt" - "net/http" - "net/http/httptest" "testing" "time" "github.com/cloudflare/pint/internal/checks" ) -func TestRateCheck(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.URL.Path { - case "/30s/api/v1/status/config": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"success","data":{"yaml":"global:\n scrape_interval: 30s\n"}}`)) - case "/1m/api/v1/status/config": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"success","data":{"yaml":"global:\n scrape_interval: 1m\n"}}`)) - case "/default/api/v1/status/config": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"success","data":{"yaml":"global:\n {}\n"}}`)) - case "/error/api/v1/status/config": - w.WriteHeader(500) - _, _ = w.Write([]byte("fake error\n")) - case "/badYaml/api/v1/status/config": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"success","data":{"yaml":"invalid yaml"}}`)) - default: - w.WriteHeader(400) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"error","errorType":"bad_data","error":"unhandled path"}`)) - } - })) - defer srv.Close() +func newRateCheck(uri string) checks.RuleChecker { + return checks.NewRateCheck(simpleProm("prom", uri, time.Second, true)) +} + +func durationMustText(name, uri, fun, multi, using string) string { + return fmt.Sprintf(`duration for %s() must be at least %s x scrape_interval, prometheus %q at %s is using %s scrape_interval`, fun, multi, name, uri, using) +} + +func durationRecommenedText(name, uri, fun, multi, using string) string { + return fmt.Sprintf("duration for %s() is recommended to be at least %s x scrape_interval, prometheus %q at %s is using %s scrape_interval", fun, multi, name, uri, using) +} +func TestRateCheck(t *testing.T) { testCases := []checkTest{ { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newRateCheck, + problems: noProblems, }, { description: "rate < 2x scrape_interval", content: "- record: foo\n expr: rate(foo[1m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[1m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationMustText("prom", uri, "rate", "2", "1m"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "rate(foo[1m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for rate() must be at least 2 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Bug, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, { description: "rate < 4x scrape_interval", content: "- record: foo\n expr: rate(foo[3m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[3m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationRecommenedText("prom", uri, "rate", "4", "1m"), + Severity: checks.Warning, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "rate(foo[3m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for rate() is recommended to be at least 4 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Warning, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, { description: "rate == 4x scrape interval", content: "- record: foo\n expr: rate(foo[2m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/30s/", time.Second, true)), + checker: newRateCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 30s\n"}, + }, + }, }, { description: "irate < 2x scrape_interval", content: "- record: foo\n expr: irate(foo[1m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "irate(foo[1m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationMustText("prom", uri, "irate", "2", "1m"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "irate(foo[1m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for irate() must be at least 2 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Bug, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, { description: "irate < 3x scrape_interval", content: "- record: foo\n expr: irate(foo[2m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "irate(foo[2m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationRecommenedText("prom", uri, "irate", "3", "1m"), + Severity: checks.Warning, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "irate(foo[2m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for irate() is recommended to be at least 3 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Warning, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, @@ -113,7 +134,14 @@ func TestRateCheck(t *testing.T) { - record: foo expr: irate({__name__="foo"}[5m]) `, - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), + checker: newRateCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, + }, + }, }, { description: "irate{__name__=~} > 3x scrape_interval", @@ -121,7 +149,14 @@ func TestRateCheck(t *testing.T) { - record: foo expr: irate({__name__=~"(foo|bar)_total"}[5m]) `, - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), + checker: newRateCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, + }, + }, }, { description: "irate{__name__} < 3x scrape_interval", @@ -129,14 +164,22 @@ func TestRateCheck(t *testing.T) { - record: foo expr: irate({__name__="foo"}[2m]) `, - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `irate({__name__="foo"}[2m])`, + Lines: []int{3}, + Reporter: "promql/rate", + Text: durationRecommenedText("prom", uri, "irate", "3", "1m"), + Severity: checks.Warning, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `irate({__name__="foo"}[2m])`, - Lines: []int{3}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for irate() is recommended to be at least 3 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Warning, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, @@ -146,120 +189,194 @@ func TestRateCheck(t *testing.T) { - record: foo expr: irate({__name__=~"(foo|bar)_total"}[2m]) `, - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `irate({__name__=~"(foo|bar)_total"}[2m])`, + Lines: []int{3}, + Reporter: "promql/rate", + Text: durationRecommenedText("prom", uri, "irate", "3", "1m"), + Severity: checks.Warning, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `irate({__name__=~"(foo|bar)_total"}[2m])`, - Lines: []int{3}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for irate() is recommended to be at least 3 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Warning, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, { description: "irate == 3x scrape interval", content: "- record: foo\n expr: irate(foo[3m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), + checker: newRateCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, + }, + }, }, { description: "valid range selector", content: "- record: foo\n expr: foo[1m]\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), + checker: newRateCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, + }, + }, }, { description: "nested invalid rate", content: "- record: foo\n expr: sum(rate(foo[3m])) / sum(rate(bar[1m]))\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/1m/", time.Second, true)), - problems: []checks.Problem{ - { - Fragment: "rate(foo[3m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for rate() is recommended to be at least 4 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Warning, - }, + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[3m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationRecommenedText("prom", uri, "rate", "4", "1m"), + Severity: checks.Warning, + }, + { + Fragment: "rate(bar[1m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationMustText("prom", uri, "rate", "2", "1m"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "rate(bar[1m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for rate() must be at least 2 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/1m/"), - Severity: checks.Bug, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, }, }, }, { description: "500 error from Prometheus API", content: "- record: foo\n expr: rate(foo[5m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/error/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[5m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: checkErrorUnableToRun(checks.RateCheckName, "prom", uri, "failed to query Prometheus config: server_error: server error: 500"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "rate(foo[5m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`cound't run "promql/rate" checks due to prometheus "prom" at %s/error/ connection error: failed to query Prometheus config: server_error: server error: 500`, srv.URL), - Severity: checks.Bug, + conds: []requestCondition{requireConfigPath}, + resp: respondWithInternalError(), }, }, }, { description: "invalid status", content: "- record: foo\n expr: rate(foo[5m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[5m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: checkErrorBadData("prom", uri, "failed to query Prometheus config: bad_data: bad input data"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "rate(foo[5m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`prometheus "prom" at %s failed with: failed to query Prometheus config: bad_data: unhandled path`, srv.URL), - Severity: checks.Bug, + conds: []requestCondition{requireConfigPath}, + resp: respondWithBadData(), }, }, }, { description: "invalid YAML", content: "- record: foo\n expr: rate(foo[5m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/badYaml/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[5m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: checkErrorUnableToRun(checks.RateCheckName, "prom", uri, + fmt.Sprintf("failed to decode config data in %s response: yaml: line 2: could not find expected ':'", uri)), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "rate(foo[5m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf("cound't run \"promql/rate\" checks due to prometheus \"prom\" at %s/badYaml/ connection error: failed to decode config data in %s/badYaml/ response: yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `invalid...` into promapi.PrometheusConfig", srv.URL, srv.URL), - Severity: checks.Bug, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:::\nglobal:{}{}{}\n"}, }, }, }, { description: "connection refused", content: "- record: foo\n expr: rate(foo[5m])\n", - checker: checks.NewRateCheck(simpleProm("prom", "http://", time.Second, false)), - problems: []checks.Problem{ - { - Fragment: "rate(foo[5m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: `cound't run "promql/rate" checks due to prometheus "prom" at http:// connection error: failed to query Prometheus config: Get "http:///api/v1/status/config": http: no Host in request URL`, - Severity: checks.Warning, - }, + checker: func(_ string) checks.RuleChecker { + return checks.NewRateCheck(simpleProm("prom", "http://127.0.0.1:1111", time.Second, true)) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "rate(foo[5m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: checkErrorUnableToRun(checks.RateCheckName, "prom", "http://127.0.0.1:1111", `failed to query Prometheus config: Get "http://127.0.0.1:1111/api/v1/status/config": dial tcp 127.0.0.1:1111: connect: connection refused`), + Severity: checks.Bug, + }, + } }, }, { description: "irate == 3 x default 1m", content: "- record: foo\n expr: irate(foo[3m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/default/", time.Second, true)), + checker: newRateCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global: {}\n"}, + }, + }, }, { description: "irate < 3 x default 1m", content: "- record: foo\n expr: irate(foo[2m])\n", - checker: checks.NewRateCheck(simpleProm("prom", srv.URL+"/default/", time.Second, true)), - problems: []checks.Problem{ + checker: newRateCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "irate(foo[2m])", + Lines: []int{2}, + Reporter: "promql/rate", + Text: durationRecommenedText("prom", uri, "irate", "3", "1m"), + Severity: checks.Warning, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "irate(foo[2m])", - Lines: []int{2}, - Reporter: "promql/rate", - Text: fmt.Sprintf(`duration for irate() is recommended to be at least 3 x scrape_interval, prometheus "prom" at %s is using 1m scrape_interval`, srv.URL+"/default/"), - Severity: checks.Warning, + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global: {}\n"}, }, }, }, diff --git a/internal/checks/promql_regexp_test.go b/internal/checks/promql_regexp_test.go index cca7dd1a..19f992c9 100644 --- a/internal/checks/promql_regexp_test.go +++ b/internal/checks/promql_regexp_test.go @@ -6,73 +6,88 @@ import ( "github.com/cloudflare/pint/internal/checks" ) +func newRegexpCheck(_ string) checks.RuleChecker { + return checks.NewRegexpCheck() +} + func TestRegexpCheck(t *testing.T) { testCases := []checkTest{ { description: "ignores rules with syntax errors", content: "- alert: foo\n expr: sum(foo) without(\n", - checker: checks.NewRegexpCheck(), + checker: newRegexpCheck, + problems: noProblems, }, { description: "static match", content: "- record: foo\n expr: foo{job=\"bar\"}\n", - checker: checks.NewRegexpCheck(), + checker: newRegexpCheck, + problems: noProblems, }, { description: "valid regexp", content: "- record: foo\n expr: foo{job=~\"bar.+\"}\n", - checker: checks.NewRegexpCheck(), + checker: newRegexpCheck, + problems: noProblems, }, { description: "valid regexp", content: "- record: foo\n expr: foo{job!~\"(.*)\"}\n", - checker: checks.NewRegexpCheck(), + checker: newRegexpCheck, + problems: noProblems, }, { description: "valid regexp", content: "- record: foo\n expr: foo{job=~\"a|b|c\"}\n", - checker: checks.NewRegexpCheck(), + checker: newRegexpCheck, + problems: noProblems, }, { description: "unnecessary regexp", content: "- record: foo\n expr: foo{job=~\"bar\"}\n", - checker: checks.NewRegexpCheck(), - problems: []checks.Problem{ - { - Fragment: `foo{job=~"bar"}`, - Lines: []int{2}, - Reporter: checks.RegexpCheckName, - Text: `unnecessary regexp match on static string job=~"bar", use job="bar" instead`, - Severity: checks.Bug, - }, + checker: newRegexpCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `foo{job=~"bar"}`, + Lines: []int{2}, + Reporter: checks.RegexpCheckName, + Text: `unnecessary regexp match on static string job=~"bar", use job="bar" instead`, + Severity: checks.Bug, + }, + } }, }, { description: "unnecessary negative regexp", content: "- record: foo\n expr: foo{job!~\"bar\"}\n", - checker: checks.NewRegexpCheck(), - problems: []checks.Problem{ - { - Fragment: `foo{job!~"bar"}`, - Lines: []int{2}, - Reporter: checks.RegexpCheckName, - Text: `unnecessary regexp match on static string job!~"bar", use job!="bar" instead`, - Severity: checks.Bug, - }, + checker: newRegexpCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `foo{job!~"bar"}`, + Lines: []int{2}, + Reporter: checks.RegexpCheckName, + Text: `unnecessary regexp match on static string job!~"bar", use job!="bar" instead`, + Severity: checks.Bug, + }, + } }, }, { description: "empty regexp", content: "- record: foo\n expr: foo{job=~\"\"}\n", - checker: checks.NewRegexpCheck(), - problems: []checks.Problem{ - { - Fragment: `foo{job=~""}`, - Lines: []int{2}, - Reporter: checks.RegexpCheckName, - Text: `unnecessary regexp match on static string job=~"", use job="" instead`, - Severity: checks.Bug, - }, + checker: newRegexpCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `foo{job=~""}`, + Lines: []int{2}, + Reporter: checks.RegexpCheckName, + Text: `unnecessary regexp match on static string job=~"", use job="" instead`, + Severity: checks.Bug, + }, + } }, }, } diff --git a/internal/checks/promql_series.go b/internal/checks/promql_series.go index bf4c9368..9b3ab7f3 100644 --- a/internal/checks/promql_series.go +++ b/internal/checks/promql_series.go @@ -183,8 +183,8 @@ func (c SeriesCheck) Check(ctx context.Context, rule parser.Rule) (problems []Pr // 5. If foo is ALWAYS/SOMETIMES there BUT {bar OR baz} value is NEVER there -> BUG if len(trsLabel.ranges) == 0 { text := fmt.Sprintf( - "%s has %q metric but there are no series matching {%s} in the last %s", - promText(c.prom.Name(), trsLabel.uri), bareSelector.String(), lm.String(), trsLabel.sinceDesc(trs.from)) + "%s has %q metric with %q label but there are no series matching {%s} in the last %s", + promText(c.prom.Name(), trsLabel.uri), bareSelector.String(), lm.Name, lm.String(), trsLabel.sinceDesc(trs.from)) var s Severity = Bug for _, name := range highChurnLabels { if lm.Name == name { diff --git a/internal/checks/promql_series_test.go b/internal/checks/promql_series_test.go index 4eb1a03a..42e424d1 100644 --- a/internal/checks/promql_series_test.go +++ b/internal/checks/promql_series_test.go @@ -18,8 +18,8 @@ func noMetricText(name, uri, metric, since string) string { return fmt.Sprintf(`prometheus %q at %s didn't have any series for %q metric in the last %s`, name, uri, metric, since) } -func noFilterMatchText(name, uri, metric, filter, since string) string { - return fmt.Sprintf(`prometheus %q at %s has %q metric but there are no series matching %s in the last %s`, name, uri, metric, filter, since) +func noFilterMatchText(name, uri, metric, label, filter, since string) string { + return fmt.Sprintf(`prometheus %q at %s has %q metric with %q label but there are no series matching %s in the last %s`, name, uri, metric, label, filter, since) } func noLabelKeyText(name, uri, metric, label, since string) string { @@ -47,7 +47,7 @@ func seriesSometimesText(name, uri, metric, since, avg string) string { } func TestSeriesCheck(t *testing.T) { - testCases := []checkTestT{ + testCases := []checkTest{ { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", @@ -69,10 +69,10 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{requireQueryPath}, - resp: respondWithBadData, + resp: respondWithBadData(), }, }, }, @@ -109,14 +109,14 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{requireQueryPath}, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{requireRangeQueryPath}, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -135,24 +135,24 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: "count(notfound)"}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: "count(notfound)"}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, { conds: []requestCondition{requireQueryPath, formCond{key: "query", value: "count(found_7)"}}, - resp: respondWithSingleInstantVector, + resp: respondWithSingleInstantVector(), }, }, }, @@ -182,20 +182,20 @@ func TestSeriesCheck(t *testing.T) { `, checker: newSeriesCheck, problems: noProblems, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(disk_info{interface_speed!="6.0 Gb/s",type="sat"})`}, }, - resp: respondWithSingleInstantVector, + resp: respondWithSingleInstantVector(), }, { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(node_filesystem_readonly{mountpoint!=""})`}, }, - resp: respondWithSingleInstantVector, + resp: respondWithSingleInstantVector(), }, }, }, @@ -204,13 +204,13 @@ func TestSeriesCheck(t *testing.T) { content: "- record: foo\n expr: node_filesystem_readonly{mountpoint!=\"\"} offset 5m\n", checker: newSeriesCheck, problems: noProblems, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(node_filesystem_readonly{mountpoint!=""})`}, }, - resp: respondWithSingleInstantVector, + resp: respondWithSingleInstantVector(), }, }, }, @@ -219,13 +219,13 @@ func TestSeriesCheck(t *testing.T) { content: "- record: foo\n expr: node_filesystem_readonly{mountpoint!=\"\"} offset -15m\n", checker: newSeriesCheck, problems: noProblems, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(node_filesystem_readonly{mountpoint!=""})`}, }, - resp: respondWithSingleInstantVector, + resp: respondWithSingleInstantVector(), }, }, }, @@ -234,10 +234,10 @@ func TestSeriesCheck(t *testing.T) { content: "- record: foo\n expr: found > 0\n", checker: newSeriesCheck, problems: noProblems, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{requireQueryPath}, - resp: respondWithSingleInstantVector, + resp: respondWithSingleInstantVector(), }, }, }, @@ -256,10 +256,10 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{requireQueryPath}, - resp: respondWithInternalError, + resp: respondWithInternalError(), }, }, }, @@ -278,14 +278,14 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{requireQueryPath}, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{requireRangeQueryPath}, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -304,14 +304,14 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{requireQueryPath}, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{requireRangeQueryPath}, - resp: respondWithInternalError, + resp: respondWithInternalError(), }, }, }, @@ -330,20 +330,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{job="foo",notfound="xxx"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(found)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ @@ -366,7 +366,7 @@ func TestSeriesCheck(t *testing.T) { requireRangeQueryPath, formCond{key: "query", value: `count(found) by (notfound)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, }, }, @@ -385,27 +385,27 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{notfound="xxx"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(found)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(found) by (notfound)`}, }, - resp: respondWithInternalError, + resp: respondWithInternalError(), }, }, }, @@ -424,13 +424,13 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{instance="bar",job="foo"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ @@ -492,25 +492,25 @@ func TestSeriesCheck(t *testing.T) { Fragment: `found{instance!~"bad",instance=~".+",not!="negative",notfound="notfound"}`, Lines: []int{2}, Reporter: checks.SeriesCheckName, - Text: noFilterMatchText("prom", uri, "found", `{notfound="notfound"}`, "1w"), + Text: noFilterMatchText("prom", uri, "found", "notfound", `{notfound="notfound"}`, "1w"), Severity: checks.Bug, }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{instance!~"bad",instance=~".+",not!="negative",notfound="notfound"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(found)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ @@ -581,7 +581,7 @@ func TestSeriesCheck(t *testing.T) { requireRangeQueryPath, formCond{key: "query", value: `count(found{notfound="notfound"})`}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -600,20 +600,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{error="xxx"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(found)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ @@ -636,7 +636,7 @@ func TestSeriesCheck(t *testing.T) { requireRangeQueryPath, formCond{key: "query", value: `count(found{error="xxx"})`}, }, - resp: respondWithInternalError, + resp: respondWithInternalError(), }, }, }, @@ -650,18 +650,18 @@ func TestSeriesCheck(t *testing.T) { Fragment: `sometimes{churn="notfound"}`, Lines: []int{2}, Reporter: checks.SeriesCheckName, - Text: noFilterMatchText("prom", uri, "sometimes", `{churn="notfound"}`, "1w") + `, "churn" looks like a high churn label`, + Text: noFilterMatchText("prom", uri, "sometimes", "churn", `{churn="notfound"}`, "1w") + `, "churn" looks like a high churn label`, Severity: checks.Warning, }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(sometimes{churn="notfound"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ @@ -724,7 +724,7 @@ func TestSeriesCheck(t *testing.T) { requireRangeQueryPath, formCond{key: "query", value: `count(sometimes{churn="notfound"})`}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -743,20 +743,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count({__name__="found",removed="xxx"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count({__name__="found"})`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ @@ -807,20 +807,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{sometimes="xxx"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(found)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ @@ -907,13 +907,13 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(sometimes{foo!="bar"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ @@ -983,25 +983,25 @@ func TestSeriesCheck(t *testing.T) { Fragment: `found{job="notfound"}`, Lines: []int{2}, Reporter: checks.SeriesCheckName, - Text: noFilterMatchText("prom", uri, "found", `{job="notfound"}`, "1w"), + Text: noFilterMatchText("prom", uri, "found", "job", `{job="notfound"}`, "1w"), Severity: checks.Bug, }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(found{job="notfound"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: "count(found)"}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ @@ -1024,7 +1024,7 @@ func TestSeriesCheck(t *testing.T) { requireRangeQueryPath, formCond{key: "query", value: `count(found{job="notfound"})`}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -1043,20 +1043,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(notfound{job="notfound"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: "count(notfound)"}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -1078,20 +1078,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count({__name__="notfound",job="bar"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count({__name__="notfound"})`}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -1144,20 +1144,20 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(notfound)`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(notfound)`}, }, - resp: respondWithEmptyMatrix, + resp: respondWithEmptyMatrix(), }, }, }, @@ -1176,30 +1176,30 @@ func TestSeriesCheck(t *testing.T) { }, } }, - mocks: []prometheusMock{ + mocks: []*prometheusMock{ { conds: []requestCondition{ requireQueryPath, formCond{key: "query", value: `count(ALERTS{notfound="foo"})`}, }, - resp: respondWithEmptyVector, + resp: respondWithEmptyVector(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(ALERTS)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, { conds: []requestCondition{ requireRangeQueryPath, formCond{key: "query", value: `count(ALERTS) by (notfound)`}, }, - resp: respondWithSingleRangeVector1W, + resp: respondWithSingleRangeVector1W(), }, }, }, } - runTestsT(t, testCases) + runTests(t, testCases) } diff --git a/internal/checks/promql_syntax_test.go b/internal/checks/promql_syntax_test.go index 466498e3..cd228227 100644 --- a/internal/checks/promql_syntax_test.go +++ b/internal/checks/promql_syntax_test.go @@ -6,44 +6,54 @@ import ( "github.com/cloudflare/pint/internal/checks" ) +func newSyntaxCheck(_ string) checks.RuleChecker { + return checks.NewSyntaxCheck() +} + func TestSyntaxCheck(t *testing.T) { testCases := []checkTest{ { description: "valid recording rule", content: "- record: foo\n expr: sum(foo)\n", - checker: checks.NewSyntaxCheck(), + checker: newSyntaxCheck, + problems: noProblems, }, { description: "valid alerting rule", content: "- alert: foo\n expr: sum(foo)\n", - checker: checks.NewSyntaxCheck(), + checker: newSyntaxCheck, + problems: noProblems, }, { description: "no arguments for aggregate expression provided", content: "- record: foo\n expr: sum(\n", - checker: checks.NewSyntaxCheck(), - problems: []checks.Problem{ - { - Fragment: "sum(", - Lines: []int{2}, - Reporter: "promql/syntax", - Text: "syntax error: no arguments for aggregate expression provided", - Severity: checks.Fatal, - }, + checker: newSyntaxCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(", + Lines: []int{2}, + Reporter: "promql/syntax", + Text: "syntax error: no arguments for aggregate expression provided", + Severity: checks.Fatal, + }, + } }, }, { description: "unclosed left parenthesis", content: "- record: foo\n expr: sum(foo) by(", - checker: checks.NewSyntaxCheck(), - problems: []checks.Problem{ - { - Fragment: "sum(foo) by(", - Lines: []int{2}, - Reporter: "promql/syntax", - Text: "syntax error: unclosed left parenthesis", - Severity: checks.Fatal, - }, + checker: newSyntaxCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo) by(", + Lines: []int{2}, + Reporter: "promql/syntax", + Text: "syntax error: unclosed left parenthesis", + Severity: checks.Fatal, + }, + } }, }, } diff --git a/internal/checks/promql_vector_matching_test.go b/internal/checks/promql_vector_matching_test.go index f890bfd4..2fedae71 100644 --- a/internal/checks/promql_vector_matching_test.go +++ b/internal/checks/promql_vector_matching_test.go @@ -1,307 +1,641 @@ package checks_test import ( - "net/http" - "net/http/httptest" + "fmt" "testing" "time" + "github.com/prometheus/common/model" + "github.com/cloudflare/pint/internal/checks" ) -func TestVectorMatchingCheck(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - err := r.ParseForm() - if err != nil { - t.Fatal(err) - } - query := r.Form.Get("query") +func newVectorMatchingCheck(uri string) checks.RuleChecker { + return checks.NewVectorMatchingCheck(simpleProm("prom", uri, time.Second, true)) +} - switch query { - case "count(foo / ignoring(notfound) foo)", - "count(foo_with_notfound / ignoring(notfound) foo_with_notfound)", - `count({__name__="foo_with_notfound"} / ignoring(notfound) foo_with_notfound)`, - "count(foo_with_notfound / ignoring(notfound) foo)", - "count(foo / ignoring(notfound) foo_with_notfound)", - "count(foo / bar)", - "count(up and foo)", - "count(memory_bytes / ignoring(job) memory_limit)": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{},"value":[1614859502.068,"1"]}] - } - }`)) - case "count(foo_with_notfound / bar)", - "count(xxx / foo)", "count(foo / xxx)", - "count(missing / foo)", - "count(foo / missing)", - "count(foo / ignoring(xxx) app_registry)", - "count(foo / on(notfound) bar)", - "count((memory_bytes / ignoring(job) memory_limit) * on(app_name) group_left(a, b, c) app_registry)", - "count(foo / on(notfound) bar_with_notfound)", - "count(foo_with_notfound / on(notfound) bar)", - "count(foo_with_notfound / ignoring(notfound) bar)", - "count(min_over_time(foo_with_notfound[30m:1m]) / bar)", - "count((foo / ignoring(notfound) foo_with_notfound) / (foo / ignoring(notfound) foo_with_notfound))", - "count((foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit))": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[] - } - }`)) - case "topk(1, foo)", "topk(1, (foo / ignoring(notfound) foo_with_notfound))": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"__name__": "foo", "instance": "instance1", "job": "foo_job"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, bar)": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"__name__": "bar", "instance": "instance1", "job": "bar_job"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, foo_with_notfound)", "topk(1, min_over_time(foo_with_notfound[30m:1m]))": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"__name__": "foo", "instance": "instance1", "job": "foo_job", "notfound": "xxx"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, bar_with_notfound)": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"__name__": "bar", "instance": "instance1", "job": "bar_job", "notfound": "xxx"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, memory_bytes)", "topk(1, (memory_bytes / ignoring(job) memory_limit))": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"__name__": "memory_bytes", "instance": "instance1", "job": "foo_job", "dev": "mem"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, memory_limit)": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"instance": "instance1", "job": "bar_job", "dev": "mem"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, app_registry)": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{"__name__": "app_registry", "app_name": "foo"},"value":[1614859502.068,"1"]}] - } - }`)) - case "topk(1, missing)", "topk(1, xxx)": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[] - } - }`)) - case "topk(1, vector(0))": - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{},"value":[1614859502.068,"0"]}] - } - }`)) - default: - w.WriteHeader(400) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"error", - "errorType":"bad_data", - "error":"unhandled query" - }`)) - t.Fatalf("Unhandled query: %s", query) - } - })) - defer srv.Close() +func differentLabelsText(l, r string) string { + return fmt.Sprintf(`both sides of the query have different labels: [%s] != [%s]`, l, r) +} + +func usingMismatchText(f, l, r string) string { + return fmt.Sprintf(`using %s won't produce any results because both sides of the query have different labels: [%s] != [%s]`, f, l, r) +} +func TestVectorMatchingCheck(t *testing.T) { testCases := []checkTest{ { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, }, { description: "one to one matching", content: "- record: foo\n expr: foo_with_notfound / bar\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "foo_with_notfound / bar", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: differentLabelsText("instance job notfound", "instance job"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo_with_notfound / bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + "notfound": "xxx", + }), + }, + }, + }, { - Fragment: "foo_with_notfound / bar", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `both sides of the query have different labels: [instance job notfound] != [instance job]`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + "instance": "aaa", + "job": "bbb", + }), + }, + }, }, }, }, { - description: "ignore left query errors", + description: "ignore missing left side", content: "- record: foo\n expr: xxx / foo\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(xxx / foo)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, xxx)"}, + }, + resp: respondWithEmptyVector(), + }, + }, }, { - description: "ignore righ query errors", + description: "ignore missing right side", content: "- record: foo\n expr: foo / xxx\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - }, - { - description: "ignore missing left series", - content: "- record: foo\n expr: missing / foo\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / xxx)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, xxx)"}, + }, + resp: respondWithEmptyVector(), + }, + }, }, { description: "ignore missing or vector", content: "- record: foo\n expr: sum(missing or vector(0))\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, }, { description: "ignore present or vector", content: "- record: foo\n expr: sum(foo or vector(0))\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - }, - { - description: "ignore missing right series", - content: "- record: foo\n expr: foo / missing\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, }, { description: "ignore with mismatched series", content: "- record: foo\n expr: foo / ignoring(xxx) app_registry\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "foo / ignoring(xxx) app_registry", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: usingMismatchText(`ignoring("xxx")`, "instance job", "app_name"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "foo / ignoring(xxx) app_registry", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `using ignoring("xxx") won't produce any results because both sides of the query have different labels: [instance job] != [app_name]`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / ignoring(xxx) app_registry)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, app_registry)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "app_registry", + "app_name": "aaa", + }), + }, + }, }, }, }, { description: "one to one matching with on() - both missing", content: "- record: foo\n expr: foo / on(notfound) bar\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "foo / on(notfound) bar", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: `using on("notfound") won't produce any results because both sides of the query don't have this label`, + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "foo / on(notfound) bar", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `using on("notfound") won't produce any results because both sides of the query don't have this label`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / on(notfound) bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + "instance": "aaa", + "job": "bbb", + }), + }, + }, }, }, }, { description: "one to one matching with ignoring() - both missing", content: "- record: foo\n expr: foo / ignoring(notfound) foo\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / ignoring(notfound) foo)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + }, }, { description: "one to one matching with ignoring() - both present", content: "- record: foo\n expr: foo_with_notfound / ignoring(notfound) foo_with_notfound\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo_with_notfound / ignoring(notfound) foo_with_notfound)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + }, }, { description: "one to one matching with ignoring() - left missing", content: "- record: foo\n expr: foo / ignoring(notfound) foo_with_notfound\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / ignoring(notfound) foo_with_notfound)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + }, }, { description: "one to one matching with ignoring() - right missing", content: "- record: foo\n expr: foo_with_notfound / ignoring(notfound) foo\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo_with_notfound / ignoring(notfound) foo)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + }, }, { description: "one to one matching with ignoring() - mismatch", content: "- record: foo\n expr: foo_with_notfound / ignoring(notfound) bar\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo_with_notfound / ignoring(notfound) bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + }, }, { description: "one to one matching with on() - left missing", content: "- record: foo\n expr: foo / on(notfound) bar_with_notfound\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "foo / on(notfound) bar_with_notfound", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: `using on("notfound") won't produce any results because left hand side of the query doesn't have this label: "foo"`, + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / on(notfound) bar_with_notfound)"}, + }, + resp: respondWithEmptyVector(), + }, { - Fragment: "foo / on(notfound) bar_with_notfound", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `using on("notfound") won't produce any results because left hand side of the query doesn't have this label: "foo"`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, }, }, }, { description: "one to one matching with on() - right missing", content: "- record: foo\n expr: foo_with_notfound / on(notfound) bar\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "foo_with_notfound / on(notfound) bar", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: `using on("notfound") won't produce any results because right hand side of the query doesn't have this label: "bar"`, + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo_with_notfound / on(notfound) bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, { - Fragment: "foo_with_notfound / on(notfound) bar", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `using on("notfound") won't produce any results because right hand side of the query doesn't have this label: "bar"`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + "instance": "aaa", + "job": "bbb", + }), + }, + }, }, }, }, { description: "nested query", content: "- alert: foo\n expr: (memory_bytes / ignoring(job) (memory_limit > 0)) * on(app_name) group_left(a,b,c) app_registry\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "(memory_bytes / ignoring(job) (memory_limit > 0)) * on(app_name) group_left(a,b,c) app_registry", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: `using on("app_name") won't produce any results because left hand side of the query doesn't have this label: "(memory_bytes / ignoring(job) (memory_limit > 0))"`, + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count((memory_bytes / ignoring(job) memory_limit) * on(app_name) group_left(a, b, c) app_registry)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, (memory_bytes / ignoring(job) memory_limit))"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "memory_bytes", + "instance": "instance1", + "job": "foo_job", + "dev": "mem", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, app_registry)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "app_registry", + "app_name": "foo", + }), + }, + }, + }, { - Fragment: "(memory_bytes / ignoring(job) (memory_limit > 0)) * on(app_name) group_left(a,b,c) app_registry", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `using on("app_name") won't produce any results because left hand side of the query doesn't have this label: "(memory_bytes / ignoring(job) (memory_limit > 0))"`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(memory_bytes / ignoring(job) memory_limit)"}, + }, + resp: respondWithSingleInstantVector(), }, }, }, @@ -311,92 +645,436 @@ func TestVectorMatchingCheck(t *testing.T) { - record: foo expr: '{__name__="foo_with_notfound"} / ignoring(notfound) foo_with_notfound' `, - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, }, { description: "skips number comparison on LHS", content: "- record: foo\n expr: 2 < foo / bar\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + }), + }, + }, + }, + }, }, { description: "skips number comparison on RHS", content: "- record: foo\n expr: foo / bar > 0\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + }), + }, + }, + }, + }, }, { description: "skips number comparison on both sides", content: "- record: foo\n expr: 1 > bool 1\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, }, { description: "up == 0 AND foo > 0", content: "- alert: foo\n expr: up == 0 AND foo > 0\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(up and foo)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, up)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "up", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + }), + }, + }, + }, + }, }, { description: "subquery is trimmed", content: "- alert: foo\n expr: min_over_time((foo_with_notfound > 0)[30m:1m]) / bar\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "min_over_time((foo_with_notfound > 0)[30m:1m]) / bar", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: `both sides of the query have different labels: [instance job notfound] != [instance job]`, + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "min_over_time((foo_with_notfound > 0)[30m:1m]) / bar", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `both sides of the query have different labels: [instance job notfound] != [instance job]`, - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(min_over_time(foo_with_notfound[30m:1m]) / bar)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, min_over_time(foo_with_notfound[30m:1m]))"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, bar)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "bar", + "instance": "aaa", + "job": "bbb", + }), + }, + }, }, }, }, { description: "scalar", content: "- alert: foo\n expr: (100*(1024^2))\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, }, { description: "binary expression on both sides / passing", content: "- alert: foo\n expr: (foo / ignoring(notfound) foo_with_notfound) / (foo / ignoring(notfound) foo_with_notfound)\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), + checker: newVectorMatchingCheck, + problems: noProblems, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count((foo / ignoring(notfound) foo_with_notfound) / (foo / ignoring(notfound) foo_with_notfound))"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, (foo / ignoring(notfound) foo_with_notfound))"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / ignoring(notfound) foo_with_notfound)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + }, }, { description: "binary expression on both sides / mismatch", content: "- alert: foo\n expr: (foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit)\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", srv.URL, time.Second, true)), - problems: []checks.Problem{ + checker: newVectorMatchingCheck, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "(foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit)", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: "both sides of the query have different labels: [instance job] != [dev instance job]", + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count((foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit))"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, (foo / ignoring(notfound) foo_with_notfound))"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, { - Fragment: "(foo / ignoring(notfound) foo_with_notfound) / (memory_bytes / ignoring(job) memory_limit)", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: "both sides of the query have different labels: [instance job] != [dev instance job]", - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo", + "instance": "aaa", + "job": "bbb", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, foo_with_notfound)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "foo_with_notfound", + "instance": "aaa", + "job": "bbb", + "notfound": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, (memory_bytes / ignoring(job) memory_limit))"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "memory_bytes", + "instance": "aaa", + "job": "bbb", + "dev": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(foo / ignoring(notfound) foo_with_notfound)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "count(memory_bytes / ignoring(job) memory_limit)"}, + }, + resp: respondWithEmptyVector(), + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, memory_bytes)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "memory_bytes", + "instance": "aaa", + "job": "bbb", + "dev": "ccc", + }), + }, + }, + }, + { + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: "topk(1, memory_limit)"}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{ + "__name__": "memory_limit", + "instance": "aaa", + "job": "xxx", + "dev": "ccc", + }), + }, + }, }, }, }, { - description: "connection refused", + description: "connection refused / required", content: "- record: foo\n expr: xxx/yyy\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", "http://127.0.0.1", time.Second, true)), - problems: []checks.Problem{ - { - Fragment: "xxx/yyy", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `cound't run "promql/vector_matching" checks due to prometheus "prom" at http://127.0.0.1 connection error: Post "http://127.0.0.1/api/v1/query": dial tcp 127.0.0.1:80: connect: connection refused`, - Severity: checks.Bug, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewVectorMatchingCheck(simpleProm("prom", "http://127.0.0.1:1111", time.Second, true)) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "xxx/yyy", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: checkErrorUnableToRun(checks.VectorMatchingCheckName, "prom", "http://127.0.0.1:1111", `Post "http://127.0.0.1:1111/api/v1/query": dial tcp 127.0.0.1:1111: connect: connection refused`), + Severity: checks.Bug, + }, + } }, }, { - description: "connection refused", + description: "connection refused / not required", content: "- record: foo\n expr: xxx/yyy\n", - checker: checks.NewVectorMatchingCheck(simpleProm("prom", "http://127.0.0.1", time.Second, false)), - problems: []checks.Problem{ - { - Fragment: "xxx/yyy", - Lines: []int{2}, - Reporter: "promql/vector_matching", - Text: `cound't run "promql/vector_matching" checks due to prometheus "prom" at http://127.0.0.1 connection error: Post "http://127.0.0.1/api/v1/query": dial tcp 127.0.0.1:80: connect: connection refused`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewVectorMatchingCheck(simpleProm("prom", "http://127.0.0.1:1111", time.Second, false)) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "xxx/yyy", + Lines: []int{2}, + Reporter: checks.VectorMatchingCheckName, + Text: checkErrorUnableToRun(checks.VectorMatchingCheckName, "prom", "http://127.0.0.1:1111", `Post "http://127.0.0.1:1111/api/v1/query": dial tcp 127.0.0.1:1111: connect: connection refused`), + Severity: checks.Warning, + }, + } }, }, } diff --git a/internal/checks/query_cost.go b/internal/checks/query_cost.go index f9ff845a..9ef5f49b 100644 --- a/internal/checks/query_cost.go +++ b/internal/checks/query_cost.go @@ -82,7 +82,7 @@ func (c CostCheck) Check(ctx context.Context, rule parser.Rule) (problems []Prob Fragment: expr.Value.Value, Lines: expr.Lines(), Reporter: c.Reporter(), - Text: fmt.Sprintf("%s completed in %.2fs returning %d result(s)%s%s", promText(c.prom.Name(), qr.URI), qr.DurationSeconds, series, estimate, above), + Text: fmt.Sprintf("%s returned %d result(s)%s%s", promText(c.prom.Name(), qr.URI), series, estimate, above), Severity: severity, }) return diff --git a/internal/checks/query_cost_test.go b/internal/checks/query_cost_test.go index bcc7d652..28c9d009 100644 --- a/internal/checks/query_cost_test.go +++ b/internal/checks/query_cost_test.go @@ -2,220 +2,346 @@ package checks_test import ( "fmt" - "net/http" - "net/http/httptest" - "regexp" - "strings" "testing" "time" - "github.com/cloudflare/pint/internal/checks" + "github.com/prometheus/common/model" - "github.com/google/go-cmp/cmp" + "github.com/cloudflare/pint/internal/checks" ) -func TestCostCheck(t *testing.T) { - content := "- record: foo\n expr: sum(foo)\n" +func costText(name, uri string, count int) string { + return fmt.Sprintf(`prometheus %q at %s returned %d result(s)`, name, uri, count) +} - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - err := r.ParseForm() - if err != nil { - t.Fatal(err) - } - query := r.Form.Get("query") - if query != "count(sum(foo))" && query != `count(sum({__name__="foo"}))` { - t.Fatalf("Prometheus got invalid query: %s", query) - } +func memUsageText(b string) string { + return fmt.Sprintf(" with %s estimated memory usage", b) +} - switch r.URL.Path { - case "/empty/api/v1/query": - time.Sleep(time.Second * 2) - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[] - } - }`)) - case "/1/api/v1/query": - time.Sleep(time.Millisecond * 550) - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{},"value":[1614859502.068,"1"]}] - } - }`)) - case "/7/api/v1/query": - time.Sleep(time.Millisecond * 100) - w.WriteHeader(200) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"success", - "data":{ - "resultType":"vector", - "result":[{"metric":{},"value":[1614859502.068,"7"]}] - } - }`)) - default: - w.WriteHeader(400) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "status":"error", - "errorType":"bad_data", - "error":"unhandled path" - }`)) - } - })) - defer srv.Close() +func maxSeriesText(m int) string { + return fmt.Sprintf(", maximum allowed series is %d", m) +} + +func TestCostCheck(t *testing.T) { + content := "- record: foo\n expr: sum(foo)\n" testCases := []checkTest{ { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewCostCheck(simpleProm("prom", "http://localhost", time.Second*5, true), 4096, 0, checks.Bug), + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 4096, 0, checks.Bug) + }, + problems: noProblems, }, { description: "empty response", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/empty/", time.Second*5, true), 4096, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 4096, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 0), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 2\...s returning 0 result\(s\)`, srv.URL+"/empty/"), - Severity: checks.Information, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: respondWithEmptyVector(), }, }, }, { description: "response timeout", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/empty/", time.Millisecond*5, true), 4096, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Millisecond*50, true), 4096, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: checkErrorUnableToRun(checks.CostCheckName, "prom", uri, + fmt.Sprintf(`Post "%s/api/v1/query": context deadline exceeded`, uri)), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:cound't run "query/cost" checks due to prometheus "prom" at %s/empty/ connection error: Post "http://127.0.0.1:.+/empty/api/v1/query": context deadline exceeded`, srv.URL), - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: sleepResponse{sleep: time.Millisecond * 100}, }, }, }, { description: "bad request", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/400/", time.Second*5, true), 4096, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 4096, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: checkErrorBadData("prom", uri, "bad_data: bad input data"), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`prometheus "prom" at %s/400/ failed with: bad_data: unhandled path`, srv.URL), - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: respondWithBadData(), }, }, }, { - description: "bad request", + description: "connection refused", content: content, - checker: checks.NewCostCheck(simpleProm("prom", "http://127.0.0.1", time.Second*5, false), 4096, 0, checks.Bug), - problems: []checks.Problem{ - { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: `cound't run "query/cost" checks due to prometheus "prom" at http://127.0.0.1 connection error: Post "http://127.0.0.1/api/v1/query": dial tcp 127.0.0.1:80: connect: connection refused`, - Severity: checks.Warning, - }, + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", "http://127.0.0.1:1111", time.Second*5, false), 4096, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: checkErrorUnableToRun(checks.CostCheckName, "prom", "http://127.0.0.1:1111", `Post "http://127.0.0.1:1111/api/v1/query": dial tcp 127.0.0.1:1111: connect: connection refused`), + Severity: checks.Warning, + }, + } }, }, { description: "1 result", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/1/", time.Second*5, true), 4096, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 4096, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 1) + memUsageText("4.0KiB"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 1 result\(s\) with 4\.0KiB estimated memory usage`, srv.URL+"/1/"), - Severity: checks.Information, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: respondWithSingleInstantVector(), }, }, }, { description: "7 results", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/7/", time.Second*5, true), 101, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 101, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 7) + memUsageText("707B"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 7 result\(s\) with 707B estimated memory usage`, srv.URL+"/7/"), - Severity: checks.Information, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + }, }, }, }, { description: "7 result with MB", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/7/", time.Second*5, true), 1024*1024, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 1024*1024, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 7) + memUsageText("7.0MiB"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 7 result\(s\) with 7\.0MiB estimated memory usage`, srv.URL+"/7/"), - Severity: checks.Information, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + }, }, }, }, { description: "7 results with 1 series max (1KB bps)", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/7/", time.Second*5, true), 1024, 1, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 1024, 1, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 7) + memUsageText("7.0KiB") + maxSeriesText(1), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 7 result\(s\) with 7.0KiB estimated memory usage, maximum allowed series is 1`, srv.URL+"/7/"), - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + }, }, }, }, { - description: "7 results with 5 series max", + description: "6 results with 5 series max", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/7/", time.Second*5, true), 0, 5, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 0, 5, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 6) + maxSeriesText(5), + Severity: checks.Bug, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 7 result\(s\), maximum allowed series is 5`, srv.URL+"/7/"), - Severity: checks.Bug, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + }, }, }, }, { description: "7 results with 5 series max / infi", content: content, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/7/", time.Second*5, true), 0, 5, checks.Information), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 0, 5, checks.Information) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "sum(foo)", + Lines: []int{2}, + Reporter: "query/cost", + Text: costText("prom", uri, 7) + maxSeriesText(5), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: "sum(foo)", - Lines: []int{2}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 7 result\(s\), maximum allowed series is 5`, srv.URL+"/7/"), - Severity: checks.Information, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum(foo))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + }, }, }, }, @@ -225,26 +351,41 @@ func TestCostCheck(t *testing.T) { - record: foo expr: 'sum({__name__="foo"})' `, - checker: checks.NewCostCheck(simpleProm("prom", srv.URL+"/7/", time.Second*5, true), 101, 0, checks.Bug), - problems: []checks.Problem{ + checker: func(uri string) checks.RuleChecker { + return checks.NewCostCheck(simpleProm("prom", uri, time.Second*5, true), 101, 0, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `sum({__name__="foo"})`, + Lines: []int{3}, + Reporter: "query/cost", + Text: costText("prom", uri, 7) + memUsageText("707B"), + Severity: checks.Information, + }, + } + }, + mocks: []*prometheusMock{ { - Fragment: `sum({__name__="foo"})`, - Lines: []int{3}, - Reporter: "query/cost", - Text: fmt.Sprintf(`RE:prometheus "prom" at %s completed in 0\...s returning 7 result\(s\) with 707B estimated memory usage`, srv.URL+"/7/"), - Severity: checks.Information, + conds: []requestCondition{ + requireQueryPath, + formCond{key: "query", value: `count(sum({__name__="foo"}))`}, + }, + resp: vectorResponse{ + samples: []*model.Sample{ + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + generateSample(map[string]string{}), + }, + }, }, }, }, } - cmpText := cmp.Comparer(func(x, y string) bool { - if strings.HasPrefix(x, "RE:") { - xr := regexp.MustCompile(x[3:]) - return xr.MatchString(y) - } - return x == y - }) - - runTests(t, testCases, cmpText) + runTests(t, testCases) } diff --git a/internal/checks/rule_label_test.go b/internal/checks/rule_label_test.go index 2ada03cd..b49547b5 100644 --- a/internal/checks/rule_label_test.go +++ b/internal/checks/rule_label_test.go @@ -11,214 +11,289 @@ func TestLabelCheck(t *testing.T) { { description: "doesn't ignore rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "record: foo", - Lines: []int{1, 2}, - Reporter: "rule/label", - Text: "severity label is required", - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "record: foo", + Lines: []int{1, 2}, + Reporter: "rule/label", + Text: "severity label is required", + Severity: checks.Warning, + }, + } }, }, { description: "no labels in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "record: foo", - Lines: []int{1, 2}, - Reporter: "rule/label", - Text: "severity label is required", - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "record: foo", + Lines: []int{1, 2}, + Reporter: "rule/label", + Text: "severity label is required", + Severity: checks.Warning, + }, + } }, }, { description: "no labels in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: noProblems, }, { description: "missing label in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "labels:", - Lines: []int{3, 4}, - Reporter: "rule/label", - Text: "severity label is required", - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "labels:", + Lines: []int{3, 4}, + Reporter: "rule/label", + Text: "severity label is required", + Severity: checks.Warning, + }, + } }, }, { description: "missing label in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: noProblems, }, { description: "invalid value in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "severity: warning", - Lines: []int{4}, - Reporter: "rule/label", - Text: `severity label value must match "^critical$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "severity: warning", + Lines: []int{4}, + Reporter: "rule/label", + Text: `severity label value must match "^critical$"`, + Severity: checks.Warning, + }, + } }, }, { description: "invalid value in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "severity: warning", - Lines: []int{4}, - Reporter: "rule/label", - Text: `severity label value must match "^critical$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "severity: warning", + Lines: []int{4}, + Reporter: "rule/label", + Text: `severity label value must match "^critical$"`, + Severity: checks.Warning, + }, + } }, }, { description: "typo in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n priority: 2a\n", - checker: checks.NewLabelCheck("priority", checks.MustTemplatedRegexp("(1|2|3)"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "priority: 2a", - Lines: []int{4}, - Reporter: "rule/label", - Text: `priority label value must match "^(1|2|3)$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("priority", checks.MustTemplatedRegexp("(1|2|3)"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "priority: 2a", + Lines: []int{4}, + Reporter: "rule/label", + Text: `priority label value must match "^(1|2|3)$"`, + Severity: checks.Warning, + }, + } }, }, { description: "typo in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n priority: 2a\n", - checker: checks.NewLabelCheck("priority", checks.MustTemplatedRegexp("(1|2|3)"), false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "priority: 2a", - Lines: []int{4}, - Reporter: "rule/label", - Text: `priority label value must match "^(1|2|3)$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("priority", checks.MustTemplatedRegexp("(1|2|3)"), false, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "priority: 2a", + Lines: []int{4}, + Reporter: "rule/label", + Text: `priority label value must match "^(1|2|3)$"`, + Severity: checks.Warning, + }, + } }, }, { description: "no labels in alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "alert: foo", - Lines: []int{1, 2}, - Reporter: "rule/label", - Text: "severity label is required", - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "alert: foo", + Lines: []int{1, 2}, + Reporter: "rule/label", + Text: "severity label is required", + Severity: checks.Warning, + }, + } }, }, { description: "no labels in alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: noProblems, }, { description: "missing label in alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "labels:", - Lines: []int{3, 4}, - Reporter: "rule/label", - Text: "severity label is required", - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "labels:", + Lines: []int{3, 4}, + Reporter: "rule/label", + Text: "severity label is required", + Severity: checks.Warning, + }, + } }, }, { description: "missing label in alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + }, + problems: noProblems, }, { description: "invalid value in alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "severity: warning", - Lines: []int{4}, - Reporter: "rule/label", - Text: `severity label value must match "^critical|info$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "severity: warning", + Lines: []int{4}, + Reporter: "rule/label", + Text: `severity label value must match "^critical|info$"`, + Severity: checks.Warning, + }, + } }, }, { description: "invalid value in alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "severity: warning", - Lines: []int{4}, - Reporter: "rule/label", - Text: `severity label value must match "^critical|info$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "severity: warning", + Lines: []int{4}, + Reporter: "rule/label", + Text: `severity label value must match "^critical|info$"`, + Severity: checks.Warning, + }, + } }, }, { description: "valid recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: critical\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning) + }, + problems: noProblems, }, { description: "valid recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: critical\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning) + }, + problems: noProblems, }, { description: "valid alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: info\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning) + }, + problems: noProblems, }, { description: "valid alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: info\n", - checker: checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning) + }, + problems: noProblems, }, { description: "templated label value / passing", content: "- alert: foo\n expr: sum(foo)\n for: 5m\n labels:\n for: 'must wait 5m to fire'\n", - checker: checks.NewLabelCheck("for", checks.MustTemplatedRegexp("must wait {{$for}} to fire"), true, checks.Warning), + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("for", checks.MustTemplatedRegexp("must wait {{$for}} to fire"), true, checks.Warning) + }, + problems: noProblems, }, { description: "templated label value / not passing", content: "- alert: foo\n expr: sum(foo)\n for: 4m\n labels:\n for: 'must wait 5m to fire'\n", - checker: checks.NewLabelCheck("for", checks.MustTemplatedRegexp("must wait {{$for}} to fire"), true, checks.Warning), - problems: []checks.Problem{ - { - Fragment: "for: must wait 5m to fire", - Lines: []int{5}, - Reporter: "rule/label", - Text: `for label value must match "^must wait {{$for}} to fire$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewLabelCheck("for", checks.MustTemplatedRegexp("must wait {{$for}} to fire"), true, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "for: must wait 5m to fire", + Lines: []int{5}, + Reporter: "rule/label", + Text: `for label value must match "^must wait {{$for}} to fire$"`, + Severity: checks.Warning, + }, + } }, }, } diff --git a/internal/checks/rule_reject_test.go b/internal/checks/rule_reject_test.go index ade2922b..cc3b900d 100644 --- a/internal/checks/rule_reject_test.go +++ b/internal/checks/rule_reject_test.go @@ -12,150 +12,208 @@ func TestRejectCheck(t *testing.T) { { description: "no rules / alerting", content: "- record: foo\n expr: sum(foo)\n", - checker: checks.NewRejectCheck(true, true, nil, nil, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, nil, nil, checks.Bug) + }, + problems: noProblems, }, { description: "no rules / recording", content: "- alert: foo\n expr: sum(foo)\n", - checker: checks.NewRejectCheck(true, true, nil, nil, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, nil, nil, checks.Bug) + }, + problems: noProblems, }, { description: "allowed label / alerting", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n", - checker: checks.NewRejectCheck(true, true, nil, nil, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, nil, nil, checks.Bug) + }, + problems: noProblems, }, { description: "allowed label / recording", content: "- record: foo\n expr: sum(foo)\n labels:\n foo: bar\n", - checker: checks.NewRejectCheck(true, true, nil, nil, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, nil, nil, checks.Bug) + }, + problems: noProblems, }, { description: "allowed label / alerting", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bar\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: noProblems, }, { description: "allowed label / alerting", content: "- record: foo\n expr: sum(foo)\n labels:\n foo: bar\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: noProblems, }, { description: "rejected key / don't check labels", content: "- alert: foo\n expr: sum(foo)\n labels:\n bad: bar\n", - checker: checks.NewRejectCheck(false, true, badRe, badRe, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(false, true, badRe, badRe, checks.Bug) + }, + problems: noProblems, }, { description: "rejected key / alerting", content: "- alert: foo\n expr: sum(foo)\n labels:\n bad: bar\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), - problems: []checks.Problem{ - { - Fragment: `bad`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `label key bad is not allowed to match "^bad$"`, - Severity: checks.Bug, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `bad`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `label key bad is not allowed to match "^bad$"`, + Severity: checks.Bug, + }, + } }, }, { description: "rejected value / alerting", content: "- alert: foo\n expr: sum(foo)\n labels:\n foo: bad\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Warning), - problems: []checks.Problem{ - { - Fragment: `bad`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `label value bad is not allowed to match "^bad$"`, - Severity: checks.Warning, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Warning) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `bad`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `label value bad is not allowed to match "^bad$"`, + Severity: checks.Warning, + }, + } }, }, { description: "rejected key / recording", content: "- record: foo\n expr: sum(foo)\n labels:\n bad: bar\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), - problems: []checks.Problem{ - { - Fragment: `bad`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `label key bad is not allowed to match "^bad$"`, - Severity: checks.Bug, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `bad`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `label key bad is not allowed to match "^bad$"`, + Severity: checks.Bug, + }, + } }, }, { description: "rejected value / recording", content: "- record: foo\n expr: sum(foo)\n labels:\n foo: bad\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), - problems: []checks.Problem{ - { - Fragment: `bad`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `label value bad is not allowed to match "^bad$"`, - Severity: checks.Bug, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `bad`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `label value bad is not allowed to match "^bad$"`, + Severity: checks.Bug, + }, + } }, }, { description: "allowed annotation", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: bar\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: noProblems, }, { description: "rejected key / don't check annotations", content: "- alert: foo\n expr: sum(foo)\n annotations:\n bad: bar\n", - checker: checks.NewRejectCheck(false, false, badRe, badRe, checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(false, false, badRe, badRe, checks.Bug) + }, + problems: noProblems, }, { description: "rejected annotation key", content: "- alert: foo\n expr: sum(foo)\n annotations:\n bad: bar\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Information), - problems: []checks.Problem{ - { - Fragment: `bad`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `annotation key bad is not allowed to match "^bad$"`, - Severity: checks.Information, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Information) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `bad`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `annotation key bad is not allowed to match "^bad$"`, + Severity: checks.Information, + }, + } }, }, { description: "rejected annotation value", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: bad\n", - checker: checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug), - problems: []checks.Problem{ - { - Fragment: `bad`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `annotation value bad is not allowed to match "^bad$"`, - Severity: checks.Bug, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, badRe, badRe, checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `bad`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `annotation value bad is not allowed to match "^bad$"`, + Severity: checks.Bug, + }, + } }, }, { description: "reject templated regexp / passing", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: alert\n", - checker: checks.NewRejectCheck(true, true, nil, checks.MustTemplatedRegexp("{{ $alert }}"), checks.Bug), + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, nil, checks.MustTemplatedRegexp("{{ $alert }}"), checks.Bug) + }, + problems: noProblems, }, { description: "reject templated regexp / not passing", content: "- alert: foo\n expr: sum(foo)\n annotations:\n alert: foo\n", - checker: checks.NewRejectCheck(true, true, nil, checks.MustTemplatedRegexp("{{ $alert }}"), checks.Bug), - problems: []checks.Problem{ - { - Fragment: `foo`, - Lines: []int{4}, - Reporter: "rule/reject", - Text: `annotation value foo is not allowed to match "^{{ $alert }}$"`, - Severity: checks.Bug, - }, + checker: func(s string) checks.RuleChecker { + return checks.NewRejectCheck(true, true, nil, checks.MustTemplatedRegexp("{{ $alert }}"), checks.Bug) + }, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `foo`, + Lines: []int{4}, + Reporter: "rule/reject", + Text: `annotation value foo is not allowed to match "^{{ $alert }}$"`, + Severity: checks.Bug, + }, + } }, }, } diff --git a/internal/discovery/git_branch_test.go b/internal/discovery/git_branch_test.go index d332b53f..dd853f30 100644 --- a/internal/discovery/git_branch_test.go +++ b/internal/discovery/git_branch_test.go @@ -6,7 +6,7 @@ import ( "strconv" "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/discovery" ) @@ -158,9 +158,7 @@ D file1 return } - if diff := cmp.Diff(tc.output, output.Results()); diff != "" { - t.Errorf("git.ModifiedFiles() returned wrong output (-want +got):\n%s", diff) - } + require.Equal(t, tc.output, output.Results(), "git.ModifiedFiles() returned wrong output") }) } } diff --git a/internal/discovery/glob_test.go b/internal/discovery/glob_test.go index 2fcf6f29..8a517e4b 100644 --- a/internal/discovery/glob_test.go +++ b/internal/discovery/glob_test.go @@ -7,9 +7,9 @@ import ( "strconv" "testing" - "github.com/cloudflare/pint/internal/discovery" + "github.com/stretchr/testify/require" - "github.com/google/go-cmp/cmp" + "github.com/cloudflare/pint/internal/discovery" ) func TestGlobFileFinder(t *testing.T) { @@ -105,9 +105,7 @@ func TestGlobFileFinder(t *testing.T) { return } - if diff := cmp.Diff(tc.output, output.Results()); diff != "" { - t.Errorf("GlobFileFinder.Discover() returned wrong output (-want +got):\n%s", diff) - } + require.Equal(t, tc.output, output.Results(), "GlobFileFinder.Discover() returned wrong output") }) } } diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 70b39373..a813ed23 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -5,9 +5,9 @@ import ( "strconv" "testing" - "github.com/cloudflare/pint/internal/git" + "github.com/stretchr/testify/require" - "github.com/google/go-cmp/cmp" + "github.com/cloudflare/pint/internal/git" ) func blameLine(sha string, line int, filename, content string) string { @@ -88,9 +88,7 @@ func TestGitBlame(t *testing.T) { return } - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("git.Blame() returned wrong output (-want +got):\n%s", diff) - } + require.Equal(t, tc.output, output, "git.Blame() returned wrong output ") }) } } @@ -165,9 +163,7 @@ func TestCommitRange(t *testing.T) { return } - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("git.CommitRange() returned wrong output (-want +got):\n%s", diff) - } + require.Equal(t, tc.output, output, "git.CommitRange() returned wrong output") }) } } @@ -218,9 +214,7 @@ func TestCurrentBranch(t *testing.T) { return } - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("git.CurrentBranch() returned wrong output (-want +got):\n%s", diff) - } + require.Equal(t, tc.output, output, "git.CurrentBranch() returned wrong output") }) } } diff --git a/internal/output/ranges_test.go b/internal/output/ranges_test.go index 357217e2..8eac9c18 100644 --- a/internal/output/ranges_test.go +++ b/internal/output/ranges_test.go @@ -4,7 +4,7 @@ import ( "strconv" "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/output" ) @@ -38,10 +38,7 @@ func TestFormatLineRangeString(t *testing.T) { t.Run(strconv.Itoa(i+1), func(t *testing.T) { output := output.FormatLineRangeString(tc.lines) - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("FormatLineRangeString() returned wrong output (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.output, output, "FormatLineRangeString() returned wrong output") }) } } diff --git a/internal/parser/read_test.go b/internal/parser/read_test.go index 42bbf85a..93c6a62c 100644 --- a/internal/parser/read_test.go +++ b/internal/parser/read_test.go @@ -6,7 +6,7 @@ import ( "strings" "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/parser" ) @@ -147,10 +147,7 @@ func TestReadContent(t *testing.T) { return } - if diff := cmp.Diff(string(tc.output), string(output)); diff != "" { - t.Errorf("ReadContent() returned wrong output (-want +got):\n%s", diff) - return - } + require.Equal(t, string(tc.output), string(output), "ReadContent() returned wrong output") }) } } diff --git a/internal/parser/utils/absent_test.go b/internal/parser/utils/absent_test.go index 2e92e563..66363206 100644 --- a/internal/parser/utils/absent_test.go +++ b/internal/parser/utils/absent_test.go @@ -3,7 +3,7 @@ package utils_test import ( "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/parser/utils" @@ -66,10 +66,7 @@ func TestHasOuterAbsent(t *testing.T) { for _, a := range calls { output = append(output, a.Node.String()) } - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("HasOuterAbsent() returned wrong result (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.output, output, "HasOuterAbsent() returned wrong output") } }) } diff --git a/internal/parser/utils/aggregation_test.go b/internal/parser/utils/aggregation_test.go index add2a586..caab78c3 100644 --- a/internal/parser/utils/aggregation_test.go +++ b/internal/parser/utils/aggregation_test.go @@ -3,7 +3,7 @@ package utils_test import ( "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/parser/utils" @@ -147,10 +147,7 @@ func TestHasOuterAggregation(t *testing.T) { for _, a := range aggs { output = append(output, a.String()) } - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("HasOuterAggregation() returned wrong result (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.output, output, "HasOuterAggregation() returned wrong output") } }) } diff --git a/internal/parser/utils/binary_expr_test.go b/internal/parser/utils/binary_expr_test.go index 424acc1b..41db915a 100644 --- a/internal/parser/utils/binary_expr_test.go +++ b/internal/parser/utils/binary_expr_test.go @@ -3,7 +3,7 @@ package utils_test import ( "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/parser/utils" @@ -54,10 +54,7 @@ func TestHasOuterBinaryExpr(t *testing.T) { t.Errorf("HasOuterBinaryExpr() returned nil, expected %s", tc.output) } } else { - if diff := cmp.Diff(tc.output, bin.String()); diff != "" { - t.Errorf("HasOuterBinaryExpr() returned wrong result (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.output, bin.String(), "HasOuterBinaryExpr() returned wrong output") } }) } diff --git a/internal/parser/utils/vectorselector_test.go b/internal/parser/utils/vectorselector_test.go index 0182c059..7e8f09ca 100644 --- a/internal/parser/utils/vectorselector_test.go +++ b/internal/parser/utils/vectorselector_test.go @@ -3,7 +3,7 @@ package utils_test import ( "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/parser/utils" @@ -59,10 +59,7 @@ func TestHasVectorSelector(t *testing.T) { for _, v := range vs { output = append(output, v.String()) } - if diff := cmp.Diff(tc.output, output); diff != "" { - t.Errorf("HasVectorSelector() returned wrong result (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.output, output, "HasVectorSelector() returned wrong output") } }) } diff --git a/internal/reporter/bitbucket_test.go b/internal/reporter/bitbucket_test.go index abbe5652..3453c219 100644 --- a/internal/reporter/bitbucket_test.go +++ b/internal/reporter/bitbucket_test.go @@ -10,8 +10,8 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" "github.com/rs/zerolog" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" @@ -474,19 +474,13 @@ func TestBitBucketReporter(t *testing.T) { if err := json.NewDecoder(r.Body).Decode(&resp); err != nil { t.Errorf("JSON decode error: %v", err) } - if diff := cmp.Diff(tc.report, resp); diff != "" { - t.Errorf("Got wrong bitbucket report body (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.report, resp, "Got wrong bitbucket report body") case "/rest/insights/1.0/projects/proj/repos/repo/commits/fake-commit-id/reports/pint/annotations": var resp reporter.BitBucketAnnotations if err := json.NewDecoder(r.Body).Decode(&resp); err != nil { t.Errorf("JSON decode error: %s", err) } - if diff := cmp.Diff(tc.annotations, resp); diff != "" { - t.Errorf("Got wrong bitbucket annotations (-want +got):\n%s", diff) - return - } + require.Equal(t, tc.annotations, resp, "Got wrong bitbucket annotations") default: w.WriteHeader(500) _, _ = w.Write([]byte(fmt.Sprintf("Unhandled path: %s", r.URL.Path)))