-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
EXPLAIN & EXPLAIN ANALYZE PromQL features #5911
Comments
👍 I'm all up for it just 2 thoughts:
|
The URL parameter makes sense. I think For output, I think having a For |
Separate field is easy to add, we can do that, what's the argument against warnings? Wrong purpose?
+1
I would love that, yes. Perhaps |
I think wrong purpose and to avoid cases, where explain/analyze output and actual warning from query, might clash? We can show both in the same way in UI somehow, but I'd really like to have separate boxes (for eg blue warn, green info). WDYT? 🙂
+1 |
Updated proposal to have |
Love this idea and the suggestions, I think a URL parameter is the way to go. As I assume this is mostly useful for distributed PromQLs, it would be overkill to change the parser for this (and probably would not be easily accepted upstream). Given that |
Hey folks 👋 - A lot of exciting work going on here. Is there anywhere I can read/learn more about the new PromQL engine? |
@PradyumnaKrishna has recently added support for:
Now we are working on adding support for exposing Explain() through the UI: #6346 It looks so good! And it's a solid foundation for future work. Some next ideas:
|
Integration of Promlens would be also awesome which is also doing some performance analysis. |
Hi,
Given the higher complexity of the new PromQL engine we introduce and the fact we are building a framework of optimizers, we would like to have better visibility on how query was optimized and performed. For this, I propose two additional commands/parameters to the
query
andquery_range
APIs:1. Explain
Instead of execution, dry run the query planning, and print the BEFORE and AFTER optimization execution plans.
On PromCon and KubeCon, we demonstrated this as the PromQL keyword:
Dev branch: #5822
I think we can iterate over the text representation and what it shows, but I would love to have an agreement on the API side of things.
2. Analyze
In this mode, I would propose to execute the query and print a similar explanation as explained, just with profile-like information about the operators involved towards important numbers like accumulated latency of each operator's step, processed samples and series. Similar to MySQL's https://dev.mysql.com/blog-archive/mysql-explain-analyze/.
Also similar to PromLens a little bit, just more execution plan-centric, not PromQL statement centric (this is because logical and physical optimizers might optimize PRomQL transparently for efficiency).
Implementation
Input
In my demo implementation, the code looks for the prefix
explain
before parsing PromQL. I think it's easy to type and play with, but it has many issues. It conflicts with a metric called "explain" (probably not a big issue), and generally, none for tools like formatters and linters supports it (etc.), so we would need to change in PromQL language itself - this would be hard as new PromQL is not yet heavily adopted, and it might be too much to change everyone's PromQL parsers.Instead I would propose an extra API parameter as we have for other Thanos special things (https://thanos.io/tip/components/query.md/#query-api-overview). e.g.
mode=explain
for explain mode so info about optimizations and physical planmode=analyze
for analyze (profiling) for physical planmode=explain&mode=analyze
forexplain analyze
so profiling and information about optimizations together.Later on we could add
explain_formatting
parameters for changing formatting etc..Output
In my demo I used
Warnings
API in both UI and backend to send formatted string. It works great with current UIs, and it might even work with Grafana at some point (there is some Errors/Warning API). It is hacky however, so ideally we could have dedicated response type in JSON.However, I believe using Warnings at the start it's not too bad - it's easy to develop new response API in the future. I would propose sticking to that for now.After good comments, let's propose adding a new field
explain
in response JSON, to not conflict withwarnings
.Thoughts/ideas? 🤗
The text was updated successfully, but these errors were encountered: