-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
React UI: Implement more sophisticated autocomplete #6160
Comments
I'm currently in the process of building that language server. It lives at https://github.com/slrtbtfs/promql-lsp. The features you named are all intended to land there. Support for completing metric and function names will probably be implemented in the coming two weeks. Other features include:
The part that has to be done on the React UI side to support the language server would be to provide a language client. A good candidate for that seems to be the monaco editor which is also used by vscode and is surprisingly fast and responsive when included in a web UI (See https://microsoft.github.io/monaco-editor/playground.html). cc @squat |
@slrtbtfs Wonderful! Really looking forward to this :) Do you know if you'll have capacity to work on the React UI integration as well? |
It would make sense to do so. Especially as there probably is a lot of overlap with the PromQL vscode extension (https://github.com/slrtbtfs/vscode-prometheus). I'll discuss this with my managers. |
@slrtbtfs One point to consider: Monaco is not exactly small. Even just the minified |
@juliusv Progress on the language server side is tracked here https://github.com/slrtbtfs/promql-lsp/issues?utf8=%E2%9C%93&q=+is%3Aissue+label%3A%22React+UI%22+ Edit: And here: https://github.com/slrtbtfs/promql-lsp/blob/master/doc/progress.md |
@juliusv An possible alternative might be the CodeMirror editor (400kb unminified, 250kb minified). Someone has started building a language client for that one: https://github.com/wylieconlon/lsp-editor-adapter . Although the development of the lsp client seems to have stalled, it already supports the features we need for our use case, according to https://langserver.org/ . It also looks small enough that we can fix issues with it ourself, if needed. Edit: Add minfied size |
Ah cool, 250kb is getting into the realm of being less painful (although it's still quite big, I guess for what we want to do, that's a reasonable size). |
As per https://bundlephobia.com/result?p=codemirror@5.49.2 is even less - 165kb minified |
Hi, With my collegue @celian-garcia, we made a library to provide PromQL in Monaco. It is quite closed to the one provided in Grafana. Recently I presented it to @juliusv and he asked me if we implemented a real PromQL parser/grammar. So no, we didn't and on my point of view I don't think it's really worth it to implement it on the client side. Autocompletion and syntax highlight should be enough, at least for the Prometheus Console. Concerning this library, we'll try to provide the PromQL language as a native language in Monaco, we are waiting a response from Microsoft about how you can make the autocompletion when it is a native language. (It's not super crystal clear for us :)) (microsoft/monaco-editor#1672) Depending of what LSP can provide in the Json, but I'm thinking that we could make a similar configuration in CodeMirror (same way we did for Monaco). @juliusv I hope I didn't forget any particular point and thanks for your patient :) |
Hi, good to know you've been working on a similar thing.
One advantage of using the language server protocol is, that the language server can be written in any language. In this case it is written in go and uses the same parser as prometheus.
Not having to implement the same features multiple times for different editors is actually the point of the language server protocol. The LSP allows implementing these features once and then using them with basically any editor, including vim, emacs, monaco, codemirror, eclipse, ...
Completion and syntax highlighting are not the only useful features one can provide for language support. Consider e.g. showing errors and documentation. See e.g. https://imgur.com/a/Lwv8G2e Also note that Completion is far less trivial than it looks at first.
Another important point to consider is, that PromQL queries often occur as part of yaml files. For a useful editor integration this case should be supported, too. The PromQL language server already does this. |
Thanks for the input, both of you! @slrtbtfs That makes sense for the case of complex autocompletion at least, yeah. Just curious, do LSP clients cache certain completion information in a way that e.g. function / operator names can be autocompleted without constant roundtrips to the server, or does basically every keystroke lead to an LSP request? For syntax highlighting, it seems there's no LSP support for it yet (microsoft/language-server-protocol#682 (comment)), and it is is generally considered to be better done on the client-side (for speed / reliability reasons anyway)? So I guess the syntax highlighting portion of what @Nexucis did here would be applicable for client-side UI integration (if ported to whatever editor we choose to use)? |
Usually there is a limit how many completions the server offers for a single request (by default 100). If the returned list is complete, i.e. there are less than 100 available completions, the server can tell this to the client and the client won't ask for additional completions again
Yes, that would be reusable. I've actually already done something similar for the vscode extension for prometheus, too. The main difference seems to be that @Nexucis provides the syntax highlighting using some typescript and I use a textmate grammar. The advantage of the textmate grammar is, that it can be used by some other tools than vscode, e.g. eclipse and atom. The advantage of the typescript approach is, that it might allow slightly better highlighting. |
The server can additionally tell the client to only ask for completions, if the cursor is over a word separator. This way no completions will be asked for if the cursor is somewhere in the middle of a word. |
Thanks for the background! So I guess for completion we will just wait for server-side LSP support then, but need to decide what to do for highlighting in the meantime. That in turn seems gated on the editor decision. If anyone interested wants to give CodeMirror a deeper look in terms of usability, quality, and LSP support (for our use case), that seems like the next step. |
Thanks for your feedback about the LSP feature @slrtbtfs . Futur is bright with it :). |
@Nexucis Sounds good, especially since I'm not that good with front end stuff. In case it is useful, here is a TextMate grammar for PromQL. At least for VS-Code it needs to be converted to JSON before using it. The language server isn't able to talk over a network, yet. I'll fix this soon, to make experiments with Code Mirror possible. |
Thank you @slrtbtfs @juliusv for all the inputs ! I'm sure it will help us ;) We'll keep you informed ;) |
@juliusv @celian-garcia @Nexucis The language server is now in a state where you can try it out locally. It even has some limited support for auto completion. Support for remote clients is not yet there, however. |
@slrtbtfs uuuh so cool ! Thanks. On my side, I didn't advance so much on the topic unfortunately :(. The promCon gave me many things to think and to say to my team :). |
Some aspects of a typical LSP implementation have become apparent that are a problem for Prometheus. Firstly communication is typically done via JSON-RPC over WebSocket, which is not at all like our current APIs. Secondly the LSP needs to maintain state for each active client (https://microsoft.github.io/language-server-protocol/overview) of at least the document (query) it's working on. This is both a complexity and reliability risk. I think any such functionality and communication needs to happen internal to the frontend, and any communication to the Prometheus server itself is done via our standard APIs. |
The idea of having a language server is kind of the opposite of having it in the frontend. While I agree with some of the concerns, there are actually good reasons to not implement such features on the client side:
For more complex setups like Thanos or Grafana it would be also possible to run the language server in a sidecar container which might reduce these risks. |
That's fine when it's running on a developer's desktop or dedicated SaaS. Not when it's living inside a key process of a critical monitoring system.
I'm not worried about the parser, I'm worried about the other parts. Namely the architecture that implicitly comes with an LSP.
I don't think that solves the problem. I'm proposing to sidestep the problem by having the usual LSP client<-> server communication happen entirely inside the browser when it's a Prometheus type use case where the primary purpose of the binary in question is monitoring. |
Creating a stateless API that uses the same logic as the PromQL language server is something that should be actually possible from a implementation side. I have some doubts about whether it's a good idea though, since that would essentially mean creating a new protocol. |
Hello guys :) And regarding the "lsp" integration I'm thinking that maybe it will be nice if the CodeMirror mode has a "offline" mode. Like that people that cannot have access to a Prometheus (like for example because of Cross Origin issue) can still have a PromQL editor. Do you think it will be interesting ? |
Nice!
I would like that. Will raise it with the others.
Yeah, definitely LSP-based autocompletion needs to be optional for cases where you don't use it directly from a Prometheus server. |
The PromQL language server and the PromQL language server REST API both can work without a Prometheus server to get data from. Syntax error checks, function name completion and function documentation still work in that case. The main issue here is that some host to run the language server would be required. Alternatively it should be also possible to run the language server in a browser using Web Assembly. However that would require downloading the langserver wasm binary, which is about 4mb gzipped. |
Good to know! I think in the normal Prometheus server we can always require the full backend support, as the Prometheus UI needs to be able to reach its API anyway. But editors might then use just the language server without a Prometheus server. Not sure if there's a good-enough use case for even pulling the language server itself into the UI using wasm (especially at 4MB size). |
it can be "ok" if you still have access to internet I guess, which is not always the case. |
It might be that tinygo will be able to compile the language server somewhen in the future, which might significantly reduce binary sizes. |
Hello Guys, So just after saying I was finished the syntax highlight, I thought it would be nice for the "offline" mode to highlight when the syntax is not good. So now I have a light grammar of promQL and I'm able to hightlight some error. Not all of them are hightlighted but it's a start (and the error shown is super ugly). I also have the autocompletion ready :) Here some kind of error that are not covered : 100 > 100 --> doesn't return an error. I did it on purpose and not sure it will be easy to make it better
100>my_tric --> it's not parsed. The parser is waiting a space to analyze it
100 > group_left 15 ---> doesn't return an error. Same problem as 100 > 100. Grammar too subversive
1 - 100 ) --> doesn't return an error. A bit weird, no idea why You can try it by yourself using this playground: https://nexucis.github.io/codemirror-mode-promql/ cheers! PS: @juliusv any news regarding if I should put it in a neutral zone ? |
@Nexucis Thanks, doing well, hope you're ok too! Oh wow, interesting! Are the current limitations of your client-side error display ones that run into fundamental problems with the CodeMirror grammar support, or is it just not "done" yet? Would be really cool to get 100% reliable client-side syntax error markings of course! Btw. not sure if you were aware of it, but we changed PromQL back to a generated parser (from a custom-built one), so now this file basically has the full grammar definition for the language: https://github.com/prometheus/prometheus/blob/master/promql/parser/generated_parser.y
I just filed prometheus-community/community#19 and prometheus-community/community#20. I had previously asked on Chat, but I think it got ignored, so thanks for reminding me. |
@juliusv I'm ok too :) thanks ! I saw the yacc file and actually I was searching for a lib that would generate the typescript/javascript code thanks to the yacc file. But I didn't find it :(. Regarding the current limitations, I'm not sure.
Because last time I was trying it, the 2nd rule didn't work because it said to me it expects a scalar. But I'm agree with you, it would be super cool to cover 100% of all possible syntax. Finally regarding the ugly error the codeMirror is currently returning when you are writting a wrong expression, I think it is just I don't handle all functionalities provided by the lib codeMirror-grammar
Oh cool, thanks a lot ! |
@Nexucis So in Prometheus the grammar itself initially allows any expression type (vector/matrix/string/scalar) around binary operators ( prometheus/promql/parser/generated_parser.y Lines 228 to 244 in 44ad28d
prometheus/promql/parser/parse.go Lines 510 to 515 in fac7a4a
There are a bunch of post-parsing checks like that that would be infeasible to all do directly in the lexer + parser, so unless we port the entire parser + checker to JS manually it seems infeasible to support all error cases client-side with just a grammar file. But maybe that's fine for a beginning. |
ah ok! Thanks for telling me it. I was wondering if I had forgotten all my knowledge about how to write a grammar because it was so irritating to fail on that :D. So yes in that case, it won't be feasible to check this kind of error with the current implementation. I just wrote the grammar and I don't have the hand on the lexer + parser. So I cannot intercept the result and analyze it :(. It would be actually super nice to be able to generate the parser and lexer thanks to the yacc file. But that would be an entire but super interesting project by itself. |
Hello, Looking at the PR #6872, it seems the functionality needed are not yet ready. Is it something that will be unlocked soon @slrtbtfs ? Do you need some help on it ? |
The main blocker currently is prometheus-community/promql-langserver#134 . I currently don't have the capacity to work on implementing this myself but if someone is willing to work on this, I'll happily help with any questions that arise and review PRs. So help on this would be appreciated.
In what context? For using it in VS Code there already exists a VS Code extension. Unfortunately publishing it to the VS Code Marketplace is blocked on prometheus-community/vscode-promql#29 . |
Ok I will take a look at it then :).
For using it in codeMirror |
If you're ok with having a language server binary running on some host you can use https://godoc.org/github.com/prometheus-community/promql-langserver/rest . (Using the master branch). If you're ok with downloading about 5mb of compressed wasm code, you should be able run the language server inside the browser. However this requires a small patch in a dependency.
Great! |
I actually recently implemented an Earley parser for @polarsignals using https://nearley.js.org/ I feel like this might be a better fit than the full-blown language server. Kubernetes SIG instrumentation's |
hello @brancz, interesting lib indeed :). I'm sorry for the lack of update on this topic. Actually with @juliusv, we implemented a complete offline parser using lezer+ codemirror v6. The promql grammar using lezer is available here and the lib that is using it, is available here. I think codemirror-promql is going to be integrated into Prometheus around Marsh. So quite soon, the Prometheus console will be deeply reviewed :D. |
and also for the followup and maybe for some clarifications. Here is the last conclusion regarding LSP: #6872 (comment) |
Very cool! Any reason why we haven't integrated this yet? |
@brancz My plan is to do it in March! |
It would be great to have more sophisticated expression field autocompletion in the new React UI.
Currently it only autocompletes metric names, and only when the expression field doesn't contain any other sub-expressions yet.
Things that would be nice to autocomplete:
For autocomplete functionality not to annoy users, it needs to be as highly performant, correct, and unobtrusive as possible. Grafana does many things right here already, but they also have a few really annoying bugs, like inserting closing parentheses in incorrect locations of an expression.
Currently @slrtbtfs has indicated interest in building a language-server-based autocomplete implementation.
The text was updated successfully, but these errors were encountered: