-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix sharding behavior for vector matches #5799
Conversation
pkg/querysharding/analyzer.go
Outdated
@@ -97,7 +97,7 @@ func (a *QueryAnalyzer) Analyze(query string) (QueryAnalysis, error) { | |||
return fmt.Errorf("expressions with %s are not shardable", n.Func.Name) | |||
} | |||
case *parser.BinaryExpr: | |||
if n.VectorMatching != nil { | |||
if n.VectorMatching != nil && n.VectorMatching.On { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdufel Can you add an unit test case please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing with @fpetkovski, I think we pinned down the issue - a recent change that removed the __name__
label exclusion from sharding caused this regression.
I adjusted the patch to ensure that __name__
isn't used as a shard label for the case of vector matches using the ignoring
keyword and added tests to the analyzer.
Sharding by __name__
appears to be fine for aggregations, but it needs special treatment for vector matching.
When analyzing a query composed of boolean expression with a non-"on" vector match, the current vertical sharding logic is incorrectly scoping to labels in the vector match and changing the semantics of the query. Example: ``` foo and without (lbl) bar ``` Signed-off-by: Samuel Dufel <[email protected]>
7caa8bd
to
3e4f24f
Compare
pkg/querysharding/analyzer.go
Outdated
@@ -99,6 +99,9 @@ func (a *QueryAnalyzer) Analyze(query string) (QueryAnalysis, error) { | |||
case *parser.BinaryExpr: | |||
if n.VectorMatching != nil { | |||
shardingLabels := without(n.VectorMatching.MatchingLabels, []string{"le"}) | |||
if !n.VectorMatching.On && len(shardingLabels) > 0 { | |||
shardingLabels = append(shardingLabels, "__name__") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. cc @fpetkovski for another look
I tested this patch on cortex and it indeed fixed the issue. Do we also need to add this on the thanos/pkg/querysharding/analyzer.go Lines 130 to 132 in 35a3e3d
As a side note, the https://github.com/prometheus/compliance/tree/main/promql was not able to catch this issue. I added locally the following query to cover this problem:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix, this looks good 👍
pkg/querysharding/analyzer.go
Outdated
@@ -99,6 +99,9 @@ func (a *QueryAnalyzer) Analyze(query string) (QueryAnalysis, error) { | |||
case *parser.BinaryExpr: | |||
if n.VectorMatching != nil { | |||
shardingLabels := without(n.VectorMatching.MatchingLabels, []string{"le"}) | |||
if !n.VectorMatching.On && len(shardingLabels) > 0 { | |||
shardingLabels = append(shardingLabels, "__name__") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: let's use model.MetricNameLabel
.
pkg/querysharding/analyzer_test.go
Outdated
@@ -142,20 +142,20 @@ sum by (container) ( | |||
{ | |||
name: "binary expression with without vector matching and grouping", | |||
expression: `sum without (cluster, pod) (http_requests_total{code="400"}) / ignoring (pod) sum without (cluster, pod) (http_requests_total)`, | |||
shardingLabels: []string{"pod", "cluster"}, | |||
shardingLabels: []string{"pod", "cluster", "__name__"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just updated these
Signed-off-by: Samuel Dufel <[email protected]>
a8b39b5
The |
Fixes #5798
When analyzing a query composed of boolean expression with a non-"on" vector match, the current vertical sharding logic is incorrectly scoping to labels in the vector match and changing the semantics of the query.
Example:
Changes
ignoring
keyword (match On == false)Verification
I am not clear on if it's actually possible to shard
without
labels without breaking queries; it's possible that a more appropriate change would be to the shard matcher code in the querier.