Add Position metadata to the parser by slrtbtfs · Pull Request #6061 · prometheus/prometheus

slrtbtfs · 2019-09-26T12:12:15Z

This aims to implement part one of proposal https://github.com/slrtbtfs/promql-lsp/blob/master/doc/proposals/2019_promql-parser-improvements.md

It introduces position metadata in the error messages and the AST.

Current state:

For error messages, the implementation is done.
~~The AST is able to store position metadata but currently mostly uses zeros as placeholders.~~
The last commit fills some of the position fields with real data. Unfortunately it triggers a data race. Despite staring at the code for quite a while I haven't been able to figure out the root cause of that race, so any help there would be appreciated.

brancz · 2019-09-26T12:18:26Z

We looked at the code a little and couldn't find the data race, maybe someone sees it or can think of a data race that may had been there all along, but only now triggers the race detector.

brancz · 2019-09-26T12:19:11Z

cc @simonpasquier @krasi-georgiev @brian-brazil @juliusv

slrtbtfs · 2019-09-26T12:38:34Z

Output of the race detector: https://gist.github.com/slrtbtfs/359c05c7a5f28c1df3f9d9a1b6be99c6

slrtbtfs · 2019-09-26T12:53:16Z

Data race has been found by Casey and Simon approx. at the same time.

slrtbtfs · 2019-09-27T17:32:35Z

Update: All AST Nodes have Position information now. There are still two edge cases where that the positions are not accurate:

~~NumberLiterals that are the right hand side of a vector matching report positions that are off by one.~~ Edit: The error was in the test case
Offset modifiers are not handled correctly.

slrtbtfs · 2019-10-07T07:44:07Z

The PR should be in a sane state now.

simonpasquier

I'm that familiar with the promql code but here are some comments.

promql/ast.go

promql/engine.go

promql/lex.go

promql/parse.go

promql/printer.go

Deprecate old file location API of the lexer and add new one Add File Option in lexer initialisation Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

ATM they are still populated with zeros Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Offset modifiers are not yet reflected in the syntax tree Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

slrtbtfs · 2019-10-25T17:07:35Z

The merge conflict should be resolved now.

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

slrtbtfs · 2019-10-29T13:32:16Z

Is there any specific reason stopping this from being merged?

cc @simonpasquier @krasi-georgiev @brian-brazil @juliusv

krasi-georgiev · 2019-10-29T14:14:33Z

sorry I don't think I will be able to provide a review here as I am focused on the cert rotation PR in Thanos.

brancz · 2019-10-30T13:22:50Z

lgtm from an implementation side, @juliusv @brian-brazil @beorn7 are you happy with moving forward with this in general?

As a side note we're going to be investigating a generated parser next, our investigation so far have shown that even if we go with that, the overwhelming majority of this PR would still be required (more comprehensive proposal in progress).

juliusv · 2019-10-30T13:28:57Z

Yeah, I haven't checked the implementation in detail, but in general I'm all for it. I'm very much looking forward to PromQL syntax highlighting and autocompletion :)

Will be part of a separate PR Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

beorn7 · 2019-10-30T14:03:29Z

I guess @tomwilkie is way more qualified than I am to comment on this.

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

slrtbtfs · 2019-11-14T17:42:31Z

Here are some benchmarks for this PR.

https://gist.github.com/slrtbtfs/07fec8f8e023e70e50ee220c9bd29ca8

slrtbtfs · 2019-11-14T17:58:42Z

Looks like there is no performance hit. The number of allocations actually seems to have gone slightly down.

slrtbtfs · 2019-11-21T16:41:09Z

The benchmarks above test the whole PromQL engine. Since the parser is only a small part of these workloads, the changes of this PR might get buried in the noise.

I've added another benchmark (see #6355) , that exclusively tests the parser. Here a negative performance impact is visible, but not dramatic. It can be seen that the position logic adds 5 allocations per parser run.

https://gist.github.com/slrtbtfs/e95ffda4b8cd12b77428d57ac2cdb774

brian-brazil · 2019-11-21T16:48:32Z

I'd consider if the changes are lost in the noise, then all is good.

slrtbtfs · 2019-11-25T12:38:58Z

Closed in favour of adding the features of this PR, after the parser has been replaced by a generated one.

slrtbtfs force-pushed the master branch 2 times, most recently from 41c5c60 to 746b49b Compare September 26, 2019 17:19

slrtbtfs mentioned this pull request Sep 27, 2019

promql: Parser bug: Prometheus does not accept spaces after opening brace in subquery. #6065

Closed

slrtbtfs force-pushed the master branch 2 times, most recently from 8458451 to f551356 Compare September 27, 2019 17:25

slrtbtfs force-pushed the master branch from f551356 to 38d04b4 Compare October 7, 2019 07:41

slrtbtfs changed the title ~~WIP: Add Position metadata to the parser~~ Add Position metadata to the parser Oct 7, 2019

simonpasquier reviewed Oct 11, 2019

View reviewed changes

slrtbtfs force-pushed the master branch 3 times, most recently from b5256dd to e402759 Compare October 16, 2019 09:48

slrtbtfs mentioned this pull request Oct 23, 2019

promql: Refactor abstract syntax tree traversion #6205

Closed

slrtbtfs force-pushed the master branch from e402759 to c762471 Compare October 25, 2019 16:50

slrtbtfs added 11 commits October 25, 2019 19:06

Rename old Pos Type to Offset

c810fef

Deprecate old file location API of the lexer and add new one Add File Option in lexer initialisation Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Use go/token for Error Positions

afd887b

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Remove deprecated functions

0b51a4a

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Update tests to expect gcc style positions

203869e

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Add Position Metadata Fields to the AST

586a6e8

ATM they are still populated with zeros Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Introduce Race Condition

3c1b889

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Fix Data race

0f63ff4

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Add Postions to Number and String Literals, improve AST printing

aeb456f

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Fix vendor

21d417c

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Implement position handling of aggregate expressions

b2bbea8

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Add Position metadata to function calls

b9b9651

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

slrtbtfs and others added 10 commits October 25, 2019 19:06

Add Position metadata to Subquery and Matrix Selector Expressions

0140b1f

Offset modifiers are not yet reflected in the syntax tree Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Add Position metadata to VectorSelectors

b677a68

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

parser: Fix positions for offset modifiers

2dcbc67

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Add owner to todos

9f82dd7

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Implement Position handling for ParenExpr

d103f92

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

parser: Fix some tests

201d4e7

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Address the issues pointed out in the PR review

fae4ece

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Fix spelling error

432de4e

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Fix func name in comment

6d0312a

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Fix vendor

b82d590

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

slrtbtfs force-pushed the master branch from 2334748 to b82d590 Compare October 25, 2019 17:06

Empty commit to trigger CI rebuild

ea66c81

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Revert removal of the Expressions type

52b32d5

Will be part of a separate PR Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

Switch functions.go back to old Expressions type

7377f3b

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

slrtbtfs mentioned this pull request Oct 31, 2019

Proposal: Replacing the PromQL Parser with a generated parser #6256

Closed

slrtbtfs closed this Nov 25, 2019

slrtbtfs mentioned this pull request Jan 10, 2020

PromQL: Add position metadata to the AST #6597

Closed

Conversation

slrtbtfs commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brancz commented Sep 26, 2019

Uh oh!

brancz commented Sep 26, 2019

Uh oh!

slrtbtfs commented Sep 26, 2019

Uh oh!

slrtbtfs commented Sep 26, 2019

Uh oh!

slrtbtfs commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slrtbtfs commented Oct 7, 2019

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slrtbtfs commented Oct 25, 2019

Uh oh!

slrtbtfs commented Oct 29, 2019

Uh oh!

krasi-georgiev commented Oct 29, 2019

Uh oh!

brancz commented Oct 30, 2019

Uh oh!

juliusv commented Oct 30, 2019

Uh oh!

beorn7 commented Oct 30, 2019

Uh oh!

slrtbtfs commented Nov 14, 2019

Uh oh!

slrtbtfs commented Nov 14, 2019

Uh oh!

slrtbtfs commented Nov 21, 2019

Uh oh!

brian-brazil commented Nov 21, 2019

Uh oh!

slrtbtfs commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

slrtbtfs commented Sep 26, 2019 •

edited

Loading

slrtbtfs commented Sep 27, 2019 •

edited

Loading