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 ability to specify some results as precomputed in PromQL expression's AST #10101

Open
GiedriusS opened this issue Jan 3, 2022 · 2 comments

Comments

@GiedriusS
Copy link
Contributor

Hello,

On Thanos side, we've been working on trying to fix one of the oldest tickets, query pushdown. Currently how Thanos works is that we always send data i.e. metrics data that matches given matchers via Select(), to a central location before executing the query. This can get quite inefficient. A lot of resources can be saved by executing part of any original query on the leaf nodes instead of sending raw data. For example, we've recently implemented a feature for pushing down max, max_over_time, and other functions: thanos-io/thanos#4917. It works well for these functions because you can apply them multiple times and get the same result. But, the current PromQL engine is not flexible enough for the other case where results are already precomputed and we don't want the PromQL engine to compute them again. So, in theory, we could calculate rate(foo[5m]) on leaf nodes but then the "global" PromQL engine would apply rate again. Thus, I think some improvements need to be made on the PromQL engine's side.

I am not the first one to come up with this. Here is some of the previous work that I am aware of:

Some options to solve this problem that I can think of:

  • Having Select() return some kind of SelectHints which would indicate whether the result has already been computed. Then, the PromQL engine could remove the function call one layer above. Probably the easiest to implement but does this make sense?
  • Separate function for doing what NodeReplacer is doing. This would give the maximum flexibility to downstream users but probably the hardest to implement. Maybe it could even be called that way?
  • Adding the same NodeReplacer to Walk/Inspect.

All of this extra code would only be used by downstream users so all in all it should be relatively easy to add this kind of "internal API" to the PromQL engine.

@brian-brazil
Copy link
Contributor

Probably the easiest to implement but does this make sense?

It doesn't, as leaf nodes don't have access to enough data to correctly evaluate a function like rate() as they don't know what data other leaves may also have for the same time series. It's basically only correct to do this sort of thing for min/max.

@GiedriusS
Copy link
Contributor Author

But things such as rate only modify each matching time series in place, right? Could you please elaborate? One blocker that I can think of is how from the result returned by query_range it is not clear whether there were any gaps in the data so it is impossible to deduplicate correctly without this information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants