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

query-frontend: stop using creation_grace_period to limit query length #8075

Merged

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented May 7, 2024

What this PR does

Reviewing #7496 made me realize this.

#3144's reason for truncating queries is to not fill the scheduler queue with effectively useless queries. Some queries can legitimately be querying into the future without there being samples ingested for that time. For example predict_linear used on a longer term scale.

Since the impact of undoing this change in this PR is per-tenant I think it is reasonable - it will only impact the tenant sending the large queries. Perhaps worth noting that -query-frontend.max-total-query-length still applies.

Which issue(s) this PR fixes or relates to

Fixes #5445

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

3144's reason for truncating queries is to not fill the scheduler queue with effectively useless queries. Queries can legitimately be querying into the future without there being samples ingested for that time. For example `predict_linear` used on a longer term scale. Since the impact is per-tenant I think undoing this clamping is reasonable - it will only impact the tenant sending the large queries. Perhaps worth noting that `-query-frontend.max-total-query-length` still applies.

Reviewing 7496 made me realize this.

Fixes 5445

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov
Copy link
Contributor Author

cc @hi-rustin @pracucci @colega since you were involved in the original work

@pracucci
Copy link
Collaborator

pracucci commented May 7, 2024

I'm not convinced removing it, sorry. IIRC, this protection was introduced after an incident and now we're removing that protection. Maybe we can convert it into a new dedicated limit, which is more generous (e.g. 1 day ahead is fine, 100 years ahead is not).

@dimitarvdimitrov
Copy link
Contributor Author

e.g. 1 day ahead is fine, 100 years ahead is not

we still have max-total-query-length which would reject a 100 year query. Why is a query into the future different from a query into the past? The former is much cheaper.

IIRC, this protection was introduced after an incident and now we're removing that protection

I couldn't find the problem this caused. I think you just noticed it happening, but the actual problem was related to mmap and lazy loading.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

I agree with @dimitarvdimitrov's arguments: querying into the future should be cheaper than querying into the past, and still limited by the max query length.

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Ok, your arguments make sense.

@dimitarvdimitrov dimitarvdimitrov merged commit 4d82008 into main May 7, 2024
29 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/query-frontend/stop-using-creation-grace-period branch May 7, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max_query_into_future seems ignored
3 participants