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 a flag to adjust queries to some max future time #10547

Merged
merged 6 commits into from
Feb 6, 2025

Conversation

chencs
Copy link
Contributor

@chencs chencs commented Jan 30, 2025

What this PR does

This PR introduces an experimental flag, query-frontend.adjust-to-max-future-query-window. When set with a duration, any range queries querying farther than now + maxFutureWindow into the future will be adjusted to query only up to now + maxFutureWindow.

Which issue(s) this PR fixes or relates to

Fixes #

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.

Copy link
Contributor

github-actions bot commented Jan 30, 2025

💻 Deploy preview deleted.

@chencs chencs marked this pull request as ready for review January 30, 2025 22:18
@chencs chencs requested review from tacole02 and a team as code owners January 30, 2025 22:18
narqo

This comment was marked as resolved.

@chencs chencs force-pushed the casie/add-future-query-window-flag branch from c233984 to 4d6b45d Compare February 4, 2025 19:38
@chencs chencs force-pushed the casie/add-future-query-window-flag branch from a121739 to c7331c7 Compare February 4, 2025 19:48
@chencs
Copy link
Contributor Author

chencs commented Feb 4, 2025

I think, in the current form, the new limit won't touch queries that were modified by the max-lookback limit — because the latter adjusts the query and moves on, before the new one takes place. Is this what we want for this PR?

I'm not sure what you mean by this. IIUC queries which fully query outside of the allowed max-lookback will not be executed, but requests which are only modified by max-lookback will continue on to be checked against other limits:

r, err = r.WithStartEnd(minStartTime, r.GetEnd())

Copy link
Contributor

@tacole02 tacole02 left a comment

Choose a reason for hiding this comment

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

Thank you, @chencs !

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -3625,6 +3625,11 @@ The `limits` block configures default and per-tenant limits imposed by component
# optimize their performance.
[instant_queries_with_subquery_spin_off: <list of strings> | default = ]

# (experimental) Mutate incoming queries that look far into the future to only
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use a word other than mutate? Update maybe?

Copy link
Contributor Author

@chencs chencs Feb 5, 2025

Choose a reason for hiding this comment

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

I was following the pattern of the query-frontend.align-queries-with-step description. Is "mutate" an ineffective word choice? This does automatically change the actual query being executed, as opposed to, e.g., being "updated" at the behest of the user executing the query.

Copy link
Contributor

Choose a reason for hiding this comment

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

If mutate is the technically accurate word choice, we can leave it. It just sounds somewhat awkward/jarring to me.

Copy link
Contributor

@narqo narqo left a comment

Choose a reason for hiding this comment

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

queries which fully query outside of the allowed max-lookback will not be executed, but requests which are only modified by max-lookback will continue on to be checked against other limits

You are right (I read the surrounding code wrong).

🔥 The changes looks good to me (except the comments about the changelog)

CHANGELOG.md Outdated Show resolved Hide resolved
@chencs chencs merged commit 741644e into main Feb 6, 2025
30 checks passed
@chencs chencs deleted the casie/add-future-query-window-flag branch February 6, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants