Merge pull request #1028 from cloudflare/comments

Improve comment warnings

cmd/pint/tests/0160_ci_comment_edit.txt

@@ -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)

cmd/pint/tests/0176_comments.txt

@@ -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"
+}

docs/changelog.md

@@ -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

docs/checks/promql/series.md

@@ -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:

internal/checks/promql_regexp.go

@@ -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 (
@@ -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
 		}

internal/checks/promql_series.go

@@ -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"
@@ -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
 		}
@@ -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(),
@@ -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
 }
@@ -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) {
@@ -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
 		}
@@ -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 {
@@ -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:
@@ -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)
 		}
 	}