Serialize query AST to JSON. #11123

jeschkies · 2023-11-02T16:01:10Z

What this PR does / why we need it:
This introduces the visitor pattern to serialize the LogQL AST to JSON. We've chose this pattern because it will be more flexible one the AST is encoded into Protobuf and an actual queryplan.

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

jeschkies · 2023-11-03T12:31:14Z

pkg/logql/syntax/walk.go

+	VisitLogfmtParser(*LogfmtParserExpr)
+}
+
+func Dispatch(root Expr, v RootVisitor) error {


This follows https://www.lihaoyi.com/post/ZeroOverheadTreeProcessingwiththeVisitorPattern.html. We could also add Accept(Visitor) to all expression like the Prettify method. This would avoid pattern matches.

can you elaborate? I'm not sure what this means

dannykopping

Looks great! I think we'll need to flesh out the tests a bit more before we land this though.

I also see changes to vendor without related changes to go.*?

dannykopping · 2023-11-11T04:53:56Z

pkg/logql/serialize_test.go

+
+			fmt.Println(buf.String())


Suggested change

fmt.Println(buf.String())

pkg/logql/syntax/ast.go

jeschkies · 2023-11-13T10:40:58Z

I think we'll need to flesh out the tests a bit more before we land this though.

For sure. The idea would be to check in some sanitized queries we see.

pkg/logql/syntax/parser_test.go

dannykopping

This is looking really slick!
Added a couple nits, great work @jeschkies

dannykopping · 2023-11-14T13:54:06Z

pkg/logql/syntax/ast.go

@@ -694,10 +733,16 @@ func (e *KeepLabelsExpr) String() string {

 func (e *KeepLabelsExpr) Walk(f WalkFn) { f(e) }

+func (e *KeepLabelsExpr) Accept(v RootVisitor) { v.VisitKeekLabel(e) }


Suggested change

func (e *KeepLabelsExpr) Accept(v RootVisitor) { v.VisitKeekLabel(e) }

func (e *KeepLabelsExpr) Accept(v RootVisitor) { v.VisitKeepLabel(e) }

pkg/logql/syntax/parser_test.go

dannykopping · 2023-11-14T13:57:59Z

pkg/logql/syntax/serialize.go

+		return decodeVector(iter)
+	case "label_replace":
+		return decodeLabelReplace(iter)
+	case "log_selector":


Can we make these constants, please?

I've missed this comment. We'll follow up.

dannykopping · 2023-11-14T14:03:33Z

pkg/logql/syntax/serialize_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestJSONSerializationRoundTrip(t *testing.T) {


Do we have a reasonable amount of coverage with these tests?

These are 153 test cases

> go test -v -run TestJSONSerializationParseTestCases ./pkg/logql/syntax/... | rg PASS | sort | uniq | wc -l 152

they are the cases for the parser. That said there might be some edge cases. I was planning to take some actual queries, sanitize them and the add them. If it's ok I'll do this afterwards as I need the visitor for the sanitizing.

I meant actual test coverage of the code paths introduced

Thanks. I actually did find two errors #11230

cstyan · 2023-11-15T01:15:00Z

pkg/logql/log/ip.go

@@ -78,39 +78,41 @@ func (f *IPLineFilter) filterTy(line []byte, ty labels.MatchType) bool {

 type IPLabelFilter struct {
 	ip *ipFilter
-	ty LabelFilterType
+	Ty LabelFilterType


does uppercase Type conflict with a built in or something?

cstyan · 2023-11-15T01:15:47Z

pkg/logql/log/label_filter.go

+//
+//sumtype:decl
 type LabelFilterer interface {
 	Stage
 	fmt.Stringer
+
+	// Seal trait
+	isLabelFilterer()


what are the comments here?

pkg/logql/syntax/ast.go

cstyan · 2023-11-15T01:18:13Z

pkg/logql/syntax/serialize.go

+		return decodeVector(iter)
+	case "label_replace":
+		return decodeLabelReplace(iter)
+	case "log_selector":


cstyan · 2023-11-15T01:19:15Z

pkg/logql/syntax/serialize.go

+	v.WriteObjectField("op")
+	v.WriteString(e.Op)
+
+	v.WriteMore()


what does WriteMore do?

cstyan · 2023-11-15T01:24:19Z

pkg/logql/syntax/walk.go

+	VisitLogfmtParser(*LogfmtParserExpr)
+}
+
+func Dispatch(root Expr, v RootVisitor) error {


can you elaborate? I'm not sure what this means

**What this PR does / why we need it**: This is a follow up to #11123 and fixes a few bugs discovered by increasing the test coverage. **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 #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`](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) --------- Signed-off-by: Callum Styan <[email protected]> Co-authored-by: Callum Styan <[email protected]>

**What this PR does / why we need it**: This introduces the visitor pattern to serialize the LogQL AST to JSON. We've chose this pattern because it will be more flexible one the AST is encoded into Protobuf and an actual queryplan. **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)

**What this PR does / why we need it**: This is a follow up to grafana#11123 and fixes a few bugs discovered by increasing the test coverage. **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)

**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]>

Define visitors.

71d0c3f

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

jeschkies added 5 commits November 2, 2023 17:05

Define root visitor.

e97a987

Start JSON serialization

ad733c7

Implement all visitors

2011b3f

Test serializer

3f48d8d

Decode AST

aab668b

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

jeschkies added 2 commits November 3, 2023 13:29

Add simple range aggregation test case

3726041

Revert changes to vendor

28f2828

jeschkies commented Nov 3, 2023

View reviewed changes

jeschkies added 6 commits November 10, 2023 17:11

Merge branch 'main' into karsten/query-plan-json

d3142cc

Start serializing log range

95d4706

Encode LogRange as serialized LogQL string

be9f6fb

define log_selecotr and log_range

1c1f65b

Call visit explicitly.

9894050

Serialize unwrap.

9261f84

dannykopping reviewed Nov 11, 2023

View reviewed changes

jeschkies added 6 commits November 13, 2023 12:26

Remove extra ","

cfa48c9

Read complete object

144e43f

Add more test cases

155cfb0

Test multiple filters

5df1b29

Support bin opts

0ac5e17

Use parse test cases for serialization.

1ee5ea1

pull-request-size bot added size/XXL and removed size/XL labels Nov 13, 2023

jeschkies added 2 commits November 13, 2023 14:25

Remove leading ,

de5c4b2

Fix leading comma again

cc2246c

jeschkies added 8 commits November 13, 2023 16:13

Fix broken test case

c3ad706

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

fc6e792

Start with post filters

0923606

Seal label filterers

75b73bb

Start encoding

f74a465

serialize filters

3d79f2d

Skip roundtrip test

e03d42c

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

c3f5af0

jeschkies marked this pull request as ready for review November 14, 2023 09:31

jeschkies requested a review from a team as a code owner November 14, 2023 09:31

Revert changes to vendor

8e5d82a

jeschkies requested a review from dannykopping November 14, 2023 09:36

jeschkies commented Nov 14, 2023

View reviewed changes

pkg/logql/syntax/parser_test.go Show resolved Hide resolved

jeschkies added 2 commits November 14, 2023 13:12

Satisfy linter

f2220de

Correct receiver name

ff9fe4d

dannykopping approved these changes Nov 14, 2023

View reviewed changes

jeschkies added 2 commits November 14, 2023 15:52

Correct VisitKeepLabel typo

881759d

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

7468583

jeschkies merged commit 3a7b5d2 into grafana:main Nov 14, 2023
6 of 7 checks passed

jeschkies deleted the karsten/query-plan-json branch November 14, 2023 15:17

cstyan reviewed Nov 15, 2023

View reviewed changes

jeschkies mentioned this pull request Nov 15, 2023

Increase test coverage for AST serialization to >94%. #11230

Merged

8 tasks

jeschkies mentioned this pull request Nov 20, 2023

Send query plan to querier. #11246

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize query AST to JSON. #11123

Serialize query AST to JSON. #11123

jeschkies commented Nov 2, 2023 •

edited

Loading

jeschkies Nov 3, 2023

cstyan Nov 15, 2023

dannykopping left a comment

dannykopping Nov 11, 2023

jeschkies commented Nov 13, 2023

dannykopping left a comment

dannykopping Nov 14, 2023

dannykopping Nov 14, 2023

cstyan Nov 15, 2023

jeschkies Nov 15, 2023

dannykopping Nov 14, 2023

jeschkies Nov 14, 2023

dannykopping Nov 14, 2023

jeschkies Nov 15, 2023

cstyan Nov 15, 2023

cstyan Nov 15, 2023

cstyan Nov 15, 2023

cstyan Nov 15, 2023

cstyan Nov 15, 2023

		@@ -694,10 +733,16 @@ func (e *KeepLabelsExpr) String() string {

		func (e *KeepLabelsExpr) Walk(f WalkFn) { f(e) }

		func (e *KeepLabelsExpr) Accept(v RootVisitor) { v.VisitKeekLabel(e) }

Serialize query AST to JSON. #11123

Serialize query AST to JSON. #11123

Conversation

jeschkies commented Nov 2, 2023 • edited Loading

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 commented Nov 13, 2023

dannykopping 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeschkies commented Nov 2, 2023 •

edited

Loading