Skip to content

Commit

Permalink
Merge pull request #1161 from cloudflare/source
Browse files Browse the repository at this point in the history 
Improve label source discovery
  • Loading branch information
@prymitive
prymitive authored Oct 23, 2024
2 parents 5205bcc + 1230821 commit 14f0935
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 37 deletions.
31 changes: 17 additions & 14 deletions internal/checks/promql_fragile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,19 +127,22 @@ NEXT:
}

func (c FragileCheck) checkSampling(node *parser.PromQLNode) (problems []exprProblem) {
src := utils.LabelsSource(node)
if src.Type != utils.AggregateSource {
return nil
}
if src.EmptyLabels {
return nil
}
if !slices.Contains([]string{"topk", "bottomk", "limit", "limit_ratio"}, src.Operation) {
return nil
s := utils.LabelsSource(node)
for _, src := range append(s.Alternatives, s) {
if src.Type != utils.AggregateSource {
continue
}
if src.FixedLabels && len(src.OnlyLabels) == 0 {
continue
}
if !slices.Contains([]string{"topk", "bottomk", "limit", "limit_ratio"}, src.Operation) {
continue
}
problems = append(problems, exprProblem{
text: fmt.Sprintf("Using `%s` to select time series might return different set of time series on every query, which would cause flapping alerts.", src.Operation),
details: FragileCheckSamplingDetails,
severity: Warning,
})
}
return append(problems, exprProblem{
text: fmt.Sprintf("Using `%s` to select time series might return different set of time series on every query, which would cause flapping alerts.", src.Operation),
details: FragileCheckSamplingDetails,
severity: Warning,
})
return problems
}
50 changes: 50 additions & 0 deletions internal/checks/promql_fragile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,56 @@ func TestFragileCheck(t *testing.T) {
}
},
},
{
description: "warns about topk() as source of series (or)",
content: "- alert: foo\n expr: bar or topk(10, foo)\n",
checker: newFragileCheck,
prometheus: noProm,
problems: func(_ string) []checks.Problem {
return []checks.Problem{
{
Lines: parser.LineRange{
First: 2,
Last: 2,
},
Reporter: checks.FragileCheckName,
Text: fragileSampleFunc("topk"),
Details: checks.FragileCheckSamplingDetails,
Severity: checks.Warning,
},
}
},
},
{
description: "warns about topk() as source of series (multiple)",
content: "- alert: foo\n expr: bar or topk(10, foo) or bottomk(10, foo)\n",
checker: newFragileCheck,
prometheus: noProm,
problems: func(_ string) []checks.Problem {
return []checks.Problem{
{
Lines: parser.LineRange{
First: 2,
Last: 2,
},
Reporter: checks.FragileCheckName,
Text: fragileSampleFunc("topk"),
Details: checks.FragileCheckSamplingDetails,
Severity: checks.Warning,
},
{
Lines: parser.LineRange{
First: 2,
Last: 2,
},
Reporter: checks.FragileCheckName,
Text: fragileSampleFunc("bottomk"),
Details: checks.FragileCheckSamplingDetails,
Severity: checks.Warning,
},
}
},
},
{
description: "ignores aggregated topk()",
content: "- alert: foo\n expr: min(topk(10, foo)) > 5000\n",
Expand Down
12 changes: 7 additions & 5 deletions internal/parser/utils/source.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ type Source struct {
ExcludedLabels []string // Labels guaranteed to be excluded from the results (without).
OnlyLabels []string // Labels that are the only ones that can be present on the results (by).
GuaranteedLabels []string // Labels guaranteed to be present on the results (matchers).
Alternatives []Source // Alternative lable sources
Type SourceType
EmptyLabels bool // No labels at all will be present on the results.
FixedLabels bool // Labels are fixed and only allowed labels can be present.
}

func LabelsSource(node *parser.PromQLNode) Source {
Expand All @@ -56,8 +57,8 @@ func parseAggregation(n *promParser.AggregateExpr) (s Source) {
s.OnlyLabels = removeFromSlice(s.OnlyLabels, name)
}
} else {
s.FixedLabels = true
if len(n.Grouping) == 0 {
s.EmptyLabels = true
s.GuaranteedLabels = nil
s.OnlyLabels = nil
} else {
Expand Down Expand Up @@ -105,9 +106,8 @@ func walkNode(node promParser.Node) (s Source) {
s.Operation = "count_values"
// Param is the label to store the count value in.
s.GuaranteedLabels = append(s.GuaranteedLabels, n.Param.(*promParser.StringLiteral).Val)
if s.EmptyLabels || len(s.OnlyLabels) > 0 {
if s.FixedLabels {
s.OnlyLabels = append(s.OnlyLabels, n.Param.(*promParser.StringLiteral).Val)
s.EmptyLabels = false
}
case promParser.QUANTILE:
s = parseAggregation(n)
Expand Down Expand Up @@ -142,8 +142,10 @@ func walkNode(node promParser.Node) (s Source) {
case n.VectorMatching.Card == promParser.CardOneToMany:
s = walkNode(n.RHS)
case n.VectorMatching.Card == promParser.CardManyToMany:
// FIXME need to handle 'foo or bar' here, we might have multiple selectors as the source.
s = walkNode(n.LHS)
if n.Op == promParser.LOR {
s.Alternatives = append(s.Alternatives, walkNode(n.RHS))
}
case n.VectorMatching.Card == promParser.CardManyToOne:
s = walkNode(n.LHS)
}
Expand Down
74 changes: 56 additions & 18 deletions internal/parser/utils/source_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "sum",
Selector: mustParseVector(`foo{job="myjob"}`, 4),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand All @@ -152,10 +152,11 @@ func TestLabelsSource(t *testing.T) {
{
expr: `sum(foo) by(job)`,
output: utils.Source{
Type: utils.AggregateSource,
Operation: "sum",
Selector: mustParseVector(`foo`, 4),
OnlyLabels: []string{"job"},
Type: utils.AggregateSource,
Operation: "sum",
Selector: mustParseVector(`foo`, 4),
OnlyLabels: []string{"job"},
FixedLabels: true,
},
},
{
Expand All @@ -166,6 +167,7 @@ func TestLabelsSource(t *testing.T) {
Selector: mustParseVector(`foo{job="myjob"}`, 4),
OnlyLabels: []string{"job"},
GuaranteedLabels: []string{"job"},
FixedLabels: true,
},
},
{
Expand All @@ -184,7 +186,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "min",
Selector: mustParseVector(`foo{job="myjob"}`, 4),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand All @@ -193,7 +195,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "max",
Selector: mustParseVector(`foo{job="myjob"}`, 4),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand All @@ -204,15 +206,17 @@ func TestLabelsSource(t *testing.T) {
Selector: mustParseVector(`foo{job="myjob"}`, 4),
GuaranteedLabels: []string{"job"},
OnlyLabels: []string{"job"},
FixedLabels: true,
},
},
{
expr: `group(foo) by(job)`,
output: utils.Source{
Type: utils.AggregateSource,
Operation: "group",
Selector: mustParseVector(`foo`, 6),
OnlyLabels: []string{"job"},
Type: utils.AggregateSource,
Operation: "group",
Selector: mustParseVector(`foo`, 6),
OnlyLabels: []string{"job"},
FixedLabels: true,
},
},
{
Expand All @@ -221,7 +225,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "stddev",
Selector: mustParseVector(`foo`, 12),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand All @@ -230,7 +234,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "stdvar",
Selector: mustParseVector(`foo`, 12),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand Down Expand Up @@ -289,7 +293,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "quantile",
Selector: mustParseVector(`foo`, 19),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand All @@ -300,6 +304,7 @@ func TestLabelsSource(t *testing.T) {
Selector: mustParseVector(`build_version`, 24),
GuaranteedLabels: []string{"version"},
OnlyLabels: []string{"version"},
FixedLabels: true,
},
},
{
Expand Down Expand Up @@ -330,6 +335,7 @@ func TestLabelsSource(t *testing.T) {
Selector: mustParseVector(`build_version`, 24),
GuaranteedLabels: []string{"version"},
OnlyLabels: []string{"job", "version"},
FixedLabels: true,
},
},
{
Expand Down Expand Up @@ -425,7 +431,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "count",
Selector: mustParseVector(`foo`, 6),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand All @@ -434,7 +440,7 @@ func TestLabelsSource(t *testing.T) {
Type: utils.AggregateSource,
Operation: "count",
Selector: mustParseVector(`up{job="a"}`, 6),
EmptyLabels: true,
FixedLabels: true,
},
},
{
Expand Down Expand Up @@ -472,19 +478,50 @@ func TestLabelsSource(t *testing.T) {
},
},
{
// FIXME
expr: `foo or bar`,
output: utils.Source{
Type: utils.SelectorSource,
Selector: mustParseVector(`foo`, 0),
Alternatives: []utils.Source{
{
Type: utils.SelectorSource,
Selector: mustParseVector(`bar`, 7),
},
},
},
},
{
expr: `foo or bar or baz`,
output: utils.Source{
Type: utils.SelectorSource,
Selector: mustParseVector(`foo`, 0),
Alternatives: []utils.Source{
{
Type: utils.SelectorSource,
Selector: mustParseVector(`bar`, 7),
},
{
Type: utils.SelectorSource,
Selector: mustParseVector(`baz`, 14),
},
},
},
},
{
// FIXME
expr: `(foo or bar) or baz`,
output: utils.Source{
Type: utils.SelectorSource,
Selector: mustParseVector(`foo`, 1),
Alternatives: []utils.Source{
{
Type: utils.SelectorSource,
Selector: mustParseVector(`bar`, 8),
},
{
Type: utils.SelectorSource,
Selector: mustParseVector(`baz`, 16),
},
},
},
},
{
Expand All @@ -494,6 +531,7 @@ func TestLabelsSource(t *testing.T) {
Operation: "count",
Selector: mustParseVector(`up{job="foo", cluster="dev"}`, 10),
ExcludedLabels: []string{"job", "cluster"},
FixedLabels: true,
},
},
{
Expand Down

0 comments on commit 14f0935

Please sign in to comment.