Send query plan to querier. #11246

jeschkies · 2023-11-16T14:57:52Z

What this PR does / why we need it:
Following #11123 and in order to enable #10417 the query frontend should send the serialized LogQL AST instead of the query string to the queriers. This enables the frontend to change the AST and inject expressions that are not expressible in LogQL.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

github-actions · 2023-11-16T20:31:18Z

Trivy scan found the following vulnerabilities:

HIGH openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.2-r0. Fixed in v3.1.4-r0
HIGH openssl: Generating excessively long X9.42 DH keys or checking excessively long X9.42 DH keys or parameters may be very slow in libcrypto3 v3.1.2-r0. Fixed in v3.1.4-r1
HIGH openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.2-r0. Fixed in v3.1.4-r0
HIGH openssl: Generating excessively long X9.42 DH keys or checking excessively long X9.42 DH keys or parameters may be very slow in libssl3 v3.1.2-r0. Fixed in v3.1.4-r1

cstyan

a few comments, some bits are still missing from my mental model for how this will work but I'll ask about that somewhere else to not clog up the PR review

cstyan · 2023-11-20T21:24:59Z

pkg/logql/evaluator.go

+// ParamsWithExpressionOverride overrides the query expression so that the query
+// string and the expression can differ. This is useful for sharding etc.


should this comment be useful for query planning when plan my not match externally available logql syntax?

pkg/logql/evaluator.go

cstyan · 2023-11-20T21:35:05Z

pkg/querier/http.go

@@ -73,6 +73,7 @@ func (q *QuerierAPI) RangeQueryHandler(ctx context.Context, req *queryrange.Loki
 		return logqlmodel.Result{}, err
 	}

+	// TODO: fun fact req should implement params. So no wrapper would be required 🤷


is this true? are you saying it should, or it does currently?

Following the chain, req is queryrangebase.Request, which is definitions.Request. That interface doesn't have a function to get the params, but I think it makes sense for it to have one. I think that's what you're suggesting 👍

The logql.Params interface is defined as

type Params interface { Query() string Start() time.Time End() time.Time Step() time.Duration Interval() time.Duration Limit() uint32 Direction() logproto.Direction Shards() []string }

queryrange.LokiRequest is very similar but has GetEndTs instead of End. I'd not change it here but wanted to surface the difference.

cstyan · 2023-11-20T21:42:38Z

pkg/logql/evaluator.go

+	var err error
+	p.queryExpr, err = syntax.ParseExpr(qs)
+	return p, err


does it make sense to keep both the string and parsed expression on this struct?

afaict we either pass the query string to parsing functions like engine.go we call
expr, err := q.parse(ctx, q.params.Query()) in Eval or evaluator.go we call expr, err := syntax.ParseSampleExpr(q.Query()) in Sortable

Other than that we only call Query to get the query string for passing to error logging. And the Expr interface has a fmt.Stringer to support String, so I think we could store just the parsed expression and remove storing the query string.

If we want to keep the string it might be nice to rename the function QueryString, can be confusing at first glance to differentiate between the logql turning Params into a Query that can be Exec'd when we have another function here Query.

Other than that we only call Query to get the query string for passing to error logging.

That's the sole reason to keep it around. The engine should not use the string other than for logging.

If we want to keep the string it might be nice to rename the function QueryString,

That's a good idea.

cstyan · 2023-11-20T22:18:49Z

pkg/logql/engine.go

 	if q.checkBlocked(ctx, tenants) {
 		return nil, logqlmodel.ErrBlocked
 	}

-	switch e := expr.(type) {
+	switch e := q.params.GetExpression().(type) {


should we have the Expr interface also support a Type function? or maybe even via the query struct modify it to save the parsed Expr instead of just having Params and a parse function?

We could but it would be very similar to this type switch.

This is btw the benefit of the change. We don't have to parse the query.

…rsten/send-query-plan

dannykopping

Overall looks fantastic! Very excited to see this all come together.
Marked the remaining TODOs, left one or two nits, and even a couple substantive questions!

pkg/logcli/client/file.go

pkg/logql/blocker.go

pkg/logql/evaluator.go

pkg/logql/test_utils.go

pkg/querier/plan/plan.go

dannykopping · 2023-11-21T13:56:51Z

pkg/querier/queryrange/codec.go

 	return p.GetQuery()
 }

+func (p paramsLabelWrapper) GetExpression() syntax.Expr {


Hhmm, does it make sense for these to even implement the interface?
Seems like we need a parent interface here, and GetExpression(), QueryString() etc can move into a sub-interface of that

I agree. logql.Params is an odd interface. I did try to change it just to find out there a logql.QueryParams that does not really fit the need. Let's see.

Ok, I don't this is feasible in this change. stats is using the params and they must have an expression for the RecordRangeAndInstantQueryMetrics. I can do a follow up and change them so they use their own interface.

Sounds good, it's a non-blocking nit

Yeah lets clean these up in a follow up. Nice that we have a better sense of how all this should fit together as a result of this PR 👍

pkg/querier/queryrange/codec.go

pkg/querier/queryrange/queryrange.proto

jeschkies · 2023-11-21T16:44:20Z

pkg/logql/rangemapper_test.go

-	_, _, err = rvm.Parse(`{app="foo"} |= "err"`)
+	_, _, err = rvm.Parse(syntax.MustParseExpr(`{app="foo"} |= "err"`))
 	require.Error(t, err)
-	_, _, err = rvm.Parse(`topk(0, sum(count_over_time({app="foo"} | json |  __error__="" [15m])))`)
-	require.Error(t, err)
-	// Check fixes for bug where missing or empty parameters for regexp and pattern parsers threw a panic
-	// Missing parameter to regexp parser
-	_, _, err = rvm.Parse(`topk(10,sum by(namespace)(count_over_time({application="nginx", site!="eu-west-1-dev"} |= "/artifactory/" != "api" != "binarystore" | regexp [1d])))`)
-	require.ErrorIs(t, err, logqlmodel.ErrParse)
-	// Empty parameter to regexp parser
-	_, _, err = rvm.Parse(`topk(10,sum by(namespace)(count_over_time({application="nginx", site!="eu-west-1-dev"} |= "/artifactory/" != "api" != "binarystore" | regexp ` + "``" + ` [1d])))`)
-	require.ErrorIs(t, err, logqlmodel.ErrParse)
-	// Empty parameter to pattern parser
-	_, _, err = rvm.Parse(`topk(10,sum by(namespace)(count_over_time({application="nginx", site!="eu-west-1-dev"} |= "/artifactory/" != "api" != "binarystore" | pattern ` + `""` + ` [1d])))`)
-	require.ErrorIs(t, err, logqlmodel.ErrParse)
 	// Empty parameter to json parser
-	_, _, err = rvm.Parse(`topk(10,sum by(namespace)(count_over_time({application="nginx", site!="eu-west-1-dev"} |= "/artifactory/" != "api" != "binarystore" | json [1d])))`)


Since we are using GetExpressions these cases do not apply anymore. Parsing errors are caught when a request is decoded.

dannykopping

Let's go!

Signed-off-by: Callum Styan <[email protected]>

**What this PR does / why we need it**: The recent change #11246 requires that `LokiRequest.Plan` is always set. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](0d4416a)

**What this PR does / why we need it**: Following grafana#11123 and in order to enable grafana#10417 the query frontend should send the serialized LogQL AST instead of the query string to the queriers. This enables the frontend to change the AST and inject expressions that are not expressible in LogQL. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](grafana@0d4416a) --------- Signed-off-by: Callum Styan <[email protected]> Co-authored-by: Callum Styan <[email protected]>

**What this PR does / why we need it**: The recent change grafana#11246 requires that `LokiRequest.Plan` is always set. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](grafana@0d4416a)

jeschkies added 5 commits November 14, 2023 18:13

start defining query plan

b954bc2

Pass parsed query

32a4374

Start fixing tests

4f3b9d8

Parsed AST when not set.

4f4bcc6

Wrap params

cbf3d1c

jeschkies requested review from dannykopping and cstyan November 16, 2023 14:57

pull-request-size bot added the size/L label Nov 16, 2023

jeschkies added 2 commits November 16, 2023 16:10

Rename params mapping

9bb42ae

Embed shards as well

945ecea

pull-request-size bot added size/XL and removed size/L labels Nov 16, 2023

jeschkies added 11 commits November 16, 2023 21:50

Pass downstreamer test

80c6594

Pass split by range

424788d

Pass query

648a134

Set plan in encode decode test

7e7468b

Merge remote-tracking branch 'grafana/main' into karsten/send-query-plan

3cc88cc

Format protos

55d3169

Set plan if it's not set.

f6a0715

Merge remote-tracking branch 'grafana/main' into karsten/send-query-plan

56c5744

Satisfy linter

64d1e2e

Fix downstreamer test

69a5ca2

Merge remote-tracking branch 'grafana/main' into karsten/send-query-plan

be2f785

jeschkies marked this pull request as ready for review November 20, 2023 15:47

jeschkies requested a review from a team as a code owner November 20, 2023 15:47

cstyan reviewed Nov 20, 2023

View reviewed changes

jeschkies added 3 commits November 21, 2023 09:12

Rename Query to QueryString

7eb7056

Merge remote-tracking branch 'grafana/main' into karsten/send-query-plan

e61f864

Merge remote-tracking branch 'origin/karsten/send-query-plan' into ka…

41bc4bb

…rsten/send-query-plan

dannykopping reviewed Nov 21, 2023

View reviewed changes

jeschkies added 5 commits November 21, 2023 16:02

Use AST to determine type.

feefd4b

Remove commented code with TODO

1bb299a

Do no parse AST in split by range

1bacf24

Calculate size with empty writer.

25a9b50

Use append writer

01cfc39

jeschkies requested review from dannykopping and cstyan November 21, 2023 16:32

jeschkies mentioned this pull request Nov 21, 2023

Ensure parsing a serialized LogQL AST results in the same AST. #11227

Closed

8 tasks

jeschkies added 2 commits November 21, 2023 17:42

Remove obsolete test cases.

b371968

Merge remote-tracking branch 'grafana/main' into karsten/send-query-plan

3f44090

jeschkies commented Nov 21, 2023

View reviewed changes

dannykopping approved these changes Nov 21, 2023

View reviewed changes

jeschkies and others added 3 commits November 21, 2023 18:01

Use GetExpression where applicable

16b697a

fix tests that panic because of missing Plan field

353e9c0

Signed-off-by: Callum Styan <[email protected]>

fix another test

21b7c81

Signed-off-by: Callum Styan <[email protected]>

jeschkies merged commit 5b97fcf into grafana:main Nov 22, 2023
7 checks passed

jeschkies deleted the karsten/send-query-plan branch November 22, 2023 07:29

jeschkies mentioned this pull request Nov 22, 2023

Set query plan when copying LokiRequest #11291

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send query plan to querier. #11246

Send query plan to querier. #11246

jeschkies commented Nov 16, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023

cstyan left a comment

cstyan Nov 20, 2023

jeschkies Nov 21, 2023

cstyan Nov 20, 2023

jeschkies Nov 21, 2023

cstyan Nov 20, 2023

cstyan Nov 20, 2023

cstyan Nov 20, 2023

jeschkies Nov 21, 2023

cstyan Nov 20, 2023

jeschkies Nov 21, 2023

dannykopping left a comment

dannykopping Nov 21, 2023

jeschkies Nov 21, 2023 •

edited

Loading

jeschkies Nov 21, 2023

dannykopping Nov 21, 2023

cstyan Nov 21, 2023

jeschkies Nov 21, 2023

dannykopping left a comment

		// ParamsWithExpressionOverride overrides the query expression so that the query
		// string and the expression can differ. This is useful for sharding etc.

Send query plan to querier. #11246

Send query plan to querier. #11246

Conversation

jeschkies commented Nov 16, 2023 • edited Loading

github-actions bot commented Nov 16, 2023

cstyan 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

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

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeschkies Nov 21, 2023 • edited Loading

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

dannykopping left a comment

Choose a reason for hiding this comment

jeschkies commented Nov 16, 2023 •

edited

Loading

jeschkies Nov 21, 2023 •

edited

Loading