Skip to content
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

Add /api/v1/format_query API endpoint for formatting queries #11036

Merged
merged 3 commits into from Jul 20, 2022

Conversation

juliusv
Copy link
Member

@juliusv juliusv commented Jul 19, 2022

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

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>
@juliusv
Copy link
Member Author

juliusv commented Jul 19, 2022

@Harkishen-Singh

@juliusv juliusv requested a review from codesome July 19, 2022 12:15
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Copy link
Member

@Nexucis Nexucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice !

Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Should we mention in the docs that any comments in the query will be removed in the prettified query?

@roidelapluie
Copy link
Member

Are they removed? I would expect them to make the query "invalid"

Signed-off-by: Julius Volz <julius.volz@gmail.com>
@juliusv
Copy link
Member Author

juliusv commented Jul 19, 2022

They are removed because the parser doesn't even remember comments or tell you that it encountered any. It just throws them away currently. So there'd also not be a way to mark such an expression invalid, but I also would not have expected that as a user.

I added a comment that the comments are being removed.

@Nexucis
Copy link
Member

Nexucis commented Jul 19, 2022

It could be cool to have the prettifier in the graph page too

@@ -206,6 +206,35 @@ $ curl 'http://localhost:9090/api/v1/query_range?query=up&start=2015-07-01T20:10
}
```

## Formatting query expressions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be nice to add that "prettified expressions will follow the specifications as mentioned here"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would skip it for now because not all things in that spec is user digestible. For example max_characters_per_line. It is more of a developer reference. I am not sure if we need to have a user facing spec for this (for now. If we do in future, we need to improve it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(In a user facing doc, max_characters_per_line will be replaced with some actual number for example)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed with @codesome here.

@codesome
Copy link
Member

We tried to preserve comments in the first attempt, by totally bypassing the parser. But that turned out to be a way too complex and fragile of a code. So this is a step 1, a simpler implementation with the trade-off of comments to get the ground moving.

@codesome
Copy link
Member

It could be cool to have the prettifier in the graph page too

We are doing it, right? 👀

@juliusv
Copy link
Member Author

juliusv commented Jul 20, 2022

It could be cool to have the prettifier in the graph page too

Of course! That's the next step :)

@juliusv
Copy link
Member Author

juliusv commented Jul 20, 2022

Will merge if nobody complains in a day or so :)

@juliusv
Copy link
Member Author

juliusv commented Jul 20, 2022

Actually, will merge now so I can build on top of it better.

@juliusv juliusv merged commit b57deb6 into main Jul 20, 2022
@juliusv juliusv deleted the feature/format-query-api-endpoint branch July 20, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants