Dynamic splitting by interval for range queries #6458

afhassan · 2024-12-24T18:43:11Z

What this PR does:
Cortex supports only using a static interval to split range queries. This PR adds two new configs that dynamically adjust the split interval to a multiple of the configured split_queries_by_interval depending on the given query.

New configs:
1 - max_shards_per_query
Accepts an int value for the total number of shards for a query. The split interval is increased into a multiple of split_queries_by_interval to ensure that the total number of shards remains below the configured value. This takes into account vertical sharding if it is configured.

Examples:
split_queries_by_interval = 24h
max_shards_per_query = 30

A 30 day range query is split to 30 shards using 24h interval.
A 40 day range query is split to 20 shards using 48h interval.
A 100 day range query is split to 25 shard using 96h interval.

2 - max_duration_of_data_fetched_from_storage_per_query
Accepts a duration for the total duration of data fetched by all shards of a query. Certain queries can fetch a long duration of data per each shard when executing. This configuration uses a multiple of split_queries_by_interval to reduce the number of shards so that the total duration of data fetched remains below the configured value.

Examples:
split_queries_by_interval = 24h
max_duration_of_data_fetched_from_storage_per_query = 2400h // 100 days

A query up with 30 day range is split into 30 shards using 24h interval
Each shard fetches 1 day of data for a total of 30 days
A query up[10d] with 30 day range is split into 6 shards using 120h interval.
Each shard fetches [5 + 10] days of data for a total of 90 days.
If the query was split into 30 shards using 24h default interval.
Each shard would fetch [1 + 10] days of data for a total of 330 days.

Checklist

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

Signed-off-by: Ahmed Hassan <[email protected]>

harry671003 · 2024-12-27T03:46:46Z

pkg/querier/tripperware/queryrange/query_range_middlewares.go

-		staticIntervalFn := func(_ tripperware.Request) time.Duration { return cfg.SplitQueriesByInterval }
-		queryRangeMiddleware = append(queryRangeMiddleware, tripperware.InstrumentMiddleware("split_by_interval", metrics), SplitByIntervalMiddleware(staticIntervalFn, limits, prometheusCodec, registerer))
+		intervalFn := func(_ tripperware.Request) time.Duration { return cfg.SplitQueriesByInterval }
+		if cfg.SplitQueriesByIntervalMaxSplits != 0 {


Shouldn't the limit be applied to both range splits and vertical spits?

cortex/pkg/querier/tripperware/shard_by.go

Line 40 in 8a46d20

func (s shardBy) Do(ctx context.Context, r Request) (Response, error) {

Technically this sets a limit for the total range and vertical splits for a given query. The number of vertical shards is static, so the max number of of splits for a given query becomes split_queries_by_interval_max_splits x query_vertical_shard_size. Because of this adding a separate limit for vertical sharding when the number of vertical shards is a static config would be redundant because we limit it already.

Signed-off-by: Ahmed Hassan <[email protected]>

pkg/querier/tripperware/queryrange/split_by_interval.go

yeya24 · 2024-12-31T19:55:30Z

Instead of changing split interval using max number of split queries, can we try to combine it with estimated data to fetch?

For example, a query up[30d] is very expensive to split to 30 splits as each split query still fetches 30 day of data so 30 splits ended up fetching 900 days of data.

Instead of having a limit of total splits should we use total days of data to fetch?

afhassan · 2024-12-31T23:59:47Z

Instead of changing split interval using max number of split queries, can we try to combine it with estimated data to fetch?

For example, a query up[30d] is very expensive to split to 30 splits as each split query still fetches 30 day of data so 30 splits ended up fetching 900 days of data.

Instead of having a limit of total splits should we use total days of data to fetch?

That's a good idea - I can add a new limit for total hours of data fetched and adjust the interval to not exceed it.

We can still keep max number of splits since it gives more flexibility to limit the number of shards for queries with long day range even if they don't fetch a lot of days of data like the example you mentioned

Signed-off-by: Ahmed Hassan <[email protected]>

docs/configuration/config-file-reference.md

pkg/querier/querier.go

pkg/querier/tripperware/queryrange/query_range_middlewares.go

pkg/querier/tripperware/queryrange/split_by_interval.go

afhassan · 2025-01-22T20:20:12Z

pkg/querier/tripperware/queryrange/split_by_interval.go

 	if err != nil {
 		return nil, httpgrpc.Errorf(http.StatusBadRequest, err.Error())
 	}
 	s.splitByCounter.Add(float64(len(reqs)))

+	stats := querier_stats.FromContext(ctx)


Yes it is only used to log the interval size used for splitting the query.

pkg/querier/tripperware/queryrange/split_by_interval.go

pkg/util/time.go

Signed-off-by: Ahmed Hassan <[email protected]>

yeya24 · 2025-01-20T09:38:57Z

I get the idea. But my main concern for such dynamic split interval + max splits by interval is that results cache will have very bad hit ratio as our current results cache key is tied to your split interval.

A 30 day range query is split to 30 queries using 24h interval
A 40 day range query is split to 20 queries using 48h interval

The first 30 day range query uses 24h interval so 24h will be part of our results cache key.
Now you run another 40 day range query with 48h interval, the results cache of the first 30 days will be missing as you are using 48h in your results cache key now.

Making vertical shard size dynamic seems more friendly to results cache because vertical shard size is not part of the results cache key. However, not all queries can be vertically sharded.

harry671003 · 2025-01-20T15:46:10Z

The first 30 day range query uses 24h interval so 24h will be part of our results cache key.
Now you run another 40 day range query with 48h interval, the results cache of the first 30 days will be missing as you are using 48h in your results cache key now.

Isn't this true today with Grafana modifying the step interval? For example the 30d query will have a step of 900s vs a 40d query will have a step of 1200s. Since the step is also in the cache key, this will already invalidate the cache.

I agree with you on changing the vertical shard size first. Could we mark this feature experimental and iterate on it?

yeya24 · 2025-01-21T18:35:07Z

Yeah let's mark it experimental in https://cortexmetrics.io/docs/configuration/v1guarantees/#experimental-features

Signed-off-by: Ahmed Hassan <[email protected]>

docs/configuration/config-file-reference.md

pkg/querier/tripperware/queryrange/dynamic_query_splits.go

harry671003 · 2025-01-23T16:57:43Z

pkg/querier/tripperware/queryrange/split_by_interval.go

+
+// calculates the total duration of data the query will have to fetch from storage as a multiple of baseInterval.
+// also returns the total time range fetched by the original query start and end times
+func durationFetchedByQuery(expr parser.Expr, req tripperware.Request, queryStoreAfter, lookbackDelta time.Duration, baseInterval time.Duration, now time.Time) (durationFetchedCount int, originalRangeCount int, lookbackDeltaCount int) {


Nit: Follow the convention for adding comments.

https://tip.golang.org/doc/comment

I rewrote the comment to be more clear. The convention seems to be general guidelines not a specific format but I tried to follow it as much as possible.

pkg/querier/tripperware/queryrange/split_by_interval.go

harry671003 · 2025-01-23T17:07:46Z

pkg/querier/tripperware/queryrange/split_by_interval_test.go

@@ -408,3 +413,189 @@ func Test_evaluateAtModifier(t *testing.T) {
 		})
 	}
 }
+
+func TestDynamicIntervalFn(t *testing.T) {


Could you also add separate tests for durationFetchedByQuery()?

Also, could you add more tests for split_by_interval with dynamic splits enabled? Maybe in TestSplitByDay()

I added tests for both and also added tests for getIntervalFromMaxSplits()

harry671003 · 2025-01-23T17:11:59Z

pkg/querier/tripperware/queryrange/split_by_interval.go

+	durationFetchedCount = 0
+	originalRangeCount = 0
+	lookbackDeltaCount = 0
+	baseIntervalMillis := util.DurationMilliseconds(baseInterval)


Is there a reason why the func returns as multiples of baseInterval rather than just a duration?
Since the function is called durationFetchedByQuery, I'd expect it to return a duration

The calculations done with these are all based on integers and rounding down is important for the result. I refactored the function to be more readable and changed its name. Let me know if you think we should make more changes to it.

pkg/querier/tripperware/queryrange/split_by_interval.go

Signed-off-by: Ahmed Hassan <[email protected]>

afhassan · 2025-01-28T00:43:03Z

Thanks @harry671003 for the all the feedback!
I think the PR is in a good place for a final review now

harry671003

Thanks for addressing the comments. LGTM

Signed-off-by: Ahmed Hassan <[email protected]>

docs/configuration/v1-guarantees.md

Signed-off-by: Ahmed Hassan <[email protected]>

yeya24 · 2025-01-28T23:20:40Z

pkg/querier/tripperware/queryrange/split_by_interval.go

+			n++
+		}
+	}
+	return n * baseInterval


This code is a bit confusing to me. I added a new test case in Test_getIntervalFromMaxSplits below. Since the total time range is only 23h so I expect it to be split by 1 day. But the result was split by 2 days. Please take a look

{ name: "23h with 10 max splits, expected to split by 1 day", baseSplitInterval: day, req: &tripperware.PrometheusRequest{ Start: 12 * 3600 * seconds, End: 35 * 3600 * seconds, Step: 5 * 60 * seconds, Query: "foo", }, maxSplits: 10, expectedInterval: 1 * day, },

yeya24 · 2025-01-28T23:21:46Z

pkg/querier/tripperware/queryrange/split_by_interval.go

+			extraIntervalsPerSplit = 1 // avoid division by 0
+		}
+
+		// Next analyze the query using the next split start time to find the additional duration fetched by lookbackDelta for other subsequent splits


I wonder if we can change some of those code below into dedicated functions. It is very hard to review now due to the complexity

add limit for range query max splits by interval

b874e4e

Signed-off-by: Ahmed Hassan <[email protected]>

pull-request-size bot added the size/S label Dec 24, 2024

harry671003 reviewed Dec 27, 2024

View reviewed changes

Change dynamic interval sharding to take into account vertical sharding

6106978

Signed-off-by: Ahmed Hassan <[email protected]>

pull-request-size bot added size/M and removed size/S labels Dec 31, 2024

afhassan commented Dec 31, 2024

View reviewed changes

pkg/querier/tripperware/queryrange/split_by_interval.go Outdated Show resolved Hide resolved

add dynamic sharding based on total days of data fetched for query

01c121c

Signed-off-by: Ahmed Hassan <[email protected]>

pull-request-size bot added size/L and removed size/M labels Jan 16, 2025

harry671003 reviewed Jan 17, 2025

View reviewed changes

add unit tests for dynamicIntervalFn

b15dde6

Signed-off-by: Ahmed Hassan <[email protected]>

allow using any base interval size for dynamicIntervalFn

a1e7a3a

Signed-off-by: Ahmed Hassan <[email protected]>

pull-request-size bot added size/XL and removed size/L labels Jan 22, 2025

add dynamic query splits to experimental features

08ec706

Signed-off-by: Ahmed Hassan <[email protected]>

afhassan marked this pull request as ready for review January 23, 2025 00:38

dosubot bot added the component/querier label Jan 23, 2025

rename dynamicIntervalFn unit tests

7349cfa

Signed-off-by: Ahmed Hassan <[email protected]>

harry671003 reviewed Jan 24, 2025

View reviewed changes

afhassan added 2 commits January 24, 2025 13:32

refactor dynamicIntervalFn to be more readable

3e95b45

Signed-off-by: Ahmed Hassan <[email protected]>

add unit tests for getIntervalFromMaxSplits

cfc5078

Signed-off-by: Ahmed Hassan <[email protected]>

afhassan changed the title ~~Add limit for max range query splits by interval~~ Add dynamic splitting by interval for range queries Jan 27, 2025

afhassan changed the title ~~Add dynamic splitting by interval for range queries~~ Dynamic splitting by interval for range queries Jan 27, 2025

add unit tests for analyzeDurationFetchedByQuery

83efc5c

Signed-off-by: Ahmed Hassan <[email protected]>

harry671003 approved these changes Jan 28, 2025

View reviewed changes

afhassan added 2 commits January 27, 2025 19:21

fix formatting

427f5b2

Signed-off-by: Ahmed Hassan <[email protected]>

update docs

c931aa6

Signed-off-by: Ahmed Hassan <[email protected]>

harry671003 reviewed Jan 28, 2025

View reviewed changes

docs/configuration/v1-guarantees.md Outdated Show resolved Hide resolved

update experimental features

c1ae047

Signed-off-by: Ahmed Hassan <[email protected]>

yeya24 reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic splitting by interval for range queries #6458

Dynamic splitting by interval for range queries #6458

afhassan commented Dec 24, 2024 •

edited

Loading

harry671003 Dec 27, 2024

afhassan Dec 30, 2024 •

edited

Loading

yeya24 commented Dec 31, 2024

afhassan commented Dec 31, 2024

This comment was marked as resolved.

afhassan Jan 22, 2025

yeya24 commented Jan 20, 2025 •

edited

Loading

harry671003 commented Jan 20, 2025 •

edited

Loading

yeya24 commented Jan 21, 2025

harry671003 Jan 23, 2025

afhassan Jan 28, 2025

harry671003 Jan 23, 2025

harry671003 Jan 23, 2025

afhassan Jan 28, 2025

harry671003 Jan 23, 2025

afhassan Jan 28, 2025

afhassan commented Jan 28, 2025

harry671003 left a comment

yeya24 Jan 28, 2025 •

edited

Loading

yeya24 Jan 28, 2025

Dynamic splitting by interval for range queries #6458

Are you sure you want to change the base?

Dynamic splitting by interval for range queries #6458

Conversation

afhassan commented Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

afhassan Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

yeya24 commented Dec 31, 2024

afhassan commented Dec 31, 2024

This comment was marked as resolved.

Choose a reason for hiding this comment

yeya24 commented Jan 20, 2025 • edited Loading

harry671003 commented Jan 20, 2025 • edited Loading

yeya24 commented Jan 21, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afhassan commented Jan 28, 2025

harry671003 left a comment

Choose a reason for hiding this comment

yeya24 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afhassan commented Dec 24, 2024 •

edited

Loading

afhassan Dec 30, 2024 •

edited

Loading

yeya24 commented Jan 20, 2025 •

edited

Loading

harry671003 commented Jan 20, 2025 •

edited

Loading

yeya24 Jan 28, 2025 •

edited

Loading