Refactor/remove global splitby #5243
Conversation
| f.Var(&cfg.ForwardHeaders, "frontend.forward-headers-list", "List of headers forwarded by the query Frontend to downstream querier.") | ||
| cfg.ResultsCacheConfig.RegisterFlags(f) | ||
| } | ||
|
|
||
| // Validate validates the config. | ||
| func (cfg *Config) Validate() error { | ||
| if cfg.SplitQueriesByInterval != 0 { | ||
| return errors.New("The yaml flag `split_queries_by_interval` must now be set in the `limits_config` section instead of the `query_range` config section.") |
There was a problem hiding this comment.
This Validate() method is invoked at the start of the query range module? 🤔
Instead of returning an error, wouldn't it be more appropriate to log a warning and also specify the current default split_queries_by_interval value?
There was a problem hiding this comment.
That's a fair point. I opted to error because it will force the user to acknowledge/understand this, whereas a warning has a fair chance of being ignored/unseen. The downside to this approach is that we're now erroring on a previously valid configuration without a major version bump. @slim-bean @cyriltovena @sandeepsukhani Any thoughts?
There was a problem hiding this comment.
I support this, I know it's not ideal to have a breaking config change in a minor release but the cost to the users IMO is low and the cost to us trying to support multiple flags and deprecated flags over time is high.
However please add a note to the upgrade guide!
There was a problem hiding this comment.
Doesn't sounds cool to do this. We have good default so why don't we let it start and log that it wasn't registered as expected with an error ?
There was a problem hiding this comment.
I agree with @cyriltovena here, I think it would be more user-friendly to start loki and log the message stating that the value being used for this flag is now 30 minutes.
It might go unseen, but at least loki still works and it doesn't represent a breaking change for current users.
|
Alright, cleaned it up a bit further, ensuring we call |
| splitInterval = x | ||
| } | ||
|
|
||
| currentInterval := r.GetStart() / splitInterval | ||
| // include both the currentInterval and the split duration in key to ensure | ||
| // a cache key can't be reused when an interval changes | ||
| return fmt.Sprintf("%s:%s:%d:%d:%d", userID, r.GetQuery(), r.GetStep(), currentInterval, split) |
There was a problem hiding this comment.
We could just set it to zero if the split is zero. That's a bit simpler. If we activate splitting, then the split part of the key will kick off.
Here's my attempt at the infamous splitby refactor 👻