Skip to content

Commit

Permalink
Add labels/conflict check
Browse files Browse the repository at this point in the history
  • Loading branch information
prymitive committed Nov 25, 2022
1 parent 39a7f27 commit 0c66d41
Show file tree
Hide file tree
Showing 16 changed files with 1,065 additions and 597 deletions.
8 changes: 4 additions & 4 deletions cmd/pint/tests/0023_enabled_checks.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
pint.error -l debug --no-color lint rules
! stdout .
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)"] path=rules/1.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)"] path=rules/1.yaml rule=two'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)"] path=rules/2.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)"] path=rules/2.yaml rule=two'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)","labels/conflict\(prom\)"] path=rules/1.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)","labels/conflict\(prom\)"] path=rules/1.yaml rule=two'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)","labels/conflict\(prom\)"] path=rules/2.yaml rule=one'
stderr 'level=debug msg="Configured checks for rule" enabled=\["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/rate\(prom\)","promql/series\(prom\)","promql/vector_matching\(prom\)"\,"promql/range_query\(prom\)","rule/duplicate\(prom\)","labels/conflict\(prom\)"] path=rules/2.yaml rule=two'

-- rules/1.yaml --
- record: one
Expand Down
1 change: 1 addition & 0 deletions cmd/pint/tests/0025_config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ level=info msg="Loading configuration file" path=.pint.hcl
"alerts/count",
"alerts/for",
"alerts/template",
"labels/conflict",
"promql/aggregate",
"alerts/comparison",
"promql/fragile",
Expand Down
2 changes: 2 additions & 0 deletions cmd/pint/tests/0054_watch_metrics_prometheus.txt
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ pint_check_duration_seconds_sum{check="alerts/for"}
pint_check_duration_seconds_count{check="alerts/for"}
pint_check_duration_seconds_sum{check="alerts/template"}
pint_check_duration_seconds_count{check="alerts/template"}
pint_check_duration_seconds_sum{check="labels/conflict"}
pint_check_duration_seconds_count{check="labels/conflict"}
pint_check_duration_seconds_sum{check="promql/fragile"}
pint_check_duration_seconds_count{check="promql/fragile"}
pint_check_duration_seconds_sum{check="promql/range_query"}
Expand Down
2 changes: 1 addition & 1 deletion cmd/pint/tests/0056_prometheus_required.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ pint.ok -l debug --no-color lint rules
! stdout .
stderr 'level=error msg="Query returned an error" error="server error: 500" query=count\(up\) uri=http://127.0.0.1:7056'
stderr 'level=error msg="Query returned an error" error="server error: 500" query=/api/v1/status/config uri=http://127.0.0.1:7056'
stderr 'level=info msg="Problems found" Warning=12'
stderr 'level=info msg="Problems found" Warning=[0-9]+'

-- rules/1.yaml --
- record: one
Expand Down
2 changes: 2 additions & 0 deletions cmd/pint/tests/0057_watch_metrics_prometheus_ignore.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ pint_check_duration_seconds_sum{check="alerts/for"}
pint_check_duration_seconds_count{check="alerts/for"}
pint_check_duration_seconds_sum{check="alerts/template"}
pint_check_duration_seconds_count{check="alerts/template"}
pint_check_duration_seconds_sum{check="labels/conflict"}
pint_check_duration_seconds_count{check="labels/conflict"}
pint_check_duration_seconds_sum{check="promql/fragile"}
pint_check_duration_seconds_count{check="promql/fragile"}
pint_check_duration_seconds_sum{check="promql/range_query"}
Expand Down
2 changes: 1 addition & 1 deletion cmd/pint/tests/0103_file_disable.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ level=info msg="Loading configuration file" path=.pint.hcl
level=debug msg="File parsed" path=rules/0001.yml rules=1
level=debug msg="Starting query workers" name=prom uri=http://127.0.0.1:7103 workers=16
level=debug msg="Found recording rule" lines=9-10 path=rules/0001.yml record=colo:test1
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/vector_matching(prom)"] path=rules/0001.yml rule=colo:test1
level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/vector_matching(prom)","labels/conflict(prom)"] path=rules/0001.yml rule=colo:test1
level=debug msg="Stopping query workers" name=prom uri=http://127.0.0.1:7103
-- rules/0001.yml --
# This should skip all online checks
Expand Down
27 changes: 26 additions & 1 deletion cmd/pint/tests/0108_rule_duplicate.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,42 @@ cmp stderr stderr.txt

-- stderr.txt --
level=info msg="Loading configuration file" path=.pint.hcl
level=error msg="Query returned an error" error="failed to query Prometheus config: Get \"http://127.0.0.1:7108/api/v1/status/config\": dial tcp 127.0.0.1:7108: connect: connection refused" query=/api/v1/status/config uri=http://127.0.0.1:7108
level=error msg="Query returned an error" error="failed to query Prometheus config: Get \"http://127.0.0.1:7108/api/v1/status/config\": dial tcp 127.0.0.1:7108: connect: connection refused" query=/api/v1/status/config uri=http://127.0.0.1:7108
level=error msg="Query returned an error" error="failed to query Prometheus config: Get \"http://127.0.0.1:7108/api/v1/status/config\": dial tcp 127.0.0.1:7108: connect: connection refused" query=/api/v1/status/config uri=http://127.0.0.1:7108
level=error msg="Query returned an error" error="failed to query Prometheus config: Get \"http://127.0.0.1:7108/api/v1/status/config\": dial tcp 127.0.0.1:7108: connect: connection refused" query=/api/v1/status/config uri=http://127.0.0.1:7108
level=error msg="Query returned an error" error="failed to query Prometheus config: Get \"http://127.0.0.1:7108/api/v1/status/config\": dial tcp 127.0.0.1:7108: connect: connection refused" query=/api/v1/status/config uri=http://127.0.0.1:7108
rules/0001.yml:1-2: duplicated rule, identical rule found at rules/0002.yml:1 (rule/duplicate)
1 | - record: "colo:duplicate"
2 | expr: sum(foo) without(job)

rules/0001.yml:7-8: couldn't run "labels/conflict" checks due to prometheus "prom" at http://127.0.0.1:7108 connection error: connection refused (labels/conflict)
7 | labels:
8 | file: a

rules/0001.yml:9-12: duplicated rule, identical rule found at rules/0002.yml:11 (rule/duplicate)
9 | - record: "colo:labels:equal"
10 | expr: sum(foo) without(job)
11 | labels:
12 | same: yes

level=info msg="Problems found" Bug=2
rules/0001.yml:11-12: couldn't run "labels/conflict" checks due to prometheus "prom" at http://127.0.0.1:7108 connection error: connection refused (labels/conflict)
11 | labels:
12 | same: yes

rules/0002.yml:5-6: couldn't run "labels/conflict" checks due to prometheus "prom" at http://127.0.0.1:7108 connection error: connection refused (labels/conflict)
5 | labels:
6 | empty: nope

rules/0002.yml:9-10: couldn't run "labels/conflict" checks due to prometheus "prom" at http://127.0.0.1:7108 connection error: connection refused (labels/conflict)
9 | labels:
10 | file: b

rules/0002.yml:13-14: couldn't run "labels/conflict" checks due to prometheus "prom" at http://127.0.0.1:7108 connection error: connection refused (labels/conflict)
13 | labels:
14 | same: yes

level=info msg="Problems found" Bug=7
level=fatal msg="Fatal error" error="problems found"
-- rules/0001.yml --
- record: "colo:duplicate"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pint.ok --no-color -d 'promql/.*' -d alerts/count lint rules
pint.ok --no-color -d 'promql/.*' -d alerts/count -d labels/conflict lint rules
! stdout .
cmp stderr stderr.txt

Expand Down
1 change: 1 addition & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Use [uber-go/automaxprocs](https://github.com/uber-go/automaxprocs) to
automatically set GOMAXPROCS to match Linux container CPU quota.
- Added [labels/conflict](checks/labels/conflict.md) check.

## v0.34.0

Expand Down
77 changes: 77 additions & 0 deletions docs/checks/labels/conflict.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
---
layout: default
parent: Checks
grand_parent: Documentation
---

# labels/conflict

This check will look for any conflicting labels used in rules.
Below is the list of conflicts it looks for.

## External labels

If recording rules are manually setting some lables that are
already present in `external_labels` Prometheus configuration option
then both labels might conflict when metrics are federated or when sending
alerts.

## Configuration

This check doesn't have any configuration options.

## How to enable it

This check is enabled by default for all configured Prometheus servers.

Example:

```js
prometheus "prod" {
uri = "https://prometheus-prod.example.com"
timeout = "60s"
include = [
"rules/prod/.*",
"rules/common/.*",
]
}

prometheus "dev" {
uri = "https://prometheus-dev.example.com"
timeout = "30s"
include = [
"rules/dev/.*",
"rules/common/.*",
]
}
```

## How to disable it

You can disable this check globally by adding this config block:

```js
checks {
disabled = ["labels/conflict"]
}
```

You can also disable it for all rules inside given file by adding
a comment anywhere in that file. Example:

`# pint file/disable labels/conflict`

Or you can disable it per rule by adding a comment to it. Example:

`# pint disable labels/conflict`

If you want to disable only individual instances of this check
you can add a more specific comment.

`# pint disable labels/conflict($prometheus)`

Where `$prometheus` is the name of Prometheus server to disable.

Example:

`# pint disable labels/conflict(prod)`
2 changes: 2 additions & 0 deletions internal/checks/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var (
AlertsCheckName,
AlertForCheckName,
TemplateCheckName,
LabelsConflictCheckName,
AggregationCheckName,
ComparisonCheckName,
FragileCheckName,
Expand All @@ -33,6 +34,7 @@ var (
}
OnlineChecks = []string{
AlertsCheckName,
LabelsConflictCheckName,
RangeQueryCheckName,
RateCheckName,
VectorMatchingCheckName,
Expand Down
73 changes: 73 additions & 0 deletions internal/checks/labels_conflict.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package checks

import (
"context"
"fmt"

"github.com/cloudflare/pint/internal/discovery"
"github.com/cloudflare/pint/internal/parser"
"github.com/cloudflare/pint/internal/promapi"
)

const (
LabelsConflictCheckName = "labels/conflict"
)

func NewLabelsConflictCheck(prom *promapi.FailoverGroup) LabelsConflictCheck {
return LabelsConflictCheck{prom: prom}
}

type LabelsConflictCheck struct {
prom *promapi.FailoverGroup
}

func (c LabelsConflictCheck) Meta() CheckMeta {
return CheckMeta{IsOnline: true}
}

func (c LabelsConflictCheck) String() string {
return fmt.Sprintf("%s(%s)", LabelsConflictCheckName, c.prom.Name())
}

func (c LabelsConflictCheck) Reporter() string {
return LabelsConflictCheckName
}

func (c LabelsConflictCheck) Check(ctx context.Context, path string, rule parser.Rule, entries []discovery.Entry) (problems []Problem) {
if rule.RecordingRule == nil || rule.RecordingRule.Expr.SyntaxError != nil {
return nil
}

if rule.RecordingRule.Labels == nil {
return nil
}

cfg, err := c.prom.Config(ctx)
if err != nil {
text, severity := textAndSeverityFromError(err, c.Reporter(), c.prom.Name(), Warning)
problems = append(problems, Problem{
Fragment: rule.RecordingRule.Labels.Key.Value,
Lines: rule.RecordingRule.Labels.Lines(),
Reporter: c.Reporter(),
Text: text,
Severity: severity,
})
return
}

for _, label := range rule.RecordingRule.Labels.Items {
for k, v := range cfg.Config.Global.ExternalLabels {
if label.Key.Value == k {
problems = append(problems, Problem{
Fragment: fmt.Sprintf("%s: %s", label.Key.Value, label.Value.Value),
Lines: label.Lines(),
Reporter: c.Reporter(),
Text: fmt.Sprintf("%s external_labels already has %s=%q label set, please choose a different name for this label to avoid any conflicts", promText(c.prom.Name(), cfg.URI), k, v),
Severity: Warning,
})
}
}
}

return problems
}
101 changes: 101 additions & 0 deletions internal/checks/labels_conflict_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package checks_test

import (
"fmt"
"testing"
"time"

"github.com/cloudflare/pint/internal/checks"
"github.com/cloudflare/pint/internal/promapi"
)

func textExternalLabels(name, uri, k, v string) string {
return fmt.Sprintf("prometheus %q at %s external_labels already has %s=%q label set, please choose a different name for this label to avoid any conflicts", name, uri, k, v)
}

func newLabelsConflict(prom *promapi.FailoverGroup) checks.RuleChecker {
return checks.NewLabelsConflictCheck(prom)
}

func TestLabelsConflictCheck(t *testing.T) {
testCases := []checkTest{
{
description: "ignores rules with syntax errors",
content: "- record: foo\n expr: sum(foo) without(\n",
checker: newLabelsConflict,
prometheus: newSimpleProm,
problems: noProblems,
},
{
description: "ignores alerting rules",
content: "- alert: foo\n expr: up == 0\n",
checker: newLabelsConflict,
prometheus: newSimpleProm,
problems: noProblems,
},
{
description: "no labels",
content: "- record: foo\n expr: up == 0\n",
checker: newLabelsConflict,
prometheus: newSimpleProm,
problems: noProblems,
},
{
description: "connection refused",
content: "- record: foo\n expr: up == 0\n labels:\n foo: bar\n",
checker: newLabelsConflict,
prometheus: func(_ string) *promapi.FailoverGroup {
return simpleProm("prom", "http://127.0.0.1:1111", time.Second, false)
},
problems: func(uri string) []checks.Problem {
return []checks.Problem{
{
Fragment: "labels",
Lines: []int{3, 4},
Reporter: checks.LabelsConflictCheckName,
Text: checkErrorUnableToRun(checks.LabelsConflictCheckName, "prom", "http://127.0.0.1:1111", "connection refused"),
Severity: checks.Warning,
},
}
},
},
{
description: "conflict",
content: "- record: foo\n expr: up == 0\n labels:\n foo: bar\n",
checker: newLabelsConflict,
prometheus: newSimpleProm,
problems: func(uri string) []checks.Problem {
return []checks.Problem{
{
Fragment: "foo: bar",
Lines: []int{4},
Reporter: checks.LabelsConflictCheckName,
Text: textExternalLabels("prom", uri, "foo", "bob"),
Severity: checks.Warning,
},
}
},
mocks: []*prometheusMock{
{
conds: []requestCondition{requireConfigPath},
resp: configResponse{yaml: "global:\n external_labels:\n foo: bob\n"},
},
},
},
{
description: "no conflict",
content: "- record: foo\n expr: up == 0\n labels:\n foo: bar\n",
checker: newLabelsConflict,
prometheus: newSimpleProm,
problems: noProblems,
mocks: []*prometheusMock{
{
conds: []requestCondition{requireConfigPath},
resp: configResponse{yaml: "global:\n external_labels:\n bob: bob\n"},
},
},
},
}

runTests(t, testCases)
}
Loading

0 comments on commit 0c66d41

Please sign in to comment.