Move cortex' querier split_queries_by_interval to limits config #5184

ssncferreira · 2022-01-19T14:11:38Z

What this PR does / why we need it:

Move split_queries_by_interval config from cortex' queryrange package to limits config.

Special notes for your reviewer:

Checklist

Documentation updated
Tests updated
Add an entry in the CHANGELOG.md about the changes.

ssncferreira · 2022-01-20T14:49:34Z

pkg/querier/queryrange/queryrangebase/roundtrip.go

@@ -52,6 +52,7 @@ var (

 // Config for query_range middleware chain.
 type Config struct {
+	// Deprecated: SplitQueriesByInterval will be removed in the next major release


How do we keep track of the flags that will be deprecated on follow-up releases? Should one issue be created to remind us of this flag? 🤔

Do we do any logging around use of deprecated fields?

I think we have flagext.DeprecatedFlag( from dskit log a warning. So worth looking into that.

@cyriltovena set the flag with flagext.DeprecatedFlag on commit c99dd51

If we move the same flag to the limits struct, we don't need to deprecate it.

Addressed in commit: d35fcf3

trevorwhitney

Looks great! My only question is why deprecate the old flag and introduce a new one vs changing where the flag is registered into limits.go? Seems less disruptive to me to do the latter since it achieves the same outcome.

pkg/querier/queryrange/queryrangebase/roundtrip.go

pkg/validation/limits.go

owen-d

Looking good, but left some suggestions.

owen-d · 2022-01-26T16:06:42Z

pkg/querier/queryrange/limits.go

-		res.splitDuration = conf.SplitQueriesByInterval
-	}
+	// Set as the default split by interval value
+	res.splitDuration = l.QuerySplitDurationDefault()


We should be able to remove res.splitDuration now, since it'll be automatically covered in the underlying Limits impl. Previously we needed to support tenant override > tenant default > global default, but we've removed global default and we already handle the first two in the underlying implementation: https://github.com/grafana/loki/blob/main/pkg/validation/limits.go#L530-L538

Oh yeah, +1 to this as well.

@owen-d I don't think we can remove res.splitDuration because of function WithSplitByLimits:

loki/pkg/querier/queryrange/limits.go

Lines 82 to 87 in d35fcf3

func WithSplitByLimits(l Limits, splitBy time.Duration) Limits {

return limits{

Limits: l,

splitDuration: splitBy,

}

}

used to force a 24-hour split in the query range middleware of the NewSeriesTripperware and NewLabelsTripperware

pkg/validation/limits.go

pkg/querier/queryrange/queryrangebase/roundtrip.go

ssncferreira · 2022-01-26T19:37:21Z

pkg/querier/queryrange/limits.go

 	if conf.SplitQueriesByInterval != 0 {
-		res.splitDuration = conf.SplitQueriesByInterval
+		level.Warn(util_log.Logger).Log("deprecated", "yaml flag 'query_range.split_queries_by_interval' is deprecated, use yaml flag 'limits_config.split_queries_by_interval' or CLI flag -querier.split-queries-by-interval instead.",
+			"default split_queries_by_interval", l.QuerySplitDurationDefault())


For users currently using the config file, setting res.splitDuration = conf.SplitQueriesByInterval has no effect here. Since limits now contain a default value (30 minutes), this will always be returned, meaning clause:

loki/pkg/querier/queryrange/limits.go

Lines 54 to 56 in 87cbe50

if dur == 0 {

return l.splitDuration

}

will never be reached.

Therefore, for users currently using the config file (while the flag is not removed), I see two options:

global default value of 30 minutes is overwritten by the value of conf.SplitQueriesByInterval

Solution here implemented: simply log a warning and use the default value of 30 minutes

What do you think?

ssncferreira · 2022-01-27T18:29:50Z

Closing in favor of #5243

pull-request-size bot added the size/M label Jan 19, 2022

ssncferreira force-pushed the deprecate_querier_split_queries_by_interval branch from 2a5cf06 to 305e520 Compare January 20, 2022 14:34

ssncferreira changed the title ~~Deprecate querier split_queries_by_interval~~ Deprecate cortex' querier split_queries_by_interval Jan 20, 2022

ssncferreira self-assigned this Jan 20, 2022

ssncferreira marked this pull request as ready for review January 20, 2022 14:42

ssncferreira requested review from KMiller-Grafana and a team as code owners January 20, 2022 14:42

ssncferreira commented Jan 20, 2022

View reviewed changes

ssncferreira force-pushed the deprecate_querier_split_queries_by_interval branch 3 times, most recently from 195a515 to 161f8b3 Compare January 26, 2022 12:07

pull-request-size bot added size/L and removed size/M labels Jan 26, 2022

ssncferreira mentioned this pull request Jan 26, 2022

Move -querier.split-queries-by-interval into limits config #5233

Closed

3 tasks

trevorwhitney reviewed Jan 26, 2022

View reviewed changes

pkg/querier/queryrange/queryrangebase/roundtrip.go Outdated Show resolved Hide resolved

pkg/validation/limits.go Outdated Show resolved Hide resolved

owen-d reviewed Jan 26, 2022

View reviewed changes

ssncferreira commented Jan 26, 2022

View reviewed changes

ssncferreira changed the title ~~Deprecate cortex' querier split_queries_by_interval~~ Move cortex' querier split_queries_by_interval to limits config Jan 26, 2022

ssncferreira force-pushed the deprecate_querier_split_queries_by_interval branch from faa9a0e to 411a5ff Compare January 26, 2022 20:22

ssncferreira added 9 commits January 26, 2022 20:38

Deprecate querier split_queries_by_interval

719cd0e

Set querier.split-queries-by-interval as deprecated flag

94c551a

Fix linter

2bcc7aa

Remove queryrange throwaway flag set

f461764

Add QueryRange register flags

8f8b508

Move querier.split-queries-by-interval to limits config

21011ab

Fix tests and linter

141487d

Fix linter

9098a49

Fix deprecation lint staticcheck.

8399387

ssncferreira force-pushed the deprecate_querier_split_queries_by_interval branch from 411a5ff to 8399387 Compare January 26, 2022 20:38

ssncferreira closed this Jan 27, 2022

ssncferreira deleted the deprecate_querier_split_queries_by_interval branch January 27, 2022 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move cortex' querier split_queries_by_interval to limits config #5184

Move cortex' querier split_queries_by_interval to limits config #5184

ssncferreira commented Jan 19, 2022 •

edited

Loading

ssncferreira Jan 20, 2022

trevorwhitney Jan 21, 2022

cyriltovena Jan 24, 2022 •

edited

Loading

ssncferreira Jan 26, 2022

owen-d Jan 26, 2022

ssncferreira Jan 26, 2022

trevorwhitney left a comment •

edited

Loading

owen-d left a comment

owen-d Jan 26, 2022

trevorwhitney Jan 26, 2022

ssncferreira Jan 26, 2022 •

edited

Loading

ssncferreira Jan 26, 2022

ssncferreira commented Jan 27, 2022

	func WithSplitByLimits(l Limits, splitBy time.Duration) Limits {
	return limits{
	Limits: l,
	splitDuration: splitBy,
	}
	}

Move cortex' querier split_queries_by_interval to limits config #5184

Move cortex' querier split_queries_by_interval to limits config #5184

Conversation

ssncferreira commented Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyriltovena Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevorwhitney left a comment • edited Loading

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssncferreira Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssncferreira commented Jan 27, 2022

ssncferreira commented Jan 19, 2022 •

edited

Loading

cyriltovena Jan 24, 2022 •

edited

Loading

trevorwhitney left a comment •

edited

Loading

ssncferreira Jan 26, 2022 •

edited

Loading