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

storage.Querier doesn't handle offsets efficiently #4224

Closed
jacksontj opened this Issue Jun 5, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@jacksontj
Copy link
Contributor

jacksontj commented Jun 5, 2018

Bug Report

I have an implementation of the storage.Querier interface for my storage, with promql on top. In this setup if a user sends a query for a metric at a single point with an offset of 4w the querier created by promql spans 4w. This makes it difficult (to say the least) to create an efficient querier implementation. presumably similar issues exist inside prom itself (since it would potentially load significantly more data than necessary). I currently have 2 fixes for the issue, either of which I'd be glad to merge upstream-- I'm looking for some input on what fix you'd prefer.

Option 1: multiple queriers

If we don't want to change the querier interface at all we can simply make promql/engine.go create a querier for each offset. This does potentially mean more connections etc. depending on what the querier does under the hood, but does avoid the gross over-scanning of data.
Code: jacksontj@4475e2a

Option 2: add offset to selectParams

If we are okay adding a field to selectParams we can simply pass in the offset there on each actual Select. This mechanism allows for the querier to be re-used, and then makes it optional for the querier to actually look at the offset. This method would require Queriers to make code changes to pick it up, but it is backwards compatible (meaning if they don't look at the selectParam field, then they just fall back to current behavior).
Given the interface etc. I think this is the one we should do.
Code: jacksontj@cb72269

Environment

  • Prometheus version:

2.2 (same issue exists in master)

@jacksontj

This comment has been minimized.

Copy link
Contributor Author

jacksontj commented Jun 5, 2018

We are also seeing symptoms from #3450 which may be related (since alerting rules somewhat regularly include offsets as well).

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 5, 2018

Passing the information as a hint sounds fine to me. There's no performance impact to Prometheus with the current implementation.

jacksontj added a commit to jacksontj/prometheus that referenced this issue Jun 5, 2018

Add offset to selectParams
This adds the Offset from the promql.EvalStmt to the selectParams which
is sent to the querier during Select()

Fixes prometheus#4224

@jacksontj jacksontj referenced this issue Jun 5, 2018

Merged

Select param #42

jacksontj added a commit to jacksontj/prometheus that referenced this issue Jun 6, 2018

Create a querier per-offset in promql evaluation
This allows the queriers to be made per-offset, such that the querier
only needs to fetch data related to the specific time range that we
require-- while still maintaining mint/maxt.

Upstream issue: prometheus#4224

jacksontj added a commit to jacksontj/prometheus that referenced this issue Jun 14, 2018

Add Start/End to SelectParams
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes prometheus#4224

jacksontj added a commit to jacksontj/prometheus that referenced this issue Jun 25, 2018

Add Start/End to SelectParams
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes prometheus#4224

jacksontj added a commit to jacksontj/prometheus that referenced this issue Jun 25, 2018

Make remote read use the new selectParams for start/end
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes: prometheus#4224

jacksontj added a commit to jacksontj/prometheus that referenced this issue Aug 14, 2018

Add Start/End to SelectParams
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes prometheus#4224

jacksontj added a commit to jacksontj/prometheus that referenced this issue Aug 14, 2018

Make remote read use the new selectParams for start/end
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes: prometheus#4224
@lock

This comment has been minimized.

Copy link

lock bot commented Mar 22, 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 22, 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.