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 MaxTotalQueryLength and fill it from MaxQueryLength if unset #3058

Merged
merged 9 commits into from
Sep 29, 2022

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Sep 27, 2022

What this PR does

Add the ability to define a higher total query length limit on the
frontend , compared to the current shared limit between querier and
query-frontend.

Keep backwards compatibility.

Which issue(s) this PR fixes or relates to

Fixes https://github.com/grafana/mimir-squad/issues/889

Checklist

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

@krajorama krajorama requested a review from a team as a code owner September 27, 2022 12:52
@krajorama krajorama marked this pull request as draft September 27, 2022 12:52
@krajorama krajorama force-pushed the krajo/20220926-query-length branch 2 times, most recently from 5a424d3 to bb951cb Compare September 28, 2022 07:45
@krajorama krajorama marked this pull request as ready for review September 28, 2022 07:45
Copy link
Contributor

@ortuman ortuman left a comment

Choose a reason for hiding this comment

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

nice job! much cleaner now 👏 👏 👏

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/util/validation/limits.go Outdated Show resolved Hide resolved
@pracucci pracucci self-requested a review September 28, 2022 13:28
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.

Thanks for working on this! I definitely see this need (see #2793), but please see my comments before merging. I think there are couple of things to improve / fix.

pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/util/validation/errors_test.go Outdated Show resolved Hide resolved
pkg/util/validation/errors_test.go Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved

Both PromQL instant and range queries can fetch metrics data over a period of time.
A [range query](https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries) requires a `start` and `end` timestamp, so the difference of `end` minus `start` is the time range length of the query.
An [instant query](https://prometheus.io/docs/prometheus/latest/querying/api/#instant-queries) requires a `time` parameter and the query is executed fetching samples at that point in time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The new limit (and so error) applies only to range queries, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes , I quickly tested in a local mimir install. Updating docs one more time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated docs, please re-review the doc parts + changelog, thank you

krajorama and others added 8 commits September 29, 2022 09:46
# This is the 1st commit message:

Add MaxTotalQueryLength and fill it from MaxQueryLength if 0

Add the ability to define a higher total query length limit on the
frontend , compared to the current shared limit between querier and
query-frontend.

Ref: grafana/mimir-squad#889

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

# Conflicts:
#	CHANGELOG.md

# This is the commit message #2:

Update runbook

# This is the commit message #3:

Update pkg/util/validation/limits.go

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>
# This is the commit message #4:

Rename store.max-total-query-length to query-frontend...

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
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.

Great work! LGTM 👏

@krajorama
Copy link
Contributor Author

Installed and tested the final version locally with helm. Merging now.

@krajorama krajorama merged commit cd75d5e into main Sep 29, 2022
@krajorama krajorama deleted the krajo/20220926-query-length branch September 29, 2022 10:40
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.

4 participants