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

Remote read ReadResponse should allow for multiple queries #2574

Closed
brian-brazil opened this Issue Apr 4, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@brian-brazil
Copy link
Member

brian-brazil commented Apr 4, 2017

The remote read proto currently allows for multiple queries to be made, in line with future expectations to bundle queries together.

However the response only allows for a single vector. We should add another level of list on top of what we have.

It'd be best to do this before 1.6.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Apr 4, 2017

But multiple queries are fine to end up in the same range vector, no? The multiple queries are there so that we can request multiple label selector sets with different time ranges and all, but all we need as a result in the end is a list of all the time series with all their data points (which we have now).

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Apr 4, 2017

But multiple queries are fine to end up in the same range vector, no?

Consider the query a + b. We need the a's and the b's separately unless you want to separate them out on the Prometheus end. Downsampling optimisations/tweaks may also require offering different data for the same metric depending on context.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Apr 4, 2017

So I guess you mean that in the end at https://sourcegraph.com/github.com/prometheus/prometheus@5f3327f620e3fd29bfc3770acd68d679d165d49c/-/blob/promql/engine.go#L584:1-591:1, it should only get the iterators for that (sub)query and not all the others. Ok yeah.

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Apr 4, 2017

For now we just need to adjust the proto, so this is possible in the future.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Apr 4, 2017

👍

brian-brazil added a commit that referenced this issue Apr 6, 2017

@juliusv juliusv closed this in #2588 Apr 6, 2017

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.