-
Notifications
You must be signed in to change notification settings - Fork 8.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
Adds support for prettifying PromQL expression #10544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work! We just need a little more test coverage and prettify a few more expressions that can get long.
Can you also find a place where we can use it in Prometheus? I am thinking we can add a UI element with a "prettify" button which sends the PromQL to Prometheus, Prometheus sends back the prettified version, and replaces the query field with the prettified version. WDYT @roidelapluie @juliusv? It will make it easier to use this feature and test it extensively as well. (This can be a follow up work an not a part of this PR)
Sounds like a good idea to me. I'd be happy to build the UI side of that after this PR. |
I think prettifying can be implemented in promtool that prettifies a set of rules files, like |
Yes, rules and expr could display prettified PromQL. We could also have a prettify button in the graph page. |
Yup thats the plan! I think for this PR we can just have the prettify implementation. We can do the rule pretty and ui in follow up works to keep this PR simple. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Just nits before we can merge this. Apologies for blocking this for so long.
f44fa4c
to
860aee5
Compare
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit adds .Pretty() for all nodes of PromQL AST. Each .Pretty() prettifies the node it belongs to, and under no circustance, the parent or child node is touch/prettified. Read more in the "Approach" part in `prettier.go`
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit removes redundancy between printer.go and prettier.go by taking out the common code into separate private functions.
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com> This commit does 2 things: 1. It adds support to split function calls that have 1 arg and exceeds the max_characters_per_line to multiple lines. 2. Splits Unary expressions that exceed the max_characters_per_line. This is done by formatting the child node and then removing the prefix indent, which is already applied before the unary operator.
Head branch was pushed to by a user without write access
860aee5
to
b9e0d73
Compare
This uses the formatting functionality introduced in #10544. I've chosen "query" instead of "expr" in both the endpoint and parameter names to stay consistent with the existing API endpoints. Otherwise, I would have preferred to use the term "expr". Signed-off-by: Julius Volz <julius.volz@gmail.com>
* Add /api/v1/format_query API endpoint for formatting queries This uses the formatting functionality introduced in #10544. I've chosen "query" instead of "expr" in both the endpoint and parameter names to stay consistent with the existing API endpoints. Otherwise, I would have preferred to use the term "expr". Signed-off-by: Julius Volz <julius.volz@gmail.com> * Add docs for /api/v1/format_query endpoint Signed-off-by: Julius Volz <julius.volz@gmail.com> * Add note that formatting expressions removes comments Signed-off-by: Julius Volz <julius.volz@gmail.com>
This PR is a simplification of #7779.
This PR uses AST information to format the Node and its children, by passing down the indentation level. Each parent increments the indentation level before calling its child. See
Approach
inprettier.go
for more information.Note: This PR adds prettifying functions only. I plan to make another PR that links this with promtool after this gets merged.