Skip to content

Commit

Permalink
Improve comment warnings
Browse files Browse the repository at this point in the history
  • Loading branch information
prymitive committed Jul 11, 2024
1 parent f4eaf91 commit da020e3
Show file tree
Hide file tree
Showing 10 changed files with 411 additions and 109 deletions.
2 changes: 1 addition & 1 deletion cmd/pint/tests/0160_ci_comment_edit.txt
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ level=INFO msg="Problems found" Bug=2 Warning=1
rules.yml:8 Bug: `prom` Prometheus server at http://127.0.0.1:7160 didn't have any series for `up` metric in the last 1w. (promql/series)
8 | expr: up == 0

rules.yml:8 Warning: pint disable comment `promql/series(xxx)` check doesn't match any selector in this query (promql/series)
rules.yml:8 Warning: pint disable comment `promql/series(xxx)` doesn't match any selector in this query (promql/series)
8 | expr: up == 0

rules.yml:16 Bug: `prom` Prometheus server at http://127.0.0.1:7160 didn't have any series for `up` metric in the last 1w. (promql/series)
Expand Down
45 changes: 45 additions & 0 deletions cmd/pint/tests/0176_comments.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
http response prometheus /api/v1/status/flags 200 {"status":"success","data":{"storage.tsdb.retention.time": "1d"}}
http response prometheus /api/v1/metadata 200 {"status":"success","data":{}}
http response prometheus /api/v1/status/config 200 {"status":"success","data":{"yaml":"global:\n scrape_interval: 1m\n"}}
http response prometheus /api/v1/query_range 200 {"status":"success","data":{"resultType":"matrix","result":[]}}
http response prometheus /api/v1/query 200 {"status":"success","data":{"resultType":"vector","result":[]}}
http start prometheus 127.0.0.1:7176

pint.error --no-color lint rules
! stdout .
cmp stderr stderr.txt

-- stderr.txt --
level=INFO msg="Loading configuration file" path=.pint.hcl
level=INFO msg="Finding all rules to check" paths=["rules"]
level=INFO msg="Configured new Prometheus server" name=prom uris=1 uptime=up tags=[] include=[] exclude=[]
level=WARN msg="No results for Prometheus uptime metric, you might have set uptime config option to a missing metric, please check your config" name=prom metric=up
level=WARN msg="Using dummy Prometheus uptime metric results with no gaps" name=prom metric=up
rules/1.yml:15 Bug: `prom` Prometheus server at http://127.0.0.1:7176 didn't have any series for `up` metric in the last 1w. (promql/series)
15 | expr: up == 0

level=INFO msg="Problems found" Bug=1
level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
-- rules/1.yml --
---
groups:
- name: "{{ source }}"
rules:
# pint ignore/begin
- alert: Ignored
# pint rule/set promql/series(colo_metadata) ignore/label-value brand
# pint rule/set promql/series ignore/label-value colo_status
expr: count(colo_metadata{colo_status="v", brand="b1"}) > 0
# pint ignore/end

# dummy comment 1
- alert: Active
# dummy comment 2
expr: up == 0

-- .pint.hcl --
prometheus "prom" {
uri = "http://127.0.0.1:7176"
failover = []
timeout = "5s"
}
5 changes: 5 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
- [promql/series](checks/promql/series.md) check will now generate warnings if there are `# pint disable`
or `# pint rule/set` comments that are not matching any valid query selector or Prometheus server.

### Fixed

- [promql/series](checks/promql/series.md) will now parse `rule/set` comments that target specific
time series selectors in PromQL the same way as `# pint disable` comments do.

## v0.61.2

### Fixed
Expand Down
2 changes: 1 addition & 1 deletion docs/checks/promql/series.md
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ Duration must follow syntax documented [here](https://prometheus.io/docs/prometh
To set `min-age` for specific metric:

```yaml
# pint rule/set promql/series($metric_name) min-age $duration
# pint rule/set promql/series($selector)) min-age $duration
```

Example:
Expand Down
3 changes: 2 additions & 1 deletion internal/checks/promql_regexp.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

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

const (
Expand Down Expand Up @@ -52,7 +53,7 @@ func (c RegexpCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule
}

done := map[string]struct{}{}
for _, selector := range getSelectors(expr.Query) {
for _, selector := range utils.HasVectorSelector(expr.Query) {
if _, ok := done[selector.String()]; ok {
continue
}
Expand Down
198 changes: 102 additions & 96 deletions internal/checks/promql_series.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/cloudflare/pint/internal/discovery"
"github.com/cloudflare/pint/internal/output"
"github.com/cloudflare/pint/internal/parser"
"github.com/cloudflare/pint/internal/parser/utils"
"github.com/cloudflare/pint/internal/promapi"

"github.com/prometheus/common/model"
Expand Down Expand Up @@ -120,10 +121,10 @@ func (c SeriesCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Ru

params := promapi.NewRelativeRange(settings.lookbackRangeDuration, settings.lookbackStepDuration)

selectors := getSelectors(expr.Query)
selectors := utils.HasVectorSelector(expr.Query)

done := map[string]bool{}
for _, selector := range selectors {
for _, selector := range getNonFallbackSelectors(expr.Query) {
if _, ok := done[selector.String()]; ok {
continue
}
Expand Down Expand Up @@ -539,12 +540,12 @@ func (c SeriesCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Ru
problems = append(problems, Problem{
Lines: expr.Value.Lines,
Reporter: c.Reporter(),
Text: fmt.Sprintf("pint %s comment `%s` check doesn't match any selector in this query", comments.DisableComment, disable.Match),
Text: fmt.Sprintf("pint %s comment `%s` doesn't match any selector in this query", comments.DisableComment, disable.Match),
Details: SeriesCheckUnusedDisableComment,
Severity: Warning,
})
}
for _, ruleSet := range orphanedRuleSetComments(c.Reporter(), rule, selectors) {
for _, ruleSet := range orphanedRuleSetComments(rule, selectors) {
problems = append(problems, Problem{
Lines: expr.Value.Lines,
Reporter: c.Reporter(),
Expand Down Expand Up @@ -635,39 +636,49 @@ func (c SeriesCheck) instantSeriesCount(ctx context.Context, query string) (int,

func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelector) (minAge time.Duration, problems []Problem) {
minAge = time.Hour * 2
prefixes := ruleSetMinAgePrefixes(c.Reporter(), selector)
for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) {
for _, prefix := range prefixes {
if !strings.HasPrefix(ruleSet.Value, prefix) {
matcher, key, value := parseRuleSet(ruleSet.Value)
if key != "min-age" {
continue
}
if matcher != "" {
isMatch, _ := matchSelectorToMetric(selector, matcher)
if !isMatch {
continue
}
val := strings.TrimPrefix(ruleSet.Value, prefix)
dur, err := model.ParseDuration(val)
if err != nil {
problems = append(problems, Problem{
Lines: rule.Lines,
Reporter: c.Reporter(),
Text: fmt.Sprintf("Failed to parse pint comment as duration: %s", err),
Details: SeriesCheckMinAgeDetails,
Severity: Warning,
})
} else {
minAge = time.Duration(dur)
}
}
dur, err := model.ParseDuration(value)
if err != nil {
problems = append(problems, Problem{
Lines: rule.Lines,
Reporter: c.Reporter(),
Text: fmt.Sprintf("Failed to parse pint comment as duration: %s", err),
Details: SeriesCheckMinAgeDetails,
Severity: Warning,
})
} else {
minAge = time.Duration(dur)
}
}

return minAge, problems
}

func (c SeriesCheck) isLabelValueIgnored(rule parser.Rule, selector promParser.VectorSelector, labelName string) bool {
values := ruleSetIgnoreLabelValValues(c.Reporter(), labelName, selector)
for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) {
for _, val := range values {
if ruleSet.Value == val {
return true
matcher, key, value := parseRuleSet(ruleSet.Value)
if key != "ignore/label-value" {
continue
}
if matcher != "" {
isMatch, _ := matchSelectorToMetric(selector, matcher)
if !isMatch {
continue
}
}
if labelName == value {
return true
}
}
return false
}
Expand All @@ -686,7 +697,7 @@ func (c SeriesCheck) textAndSeverity(settings *PromqlSeriesSettings, name, text
return text, s
}

func getSelectors(n *parser.PromQLNode) (selectors []promParser.VectorSelector) {
func getNonFallbackSelectors(n *parser.PromQLNode) (selectors []promParser.VectorSelector) {
LOOP:
for _, vs := range parser.WalkDownExpr[*promParser.VectorSelector](n) {
for _, bin := range parser.WalkUpExpr[*promParser.BinaryExpr](vs.Parent) {
Expand Down Expand Up @@ -735,26 +746,12 @@ func isDisabled(rule parser.Rule, selector promParser.VectorSelector) bool {
for _, disable := range comments.Only[comments.Disable](rule.Comments, comments.DisableType) {
if strings.HasPrefix(disable.Match, SeriesCheckName+"(") && strings.HasSuffix(disable.Match, ")") {
cs := strings.TrimSuffix(strings.TrimPrefix(disable.Match, SeriesCheckName+"("), ")")
// try full string or name match first
if cs == selector.String() || cs == selector.Name {
return true
}
// then try matchers
m, err := promParser.ParseMetricSelector(cs)
if err != nil {
isMatch, ok := matchSelectorToMetric(selector, cs)
if !ok {
continue
}
for _, l := range m {
var isMatch bool
for _, s := range selector.LabelMatchers {
if s.Type == l.Type && s.Name == l.Name && s.Value == l.Value {
isMatch = true
break
}
}
if !isMatch {
goto NEXT
}
if !isMatch {
goto NEXT
}
return true
}
Expand All @@ -763,6 +760,48 @@ func isDisabled(rule parser.Rule, selector promParser.VectorSelector) bool {
return false
}

func matchSelectorToMetric(selector promParser.VectorSelector, metric string) (bool, bool) {
// Try full string or name match first.
if metric == selector.String() || metric == selector.Name {
return true, true
}
// Then try matchers.
m, err := promParser.ParseMetricSelector(metric)
if err != nil {
// Ignore errors
return false, false
}
for _, l := range m {
var isMatch bool
for _, s := range selector.LabelMatchers {
if s.Type == l.Type && s.Name == l.Name && s.Value == l.Value {
return true, true
}
}
if !isMatch {
return false, true
}
}
return false, true
}

func parseRuleSet(s string) (matcher, key, value string) {
if strings.HasPrefix(s, SeriesCheckName+"(") {
matcher = strings.TrimPrefix(s[:strings.LastIndex(s, ")")], SeriesCheckName+"(")
s = s[strings.LastIndex(s, ")")+1:]
} else {
s = strings.TrimPrefix(s, SeriesCheckName)
}
parts := strings.Fields(s)
if len(parts) > 0 {
key = parts[0]
}
if len(parts) > 1 {
value = strings.Join(parts[1:], " ")
}
return matcher, key, value
}

func orphanedDisableComments(ctx context.Context, rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.Disable) {
var promNames, promTags []string
if val := ctx.Value(promapi.AllPrometheusServers); val != nil {
Expand All @@ -787,32 +826,13 @@ func orphanedDisableComments(ctx context.Context, rule parser.Rule, selectors []
continue
}
for _, selector := range selectors {
// try full string or name match first
if match == selector.String() || match == selector.Name {
wasUsed = true
goto NEXT
}
// then try matchers
m, err := promParser.ParseMetricSelector(match)
if err != nil {
isMatch, ok := matchSelectorToMetric(selector, match)
if !ok {
continue
}
var isMismatch bool
for _, l := range m {
var isMatch bool
for _, s := range selector.LabelMatchers {
if s.Type == l.Type && s.Name == l.Name && s.Value == l.Value {
isMatch = true
break
}
}
if !isMatch {
isMismatch = true
break
}
}
if !isMismatch {
if isMatch {
wasUsed = true
goto NEXT
}
}
NEXT:
Expand All @@ -823,45 +843,31 @@ func orphanedDisableComments(ctx context.Context, rule parser.Rule, selectors []
return orhpaned
}

func ruleSetIgnoreLabelValValues(reporter, labelName string, selector promParser.VectorSelector) []string {
bareSelector := stripLabels(selector)
return []string{
fmt.Sprintf("%s ignore/label-value %s", reporter, labelName),
fmt.Sprintf("%s(%s) ignore/label-value %s", reporter, bareSelector.String(), labelName),
fmt.Sprintf("%s(%s) ignore/label-value %s", reporter, selector.String(), labelName),
}
}

func ruleSetMinAgePrefixes(reporter string, selector promParser.VectorSelector) []string {
bareSelector := stripLabels(selector)
return []string{
reporter + " min-age ",
fmt.Sprintf("%s(%s) min-age ", reporter, bareSelector.String()),
fmt.Sprintf("%s(%s) min-age ", reporter, selector.String()),
}
}

func orphanedRuleSetComments(reporter string, rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.RuleSet) {
func orphanedRuleSetComments(rule parser.Rule, selectors []promParser.VectorSelector) (orhpaned []comments.RuleSet) {
for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) {
var used bool
var wasUsed bool
matcher, key, value := parseRuleSet(ruleSet.Value)
for _, selector := range selectors {
for _, lm := range selector.LabelMatchers {
for _, val := range ruleSetIgnoreLabelValValues(reporter, lm.Name, selector) {
if ruleSet.Value == val {
used = true
goto NEXT
}
if matcher != "" {
isMatch, _ := matchSelectorToMetric(selector, matcher)
if !isMatch {
continue
}
for _, val := range ruleSetMinAgePrefixes(reporter, selector) {
if strings.HasPrefix(ruleSet.Value, val) {
used = true
}
switch key {
case "min-age":
wasUsed = true
case "ignore/label-value":
for _, lm := range selector.LabelMatchers {
if lm.Name == value {
wasUsed = true
goto NEXT
}
}
}
}
NEXT:
if !used {
if !wasUsed {
orhpaned = append(orhpaned, ruleSet)
}
}
Expand Down
Loading

0 comments on commit da020e3

Please sign in to comment.