add group_{left,right} to LogQL #4510

lingpeng0314 · 2021-10-21T02:44:24Z

What this PR does / why we need it:
Add group_{left,right} to LogQL, the group_left/group_right modifier allows one-to-many and many-to-one vector matching.

Which issue(s) this PR fixes:
Fixes TODO: Add group_{left,right} to LogQL

Special notes for your reviewer:

Checklist

Documentation added
Tests updated

CLAassistant · 2021-10-21T02:44:29Z

All committers have signed the CLA.

dannykopping · 2021-10-21T06:56:49Z

@lingpeng0314 thanks for this submission! Looks quite exciting.
We'll get this reviewed in the next few days.

chri2547 · 2021-10-25T19:13:42Z

docs/sources/logql/_index.md

@@ -163,6 +163,23 @@ This example will return every machine total count within the last minutes ratio
 sum by(machine) (count_over_time({app="foo"}[1m])) / on() sum(count_over_time({app="foo"}[1m]))
 ```

+### Many-to-one and one-to-many vector matches
+Many-to-one and one-to-many matchings refer to the case where each vector element on the "one"-side can match with multiple elements on the "many"-side. This has to be explicitly requested using the group_left or group_right modifier, where left/right determines which vector has the higher cardinality.


Suggested change

Many-to-one and one-to-many matchings refer to the case where each vector element on the "one"-side can match with multiple elements on the "many"-side. This has to be explicitly requested using the group_left or group_right modifier, where left/right determines which vector has the higher cardinality.

Many-to-one and one-to-many matchings occur when each vector element on the "one"-side can match with multiple elements on the "many"-side. You must explicitly request matching by using the group_left or group_right modifier, where left or right determines which vector has the higher cardinality.

chri2547 · 2021-10-25T19:15:59Z

docs/sources/logql/_index.md

+<vector expr> <bin-op> on(<labels>) group_left(<labels>) <vector expr>
+<vector expr> <bin-op> on(<labels>) group_right(<labels>) <vector expr>
+```
+The label list provided with the group modifier contains additional labels from the "one"-side to be included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.


Suggested change

The label list provided with the group modifier contains additional labels from the "one"-side to be included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.

The label list provided with the group modifier contains additional labels from the "one"-side that are included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.

Clarify the sentence that starts "For on a label..." What does that mean?

<vector expr> <bin-op> on(<labels>) group_right(<labels>) <vector expr>
What i want to say is a label shouldn;t appear in the on label list and group_x label list at the same time because on label as the join key, the value in the lef/right vector is the same. So the label in group_x is redundant.

chri2547 · 2021-10-25T19:22:21Z

docs/sources/logql/_index.md

+<vector expr> <bin-op> on(<labels>) group_right(<labels>) <vector expr>
+```
+The label list provided with the group modifier contains additional labels from the "one"-side to be included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.
+Grouping modifiers can only be used for comparison and arithmetic. Operations as and, unless and or operations match with all possible entries in the right vector by default.


Possible wording below for the sentence starting with "Operations as..."

"By default, the system matches as, unless, and or operations with all entries in the right vector."

chri2547 · 2021-10-25T19:25:53Z

docs/sources/logql/_index.md

+The label list provided with the group modifier contains additional labels from the "one"-side to be included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.
+Grouping modifiers can only be used for comparison and arithmetic. Operations as and, unless and or operations match with all possible entries in the right vector by default.
+
+This example will return sum results for the same app with the many part labels and the labels specified by group_right in the one side.


update wording to:

"The following example returns sum results for the same application with the many part labels and the labels specified by the group_right operation."

chri2547

Provided wording suggestions.

owen-d

Partial review:
This is looking wonderful. Thanks for adding this feature set and fixing some existing bugs for us. I had to play around in Prometheus to compare/validate some of this behavior :) and came to the conclusion that you're right!

Looking forward to reading the rest of the PR and thanks a ton for the contribution!

owen-d · 2021-10-27T13:40:00Z

pkg/logql/engine_test.go

-			promql.Vector{
-				promql.Sample{Point: promql.Point{T: 60 * 1000, V: 0}, Metric: labels.Labels{labels.Label{Name: "app", Value: "foo"}}},
-			},
+			promql.Vector{},


Thanks for the fix!

owen-d · 2021-10-27T13:40:05Z

pkg/logql/engine_test.go

-			promql.Vector{
-				promql.Sample{Point: promql.Point{T: 60 * 1000, V: 0}, Metric: labels.Labels{}},
-			},
+			promql.Vector{},


Thanks for the fix!

owen-d · 2021-10-27T14:20:04Z

pkg/logql/engine_test.go

@@ -649,7 +645,24 @@ func TestEngine_LogsInstantQuery(t *testing.T) {
 				{&logproto.SampleQueryRequest{Start: time.Unix(0, 0), End: time.Unix(60, 0), Selector: `sum by (app) (count_over_time({app="foo"}[1m]))`}},
 			},
 			promql.Vector{
-				promql.Sample{Point: promql.Point{T: 60 * 1000, V: 120}, Metric: labels.Labels{labels.Label{Name: "app", Value: "foo"}, labels.Label{Name: "machine", Value: "fuzz"}}},
+				promql.Sample{Point: promql.Point{T: 60 * 1000, V: 120}, Metric: labels.Labels{}},


owen-d · 2021-10-27T14:28:47Z

pkg/logql/engine_test.go

+			errors.New("multiple matches for labels: many-to-one matching must be explicit (group_left/group_right)"),
+		},
+		{
+			`sum by (app,machine) (count_over_time({app="foo"}[1m])) > bool on () group_left (app) sum by (app) (count_over_time({app="foo"}[1m]))`,


We can safely remove the (app) here from group_left because it's already ensured via the sum by (app,machine) grouping on the left hand side of this binary operation.

Suggested change

`sum by (app,machine) (count_over_time({app="foo"}[1m])) > bool on () group_left (app) sum by (app) (count_over_time({app="foo"}[1m]))`,

`sum by (app,machine) (count_over_time({app="foo"}[1m])) > bool on () group_left sum by (app) (count_over_time({app="foo"}[1m]))`,

You are correct, removed it.

lingpeng0314 · 2021-10-28T01:03:06Z

Partial review: This is looking wonderful. Thanks for adding this feature set and fixing some existing bugs for us. I had to play around in Prometheus to compare/validate some of this behavior :) and came to the conclusion that you're right!

Looking forward to reading the rest of the PR and thanks a ton for the contribution!

Thanks for your review and validation!
Actually when i implement the feature, i found the unit tests were broken so i went to prometheus to double confirm the behavior and finally decide to keep consistent with prometheus, which seems more meaningful.

chri2547

LGTM!

owen-d · 2021-10-28T16:56:41Z

@lingpeng0314 I've opened a PR into your fork (lingpeng0314#1) which removes the CardManyToMany, which we don't actually support. I think this makes the code a little cleaner. I opted to make the PR mysefl as I found that a bit easier than trying to explain it in PR review - I hope that's ok :) .

Edit: finished up the review - great work! If you're willing to merge the PR I linked, I'll approve this PR and we can merge.

removes CardManyToMany

lingpeng0314 · 2021-10-29T03:05:42Z

@lingpeng0314 I've opened a PR into your fork (lingpeng0314#1) which removes the CardManyToMany, which we don't actually support. I think this makes the code a little cleaner. I opted to make the PR mysefl as I found that a bit easier than trying to explain it in PR review - I hope that's ok :) .

Edit: finished up the review - great work! If you're willing to merge the PR I linked, I'll approve this PR and we can merge.

Merged the PR, thanks a lot!

owen-d

Great work, thanks!

lingpeng0314 requested review from KMiller-Grafana and a team as code owners October 21, 2021 02:44

pull-request-size bot added the size/XXL label Oct 21, 2021

lingpeng0314 force-pushed the group_left_right branch 2 times, most recently from 6437ba4 to 8cfe7fd Compare October 21, 2021 04:35

lingpeng0314 marked this pull request as draft October 21, 2021 04:42

lingpeng0314 force-pushed the group_left_right branch from 8cfe7fd to f357113 Compare October 21, 2021 05:21

Add group_left/group_right support

e6cf282

lingpeng0314 force-pushed the group_left_right branch from f357113 to e6cf282 Compare October 21, 2021 05:40

lingpeng0314 marked this pull request as ready for review October 21, 2021 05:54

dannykopping added the component/logql label Oct 21, 2021

GrafanaWriter assigned chri2547 Oct 25, 2021

chri2547 self-requested a review October 25, 2021 19:05

chri2547 reviewed Oct 25, 2021

View reviewed changes

chri2547 requested changes Oct 25, 2021

View reviewed changes

owen-d reviewed Oct 27, 2021

View reviewed changes

lingpeng0314 added 2 commits October 28, 2021 10:03

Update docs/sources/logql/_index.md

a3e2379

Minor change for a test case

60b5550

lingpeng0314 requested review from dannykopping and slim-bean as code owners October 28, 2021 02:15

lingpeng0314 requested a review from chri2547 October 28, 2021 02:36

chri2547 approved these changes Oct 28, 2021

View reviewed changes

removes CardManyToMany

b6419dd

owen-d and others added 2 commits October 28, 2021 13:56

removes now-unused IsSetOperator helper

22c6d04

Merge pull request #1 from owen-d/pull/4510

96a6e98

removes CardManyToMany

owen-d approved these changes Oct 29, 2021

View reviewed changes

owen-d merged commit 76e2a14 into grafana:main Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add group_{left,right} to LogQL #4510

add group_{left,right} to LogQL #4510

lingpeng0314 commented Oct 21, 2021 •

edited

Loading

CLAassistant commented Oct 21, 2021 •

edited

Loading

dannykopping commented Oct 21, 2021

chri2547 Oct 25, 2021

lingpeng0314 Oct 28, 2021

chri2547 Oct 25, 2021

chri2547 Oct 25, 2021 •

edited

Loading

lingpeng0314 Oct 28, 2021 •

edited

Loading

lingpeng0314 Oct 28, 2021

chri2547 Oct 25, 2021 •

edited

Loading

lingpeng0314 Oct 28, 2021

chri2547 Oct 25, 2021 •

edited

Loading

lingpeng0314 Oct 28, 2021

chri2547 left a comment

owen-d left a comment

owen-d Oct 27, 2021

owen-d Oct 27, 2021

owen-d Oct 27, 2021

owen-d Oct 27, 2021

lingpeng0314 Oct 28, 2021

lingpeng0314 commented Oct 28, 2021

chri2547 left a comment

owen-d commented Oct 28, 2021 •

edited

Loading

lingpeng0314 commented Oct 29, 2021

owen-d left a comment

	Many-to-one and one-to-many matchings refer to the case where each vector element on the "one"-side can match with multiple elements on the "many"-side. This has to be explicitly requested using the group_left or group_right modifier, where left/right determines which vector has the higher cardinality.
	Many-to-one and one-to-many matchings occur when each vector element on the "one"-side can match with multiple elements on the "many"-side. You must explicitly request matching by using the group_left or group_right modifier, where left or right determines which vector has the higher cardinality.

	The label list provided with the group modifier contains additional labels from the "one"-side to be included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.
	The label list provided with the group modifier contains additional labels from the "one"-side that are included in the result metrics. For on a label can only appear in one of the lists. Every time series of the result vector must be uniquely identifiable.

	`sum by (app,machine) (count_over_time({app="foo"}[1m])) > bool on () group_left (app) sum by (app) (count_over_time({app="foo"}[1m]))`,
	`sum by (app,machine) (count_over_time({app="foo"}[1m])) > bool on () group_left sum by (app) (count_over_time({app="foo"}[1m]))`,

add group_{left,right} to LogQL #4510

add group_{left,right} to LogQL #4510

Conversation

lingpeng0314 commented Oct 21, 2021 • edited Loading

CLAassistant commented Oct 21, 2021 • edited Loading

dannykopping commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chri2547 Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

lingpeng0314 Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chri2547 Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chri2547 Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chri2547 left a comment

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lingpeng0314 commented Oct 28, 2021

chri2547 left a comment

Choose a reason for hiding this comment

owen-d commented Oct 28, 2021 • edited Loading

lingpeng0314 commented Oct 29, 2021

owen-d left a comment

Choose a reason for hiding this comment

lingpeng0314 commented Oct 21, 2021 •

edited

Loading

CLAassistant commented Oct 21, 2021 •

edited

Loading

chri2547 Oct 25, 2021 •

edited

Loading

lingpeng0314 Oct 28, 2021 •

edited

Loading

chri2547 Oct 25, 2021 •

edited

Loading

chri2547 Oct 25, 2021 •

edited

Loading

owen-d commented Oct 28, 2021 •

edited

Loading