From 1887ad0a9ecf3e60faa66c3b3190950167f47e0e Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Wed, 30 Oct 2024 13:18:49 +0000 Subject: [PATCH] Move more label reason to source --- internal/checks/alerts_template.go | 44 ++----- internal/checks/alerts_template_test.go | 135 +++++++++++++++------ internal/parser/utils/source.go | 128 +++++++++++++++++--- internal/parser/utils/source_test.go | 150 ++++++++++++++++++++++++ 4 files changed, 370 insertions(+), 87 deletions(-) diff --git a/internal/checks/alerts_template.go b/internal/checks/alerts_template.go index e80ebd53..bc33e308 100644 --- a/internal/checks/alerts_template.go +++ b/internal/checks/alerts_template.go @@ -24,12 +24,6 @@ import ( const ( TemplateCheckName = "alerts/template" TemplateCheckSyntaxDetails = `Supported template syntax is documented [here](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/#templating).` - TemplateCheckAbsentDetails = `The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series. -You will only get any results back if the metric selector you pass doesn't match anything. -Since there are no matching time series there are also no labels. If some time series is missing you cannot read its labels. -This means that the only labels you can get back from absent call are the ones you pass to it. -If you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the ` + "`up`" + ` metric instead.` - TemplateCheckLabelsDetails = `This query doesn't seem to be using any time series and so cannot have any labels.` ) var ( @@ -464,37 +458,13 @@ func checkQueryLabels(query, labelName, labelValue string, src []utils.Source) ( } func textForProblem(query, label, reasonLabel string, src utils.Source, severity Severity) exprProblem { - switch { - case src.Operation == "absent": - return exprProblem{ - text: fmt.Sprintf("Template is using `%s` label but `%s` is not passing it.", label, src.Call.String()), - details: TemplateCheckAbsentDetails, - severity: severity, - } - case src.Operation == "vector": - return exprProblem{ - text: fmt.Sprintf("Template is using `%s` label but `%s` doesn't produce any labels.", label, src.Call.String()), - details: TemplateCheckLabelsDetails, - severity: severity, - } - case src.Returns == promParser.ValueTypeScalar, src.Returns == promParser.ValueTypeString: - return exprProblem{ - text: fmt.Sprintf("Template is using `%s` label but the query doesn't produce any labels.", label), - details: TemplateCheckLabelsDetails, - severity: severity, - } - default: - return exprProblem{ - text: fmt.Sprintf("Template is using `%s` label but the query results won't have this label.", label), - details: maybeAddQueryFragment(query, src.ExcludeReason[reasonLabel].Fragment, src.ExcludeReason[reasonLabel].Reason), - severity: severity, - } + details := src.ExcludeReason[reasonLabel].Reason + if query != src.ExcludeReason[reasonLabel].Fragment { + details = fmt.Sprintf("%s\nQuery fragment causing this problem: `%s`.", details, src.ExcludeReason[reasonLabel].Fragment) } -} - -func maybeAddQueryFragment(query, fragment, msg string) string { - if fragment == query { - return msg + return exprProblem{ + text: fmt.Sprintf("Template is using `%s` label but the query results won't have this label.", label), + details: details, + severity: severity, } - return fmt.Sprintf("%s\nQuery fragment causing this problem: `%s`.", msg, fragment) } diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go index e7277d4c..1fca8f9c 100644 --- a/internal/checks/alerts_template_test.go +++ b/internal/checks/alerts_template_test.go @@ -496,8 +496,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but `absent(foo{job=\"bar\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"bar\"})`.", Severity: checks.Bug, }, { @@ -506,8 +506,8 @@ func TestTemplateCheck(t *testing.T) { Last: 7, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but `absent(foo{job=\"bar\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"bar\"})`.", Severity: checks.Bug, }, { @@ -516,8 +516,8 @@ func TestTemplateCheck(t *testing.T) { Last: 7, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `foo` label but `absent(foo{job=\"bar\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `foo` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"bar\"})`.", Severity: checks.Bug, }, { @@ -526,8 +526,8 @@ func TestTemplateCheck(t *testing.T) { Last: 8, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `xxx` label but `absent(foo{job=\"bar\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `xxx` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"bar\"})`.", Severity: checks.Bug, }, } @@ -551,8 +551,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but `absent(sum by (job, instance) (foo))` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", Severity: checks.Bug, }, { @@ -561,8 +561,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `job` label but `absent(sum by (job, instance) (foo))` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `job` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", Severity: checks.Bug, }, } @@ -586,8 +586,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but `absent(sum by (job) (foo))` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", Severity: checks.Bug, }, { @@ -596,8 +596,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `job` label but `absent(sum by (job) (foo))` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `job` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", Severity: checks.Bug, }, } @@ -621,8 +621,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `job` label but `absent({job=~\".+\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `job` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", Severity: checks.Bug, }, } @@ -646,8 +646,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `job` label but `absent(foo)` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `job` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo)`.", Severity: checks.Bug, }, } @@ -683,8 +683,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `cluster` label but `absent(foo{job=\"xxx\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `cluster` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"xxx\"})`.", Severity: checks.Bug, }, { @@ -693,8 +693,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `env` label but `absent(foo{job=\"xxx\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `env` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"xxx\"})`.", Severity: checks.Bug, }, } @@ -730,8 +730,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `cluster` label but `absent(foo{job=\"xxx\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `cluster` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"xxx\"})`.", Severity: checks.Bug, }, { @@ -740,8 +740,8 @@ func TestTemplateCheck(t *testing.T) { Last: 5, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `env` label but `absent(foo{job=\"xxx\"})` is not passing it.", - Details: checks.TemplateCheckAbsentDetails, + Text: "Template is using `env` label but the query results won't have this label.", + Details: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.\nQuery fragment causing this problem: `absent(foo{job=\"xxx\"})`.", Severity: checks.Bug, }, } @@ -1216,8 +1216,8 @@ func TestTemplateCheck(t *testing.T) { Last: 4, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but `vector(1)` doesn't produce any labels.", - Details: checks.TemplateCheckLabelsDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "Calling `vector()` will return a vector value with no labels.", Severity: checks.Bug, }, } @@ -1236,8 +1236,8 @@ func TestTemplateCheck(t *testing.T) { Last: 4, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but `vector(1)` doesn't produce any labels.", - Details: checks.TemplateCheckLabelsDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "Calling `vector()` will return a vector value with no labels.", Severity: checks.Bug, }, } @@ -1256,8 +1256,8 @@ func TestTemplateCheck(t *testing.T) { Last: 4, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `instance` label but the query doesn't produce any labels.", - Details: checks.TemplateCheckLabelsDetails, + Text: "Template is using `instance` label but the query results won't have this label.", + Details: "This returns a number value with no labels.\nQuery fragment causing this problem: `1`.", Severity: checks.Bug, }, { @@ -1266,8 +1266,8 @@ func TestTemplateCheck(t *testing.T) { Last: 4, }, Reporter: checks.TemplateCheckName, - Text: "Template is using `job` label but the query doesn't produce any labels.", - Details: checks.TemplateCheckLabelsDetails, + Text: "Template is using `job` label but the query results won't have this label.", + Details: "This returns a number value with no labels.\nQuery fragment causing this problem: `1`.", Severity: checks.Bug, }, } @@ -1411,6 +1411,69 @@ func TestTemplateCheck(t *testing.T) { } }, }, + { + description: "metric or (metric or vector)", + content: ` +- alert: Foo + expr: | + max without (instance) (metric1{exported_job="abc"}) == 0 or (metric2 OR on() vector(0)) == 0 + for: 15m + annotations: + summary: 'Foo is down in {{ $labels.colo_name }}' +`, + checker: newTemplateCheck, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 7, + Last: 7, + }, + Reporter: checks.TemplateCheckName, + Text: "Template is using `colo_name` label but the query results won't have this label.", + Details: "Calling `vector()` will return a vector value with no labels.\nQuery fragment causing this problem: `vector(0)`.", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "", + content: ` +- alert: Foo + expr: sum by (region, target, colo_name) (sum_over_time(probe_success{job="abc"}[5m]) or vector(1)) == 0 + for: 5m + annotations: + summary: "Probe from {{ $labels.region }} to {{ $labels.target }} failed for the last 5m" +`, + checker: newTemplateCheck, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 6, + Last: 6, + }, + Reporter: checks.TemplateCheckName, + Text: "Template is using `region` label but the query results won't have this label.", + Details: "Calling `vector()` will return a vector value with no labels.\nQuery fragment causing this problem: `vector(1)`.", + Severity: checks.Bug, + }, + { + Lines: parser.LineRange{ + First: 6, + Last: 6, + }, + Reporter: checks.TemplateCheckName, + Text: "Template is using `target` label but the query results won't have this label.", + Details: "Calling `vector()` will return a vector value with no labels.\nQuery fragment causing this problem: `vector(1)`.", + Severity: checks.Bug, + }, + } + }, + }, } runTests(t, testCases) } diff --git a/internal/parser/utils/source.go b/internal/parser/utils/source.go index 9229e29c..8e673686 100644 --- a/internal/parser/utils/source.go +++ b/internal/parser/utils/source.go @@ -65,7 +65,17 @@ func walkNode(expr string, node promParser.Node) (src []Source) { case *promParser.NumberLiteral: s.Type = NumberSource s.Returns = promParser.ValueTypeScalar + s.IncludedLabels = nil + s.GuaranteedLabels = nil s.FixedLabels = true + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: "This returns a number value with no labels.", + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) src = append(src, s) case *promParser.ParenExpr: @@ -74,7 +84,17 @@ func walkNode(expr string, node promParser.Node) (src []Source) { case *promParser.StringLiteral: s.Type = StringSource s.Returns = promParser.ValueTypeString + s.IncludedLabels = nil + s.GuaranteedLabels = nil s.FixedLabels = true + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: "This returns a string value with no labels.", + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) src = append(src, s) case *promParser.UnaryExpr: @@ -266,7 +286,6 @@ func parseAggregation(expr string, n *promParser.AggregateExpr) (src []Source) { ) } } else { - s.FixedLabels = true if len(n.Grouping) == 0 { s.IncludedLabels = nil s.GuaranteedLabels = nil @@ -279,20 +298,29 @@ func parseAggregation(expr string, n *promParser.AggregateExpr) (src []Source) { }, ) } else { - s.IncludedLabels = appendToSlice(s.IncludedLabels, n.Grouping...) - for _, name := range n.Grouping { - s.ExcludedLabels = removeFromSlice(s.ExcludedLabels, name) + // Check if source of labels already fixes them. + if !s.FixedLabels { + s.IncludedLabels = appendToSlice(s.IncludedLabels, n.Grouping...) + for _, name := range n.Grouping { + s.ExcludedLabels = removeFromSlice(s.ExcludedLabels, name) + } + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf("Query is using aggregation with `by(%s)`, only labels included inside `by(...)` will be present on the results.", + strings.Join(n.Grouping, ", ")), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) + } + for _, name := range s.GuaranteedLabels { + if !slices.Contains(n.Grouping, name) { + s.GuaranteedLabels = removeFromSlice(s.GuaranteedLabels, name) + } } - s.ExcludeReason = setInMap( - s.ExcludeReason, - "", - ExcludedLabel{ - Reason: fmt.Sprintf("Query is using aggregation with `by(%s)`, only labels included inside `by(...)` will be present on the results.", - strings.Join(n.Grouping, ", ")), - Fragment: getQueryFragment(expr, n.PosRange), - }, - ) } + s.FixedLabels = true } s.Type = AggregateSource s.Returns = promParser.ValueTypeVector @@ -352,6 +380,19 @@ func parseCall(expr string, n *promParser.Call) (s Source) { s.IncludedLabels = appendToSlice(s.IncludedLabels, name) s.GuaranteedLabels = appendToSlice(s.GuaranteedLabels, name) } + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf(`The [%s()](https://prometheus.io/docs/prometheus/latest/querying/functions/#%s) function is used to check if provided query doesn't match any time series. +You will only get any results back if the metric selector you pass doesn't match anything. +Since there are no matching time series there are also no labels. If some time series is missing you cannot read its labels. +This means that the only labels you can get back from absent call are the ones you pass to it. +If you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `+"`up`"+` metric instead.`, + n.Func.Name, n.Func.Name), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) case "avg_over_time", "count_over_time", "last_over_time", "max_over_time", "min_over_time", "present_over_time", "quantile_over_time", "stddev_over_time", "stdvar_over_time", "sum_over_time": // No change to labels. @@ -364,6 +405,17 @@ func parseCall(expr string, n *promParser.Call) (s Source) { // Otherwise no change to labels. if len(s.Call.Args) == 0 { s.FixedLabels = true + s.IncludedLabels = nil + s.GuaranteedLabels = nil + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf("Calling `%s()` with no arguments will return an empty time series with no labels.", + n.Func.Name), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) } else { s.GuaranteedLabels = appendToSlice(s.GuaranteedLabels, labelsFromSelectors(guaranteedLabelsMatches, s.Selectors...)...) } @@ -396,11 +448,31 @@ func parseCall(expr string, n *promParser.Call) (s Source) { case "pi": s.Returns = promParser.ValueTypeScalar + s.IncludedLabels = nil + s.GuaranteedLabels = nil s.FixedLabels = true + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf("Calling `%s()` will return a scalar value with no labels.", n.Func.Name), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) case "scalar": s.Returns = promParser.ValueTypeScalar + s.IncludedLabels = nil + s.GuaranteedLabels = nil s.FixedLabels = true + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf("Calling `%s()` will return a scalar value with no labels.", n.Func.Name), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) case "sort", "sort_desc": // No change to labels. @@ -408,7 +480,17 @@ func parseCall(expr string, n *promParser.Call) (s Source) { case "time": s.Returns = promParser.ValueTypeScalar + s.IncludedLabels = nil + s.GuaranteedLabels = nil s.FixedLabels = true + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf("Calling `%s()` will return a scalar value with no labels.", n.Func.Name), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) case "timestamp": // No change to labels. @@ -417,7 +499,17 @@ func parseCall(expr string, n *promParser.Call) (s Source) { case "vector": s.Returns = promParser.ValueTypeVector + s.IncludedLabels = nil + s.GuaranteedLabels = nil s.FixedLabels = true + s.ExcludeReason = setInMap( + s.ExcludeReason, + "", + ExcludedLabel{ + Reason: fmt.Sprintf("Calling `%s()` will return a vector value with no labels.", n.Func.Name), + Fragment: getQueryFragment(expr, n.PosRange), + }, + ) default: // Unsupported function @@ -439,7 +531,15 @@ func parseBinOps(expr string, n *promParser.BinaryExpr) (src []Source) { } } if !ok { - src = append(src, walkNode(expr, n.RHS)...) + for _, rs := range walkNode(expr, n.RHS) { + if rs.Returns != promParser.ValueTypeScalar && rs.Returns != promParser.ValueTypeString { + ok = true + src = append(src, rs) + } + } + } + if !ok { + src = append(src, s) } // foo{} + bar{} diff --git a/internal/parser/utils/source_test.go b/internal/parser/utils/source_test.go index 15000b0f..7fee9aa7 100644 --- a/internal/parser/utils/source_test.go +++ b/internal/parser/utils/source_test.go @@ -46,6 +46,12 @@ func TestLabelsSource(t *testing.T) { Type: utils.NumberSource, Returns: promParser.ValueTypeScalar, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "This returns a number value with no labels.", + Fragment: "1", + }, + }, }, }, }, @@ -56,6 +62,28 @@ func TestLabelsSource(t *testing.T) { Type: utils.NumberSource, Returns: promParser.ValueTypeScalar, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "This returns a number value with no labels.", + Fragment: "1", + }, + }, + }, + }, + }, + { + expr: `1 > bool 0`, + output: []utils.Source{ + { + Type: utils.NumberSource, + Returns: promParser.ValueTypeScalar, + FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "This returns a number value with no labels.", + Fragment: "1", + }, + }, }, }, }, @@ -66,6 +94,12 @@ func TestLabelsSource(t *testing.T) { Type: utils.StringSource, Returns: promParser.ValueTypeString, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "This returns a string value with no labels.", + Fragment: `"test"`, + }, + }, }, }, }, @@ -1164,6 +1198,12 @@ func TestLabelsSource(t *testing.T) { Returns: promParser.ValueTypeVector, Operation: "year", FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "Calling `year()` with no arguments will return an empty time series with no labels.", + Fragment: `year()`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "year", @@ -1336,6 +1376,12 @@ sum(foo:count) by(job) > 20`, IncludedLabels: []string{"job"}, GuaranteedLabels: []string{"job"}, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(foo{job="bar"})`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1369,6 +1415,12 @@ sum(foo:count) by(job) > 20`, IncludedLabels: []string{"job", "env"}, GuaranteedLabels: []string{"job", "env"}, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(foo{job="bar", cluster!="dev", instance=~".+", env="prod"})`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1400,6 +1452,12 @@ sum(foo:count) by(job) > 20`, mustParseVector(`foo`, 11), }, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(sum(foo) by(job, instance))`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1441,6 +1499,12 @@ sum(foo:count) by(job) > 20`, IncludedLabels: []string{"job", "xxx"}, GuaranteedLabels: []string{"job", "xxx"}, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(foo{job="prometheus", xxx="1"})`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1514,6 +1578,12 @@ sum(foo:count) by(job) > 20`, mustParseVector(`foo`, 7), }, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(foo)`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1540,6 +1610,12 @@ sum(foo:count) by(job) > 20`, mustParseVector(`bar`, 22), }, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(bar)`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1571,6 +1647,12 @@ sum(foo:count) by(job) > 20`, mustParseVector(`foo`, 17), }, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent_over_time()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent_over_time) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent_over_time(foo[5m])`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent_over_time", @@ -1597,6 +1679,12 @@ sum(foo:count) by(job) > 20`, mustParseVector(`bar`, 36), }, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(bar)`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1630,6 +1718,12 @@ sum(foo:count) by(job) > 20`, IncludedLabels: []string{"job", "cluster", "env"}, GuaranteedLabels: []string{"job"}, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(foo{job="xxx"})`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1663,6 +1757,12 @@ sum(foo:count) by(job) > 20`, IncludedLabels: []string{"job"}, GuaranteedLabels: []string{"job"}, FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "The [absent()](https://prometheus.io/docs/prometheus/latest/querying/functions/#absent) function is used to check if provided query doesn't match any time series.\nYou will only get any results back if the metric selector you pass doesn't match anything.\nSince there are no matching time series there are also no labels. If some time series is missing you cannot read its labels.\nThis means that the only labels you can get back from absent call are the ones you pass to it.\nIf you're hoping to get instance specific labels this way and alert when some target is down then that won't work, use the `up` metric instead.", + Fragment: `absent(foo{job="xxx"})`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "absent", @@ -1691,6 +1791,12 @@ sum(foo:count) by(job) > 20`, Returns: promParser.ValueTypeVector, Operation: "vector", FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "Calling `vector()` will return a vector value with no labels.", + Fragment: `vector(1)`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "vector", @@ -1756,6 +1862,12 @@ sum(foo:count) by(job) > 20`, Returns: promParser.ValueTypeVector, Operation: "days_in_month", FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "Calling `days_in_month()` with no arguments will return an empty time series with no labels.", + Fragment: `days_in_month()`, + }, + }, Call: &promParser.Call{ Func: &promParser.Function{ Name: "days_in_month", @@ -2061,6 +2173,44 @@ or avg without(router, colo_id, instance) (router_anycast_prefix_enabled{cidr_us }, }, }, + { + expr: ` +sum by (region, target, colo_name) ( + sum_over_time(probe_success{job="abc"}[5m]) + or + vector(1) +) == 0`, + output: []utils.Source{ + { + Type: utils.AggregateSource, + Returns: promParser.ValueTypeVector, + Operation: "sum", + Selectors: []*promParser.VectorSelector{ + mustParseVector(`probe_success{job="abc"}`, 56), + }, + IncludedLabels: []string{"region", "target", "colo_name"}, + FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "Query is using aggregation with `by(region, target, colo_name)`, only labels included inside `by(...)` will be present on the results.", + Fragment: "sum by (region, target, colo_name) (\n sum_over_time(probe_success{job=\"abc\"}[5m])\n\tor\n\tvector(1)\n)", + }, + }, + }, + { + Type: utils.AggregateSource, + Returns: promParser.ValueTypeVector, + Operation: "sum", + FixedLabels: true, + ExcludeReason: map[string]utils.ExcludedLabel{ + "": { + Reason: "Calling `vector()` will return a vector value with no labels.", + Fragment: "vector(1)", + }, + }, + }, + }, + }, } for _, tc := range testCases {