Added -querier.max-query-lookback and fixed -querier.max-query-into-future #3452
Conversation
pkg/querier/querier.go
Outdated
There was a problem hiding this comment.
Moved to validateQueryTimeRange(), which is also called in the Select(). If this limit is not enforced in the Select() too, we're not really enforcing it. Unit tests were weak: I spotted this issue improving unit tests.
There was a problem hiding this comment.
Is this because Select uses hints?
pkg/chunk/chunk_store.go
Outdated
There was a problem hiding this comment.
Can we warn users if this is set in validate?
flagext.DeprecatedFlagsUsed.Inc()
level.Warn(logger).Log("msg", "running with DEPRECATED ..... use ... instead")
pkg/querier/querier.go
Outdated
There was a problem hiding this comment.
Is this because Select uses hints?
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
add-querier-max-lookback
branch
from
020c19a to
3e304f3
Compare
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
| if endTime.Before(startTime) { | ||
| return 0, 0, errEmptyTimeRange | ||
| } |
There was a problem hiding this comment.
This check is repeated in both if conditions. Should we do it only once, after all manipulations?
There was a problem hiding this comment.
I think it's more future proof doing it in both places. Doesn't look an overcomplication.
There was a problem hiding this comment.
I think it's more future proof doing it in both places
I don't quite understand what you mean by "future proof" here. (It's not overcomplication, but looks like unnecessary duplication. Plus it would cover case when input start time is already after end time).
There was a problem hiding this comment.
As of today, we can remove the if and move it to the end because the two validations are done one on start time and the other one on end time. If in the future we add more validation (either on start or end), we may end up in bugs which we don't realise because of an invalid time range (start > end) introduced by a previous validation.
There was a problem hiding this comment.
I see, although I think this check is pretty universal, and hopefully unit tests will warn us about newly introduced bugs. Thanks for explaining your thinking.
Signed-off-by: Marco Pracucci <[email protected]>
|
Thanks @pstibrany for your thoughtful review. I've addressed your comments. |
4 participants
What this PR does:
Currently, the only option to limit the time range of queries is
-store.max-look-back-period, but it suffers the following issues:In this PR I'm adding
-querier.max-query-lookbackwhich is expected is superseed-store.max-look-back-periodsolving the issues above. While working on tests, I've also noticed that-querier.max-query-into-futureis currently broken (doesn't really enforce it), so I've fixed it in this PR.Out of the scope of this PR:
Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]