Added -querier.max-query-lookback and fixed -querier.max-query-into-future #3452
Conversation
| @@ -204,18 +205,20 @@ func NewQueryable(distributor QueryableWithFilter, stores []QueryableWithFilter, | |||
| return storage.QueryableFunc(func(ctx context.Context, mint, maxt int64) (storage.Querier, error) { | |||
| now := time.Now() | |||
|
|
|||
| if cfg.MaxQueryIntoFuture > 0 { | |||
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?
| @@ -74,7 +74,7 @@ func (cfg *StoreConfig) RegisterFlags(f *flag.FlagSet) { | |||
| cfg.WriteDedupeCacheConfig.RegisterFlagsWithPrefix("store.index-cache-write.", "Cache config for index entry writing. ", f) | |||
|
|
|||
| f.Var(&cfg.CacheLookupsOlderThan, "store.cache-lookups-older-than", "Cache index entries older than this period. 0 to disable.") | |||
| f.Var(&cfg.MaxLookBackPeriod, "store.max-look-back-period", "Limit how long back data can be queried") | |||
| f.Var(&cfg.MaxLookBackPeriod, "store.max-look-back-period", "Deprecated: use -querier.max-query-lookback instead. Limit how long back data can be queried. This setting applies to chunks storage only.") // To be removed in Cortex 1.8. | |||
| } | |||
|
|
|||
| // Validate validates the store config. | |||
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")
| @@ -204,18 +205,20 @@ func NewQueryable(distributor QueryableWithFilter, stores []QueryableWithFilter, | |||
| return storage.QueryableFunc(func(ctx context.Context, mint, maxt int64) (storage.Querier, error) { | |||
| now := time.Now() | |||
|
|
|||
| if cfg.MaxQueryIntoFuture > 0 { | |||
There was a problem hiding this comment.
Is this because Select uses hints?
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
add-querier-max-lookback
branch
from
020c19a to
3e304f3
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
| 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 <marco@pracucci.com>
|
Thanks @pstibrany for your thoughtful review. I've addressed your comments. |
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]