prometheus
WIP: Clean up the Lezer grammar output tree
I have been asked to review the lezer-promql grammar, and noticed that it was creating more nodes and a more complex tree structure than it needed to. The first patch in this PR makes the following changes:
- Removes the
Exprwrapping node around expressions, tagging the (lowercase)exprrule with@isGroup=Exprso that you can still query whether something is an expression. - Replaces the
MetricIdentifier(Identifier)structure with justIdentifier. - Makes the binary op modifiers optional, and splits
BoolModifierfrom ignoring/on modifiers (ModifierClause) for clarity. - Collapses recursive rules (
GroupingLabelList,FunctionCallArgs,LabelMatchList) that just existed to implement a repetition into a simpler rule that uses the*operator and creates a flat tree.
I've adjusted the lezer-promql tests to test for the updated tree shape.
However, after doing this I found out the codemirror-promql package had some code that is very tightly coupled to the way the syntax tree looks. I've tried to adjust this in the second patch, but I was unable to run the test suite, so this is most likely not done. Firstly, the package files for the packages under module/ seem to miss some dev dependencies (jest, rollup) and rely on higher-up packages to also be installed. After doing that, I could start jest, but it still failed to run with messages like...
ReferenceError: exports is not defined
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'. https://github.com/prometheus/prometheus/pull/11333 ... suggesting some kind of module-system confusion. (I'm using Node 16.13.) Would anybody be able to point me in the right direction regarding how to run these tests?
Awesome, thanks! I haven't studied the code changes yet (aim to do that soon), but for the tests, this is what works for me:
$ node --version
v16.16.0
$ npm --version
8.11.0
$ cd web/ui
$ npm i
$ npm test # Run tests for all the web stuff
$ cd module/codemirror-promql
$ npm test # Run tests just for codemirror-promql
Updated with a version that passes the tests. You should probably still review carefully, since some of the code I touched was somewhat opaque to me, not having experience with the system.
Generally looks great to me besides my one node naming comment, thanks for the extensive changes :+1:
@Nexucis are you fine with the codemirror-promql changes around it as well?
Perfect, thanks! I'll merge this in the next few days then, unless @Nexucis has any complaints during that time.
Oops, we're actually getting one linter error now (just some code formatting):
/__w/prometheus/prometheus/web/ui/module/codemirror-promql/src/parser/vector.ts
16:9 error Replace `·And,·BinaryExpr,·MatchingModifierClause,·LabelName,·GroupingLabels,·GroupLeft,·GroupRight,·On,·Or,·Unless·` with `⏎··And,⏎··BinaryExpr,⏎··MatchingModifierClause,⏎··LabelName,⏎··GroupingLabels,⏎··GroupLeft,⏎··GroupRight,⏎··On,⏎··Or,⏎··Unless,⏎` prettier/prettier
✖ 1 problem (1 error, 0 warnings)
1 error and 0 warnings potentially fixable with the `--fix` option.
@marijnh This change breaks Grafana's current usage of this module, we are currently using BinModifiers, OnOrIgnoring, LabelMatchList, MetricIdentifier, Expr, FunctionCallArgs, GroupingLabel, and GroupingLabelList which no longer exist as of this merge. Some usages appear to have trivial replacements, but for others it is not clear to me how we should be migrating to support this change.
The unit tests are helpful for seeing how we should update, but most of the previously exported variables above are not covered.
Are there any tips or documentation you can provide for replacing the usages of these exports that are no longer available?
@gtk-grafana I'm sorry to hear! FWIW, Grafana has started using this module for purposes that it wasn't intended or suited for, and to my knowledge, without consulting with the original developers (myself and @Nexucis) first. Generally I'm not sure if that's a good idea, as lezer trees are optimized towards CodeMirror-like text-editing use cases (optimizing for things like parse error recovery, incremental parsing, etc.), but not as great for doing other high-level interpretation and manipulation of symbolic ASTs, like I understand you're currently using it for in the graphical query builder in Grafana.
See also this excerpt from https://lezer.codemirror.net/docs/guide/:
These trees, represented by data structures from the @lezer/common package, are more limited than the abstract syntax trees you might have seen in other contexts. They are not very abstract. Each node only stores a type, a start and end position, and a flat sequence of children. When writing a grammar, you choose which productions are stored as nodes—the others don't appear in the tree at all.
This means that the tree is very compact in memory and cheap to build. It does make doing refined analysis on it somewhat awkward. The use case guiding the design of this library is an editor system, which keeps a syntax tree of the edited document, and uses it for things like syntax highlighting and smart indentation.
Now given the current reality of Grafana using this (so far, successfully) for the query editor, I'm not sure exactly how to migrate things there, but I hope it's possible of course. But I wanted to clarify that this is not generally a use case we were looking to explicitly support with this package.
To clarify we're using this package for the promQL code editor (using Monaco) in Grafana, AND the graphical query builder (although it is also used for some novel use cases like adding labels to existing queries via ad-hoc filters and other one-off situations), but I understand that your use case for this package is limited to the code mirror package.
We'll try to figure out if we want to fork an older version or see if we can rewrite things in Grafana to work with these changes. Thank you for the prompt response on this!