Split by range of instant queries #5662

ssncferreira · 2022-03-17T16:39:43Z

What this PR does / why we need it:

This PR introduces a new middleware that splits instant queries with a large range into several smaller sub-queries to parallelize their execution.

The interval by which instant queries are split is defined by the setting split_queries_by_interval.

Note: Using a too small split interval may result in a lot of sub-queries, which can cause the query queue to fill up and make requests slower than faster.

Special notes for your reviewer:

The following screenshot shows the p99 query latency for old and new implementation, using a split interval of 1d and various vector and range aggregations with 7d and 30d ranges respectively.

Checklist

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

chaudum · 2022-03-29T11:14:50Z

pkg/querier/queryrange/split_by_range.go

+		return nil, httpgrpc.Errorf(http.StatusBadRequest, err.Error())
+	}
+
+	interval := validation.SmallestPositiveNonZeroDurationPerTenant(tenants, s.limits.QuerySplitDuration)


The split duration is still subject to change and requires further testing. We may need to introduce a second setting, or even calculate the split duration dynamically based on the range of the query.

owen-d

This is looking really good, but it's in need of some equivalence testing (see TestMappingEquivalence in logql/downstream_test.go)

pkg/logql/rangemapper.go

owen-d · 2022-04-01T18:06:06Z

pkg/logql/rangemapper.go

+			Operation: overrideDownstream.Operation,
+		}
+	}
+	return &syntax.BinOpExpr{


Clever how you've accounted for all this inside logql evaluation!

pkg/logql/rangemapper.go

owen-d · 2022-04-01T18:29:41Z

pkg/logql/rangemapper.go

+	//  Currently, it is sending the last inner expression grouping dowstream.
+	//  Which grouping should be sent downstream?
+	var vectorAggrPushdown *syntax.VectorAggregationExpr
+	if expr.Grouping != nil && expr.Grouping.Groups != nil && expr.Operation != syntax.OpTypeCount {


I'm not sure we need the expr.Grouping.Groups != nil check. Can't we also push down a sum (foo), which is effectively sum by () (foo)?

I'll check.

pkg/logql/rangemapper.go

pkg/logql/rangemapper_test.go

pkg/logql/rangemapper.go

owen-d · 2022-04-01T19:01:44Z

pkg/querier/queryrange/split_by_range.go

+	}
+
+	switch res.Data.Type() {
+	case parser.ValueTypeMatrix:


nit: by having two separate middlewares, we're converting back and forth between promtheus types (Matrix, Vector, Scalar) and queryrange.Response types multiple times in each query (since each middleware has it's own logql engine which downstreams. This is probably fine for simplicity's sake unless we find a better way to handle this.

We also parse the query each time. That's probably not as bad. In any case it feels these middlewares could be refactored at some point and "just" pass down the modified query or query plan.

chaudum · 2022-04-04T09:38:58Z

This is looking really good, but it's in need of some equivalence testing (see TestMappingEquivalence in logql/downstream_test.go)

There are a bunch of range aggregation and vector aggregation queries tested in TestRangeMappingEquivalence in downstream_test.go.
Or do you mean, adding an additional test with the new range mapper and the shard mapper as in TestMappingEquivalence?

owen-d · 2022-04-04T12:34:08Z

I've taken another look and found the equivalence tests -- nice work! I've also dismissed a few of my earlier comments. This is looking about ready, @cyriltovena do you want to give it another pair of 👀 ?

chaudum · 2022-04-04T12:43:49Z

@owen-d thanks for the review! I think this new feature still needs additional instrumentation, like in the shard mapper. Hope it's ok to do that in a separate PR.

jeschkies

Nice. I'm half way through the mapper. Need to go once more through it.

pkg/logql/rangemapper.go

jeschkies · 2022-04-04T13:26:06Z

pkg/querier/queryrange/split_by_range.go

+	}
+
+	switch res.Data.Type() {
+	case parser.ValueTypeMatrix:


We also parse the query each time. That's probably not as bad. In any case it feels these middlewares could be refactored at some point and "just" pass down the modified query or query plan.

pkg/querier/queryrange/split_by_range.go

cyriltovena · 2022-04-06T09:35:56Z

pkg/logql/downstream_test.go

+
+		// Non-splittable vector aggregators
+		// TODO: Fix topk
+		//{`topk(2, count_over_time({a=~".+"}[2s]))`, time.Second},


I'd like to see some successful topk.

Let's do it in a 2nd iteration :)

cyriltovena

LGTM ship it 🚀

trevorwhitney

Well that was a lesson in LogQL. Very cool stuff, excited for how this will improve our queries. Just a couple nits for you as that's about all I could contribute to this already impressive effort. Nice work!

trevorwhitney · 2022-04-06T21:06:54Z

pkg/logql/rangemapper.go

+	}
+
+	var downstreams *ConcatSampleExpr
+	for interval = 0; interval < splitCount; interval++ {


super small nit: interval is a bit confusing here since it's an iterator while rangeInterval and m.splitByInterval are both durations. Maybe call this split, since it's an iterator across the splits in the splitCount?

trevorwhitney · 2022-04-06T21:28:53Z

pkg/logql/rangemapper_test.go

+	for _, tc := range []struct {
+		expr       string
+		expected   string
+		expectNoop bool


nit: this test is massive. I think we could move all the noop cases to their own test for ease of finding them / organizational sake, which means we could get rid of this property in the struct. WDYT?

jeschkies · 2022-04-07T14:26:28Z

pkg/logql/rangemapper.go

+
+// clone returns a copy of the given sample expression
+// This is needed whenever we want to modify the existing query tree.
+func clone(expr syntax.SampleExpr) (syntax.SampleExpr, error) {


Why not syntax.Clone(expr)?

syntax.Clone() does basically the same thing, but with different types: Expr->Expr, whereas here I want SampleExpr->SampleExpr. This is mainly for convenience.

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

if the outer query range contains an offset as well. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

…ressions

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Since this is the main function of the mapper, we can ensure here that only supported vector/range aggregations are handled. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

…sent Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

This test is essentially the same as the test Test_SplitRangeVectorMapping, just using a different representation of the result. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

… is present Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

…rouping is present Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

to have the test for noop queries separated Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum · 2022-04-07T19:16:11Z

Hey @owen-d I think we are ready to merge this PR. When you squash the commits, please feel free to remove all the "intermediate" commit messages from the individual commits, and use the PR description instead.

pull-request-size bot added the size/XXL label Mar 17, 2022

ssncferreira force-pushed the instant-query-splitting-v3-optimizations branch 4 times, most recently from 084407e to 6449bf9 Compare March 18, 2022 10:18

kavirajk mentioned this pull request Mar 18, 2022

Split by range on Instant queries POC #5561

Closed

3 tasks

chaudum force-pushed the instant-query-splitting-v3-optimizations branch from 797d2b1 to e116c31 Compare March 29, 2022 11:08

chaudum marked this pull request as ready for review March 29, 2022 11:09

chaudum requested a review from a team as a code owner March 29, 2022 11:09

chaudum requested review from owen-d, slim-bean and cyriltovena March 29, 2022 11:09

chaudum changed the title ~~Split by range on Instant queries POC~~ Split by range if instant queries Mar 29, 2022

chaudum changed the title ~~Split by range if instant queries~~ Split by range of instant queries Mar 29, 2022

chaudum reviewed Mar 29, 2022

View reviewed changes

owen-d reviewed Apr 1, 2022

View reviewed changes

jeschkies reviewed Apr 4, 2022

View reviewed changes

chaudum force-pushed the instant-query-splitting-v3-optimizations branch from 83a67d4 to 43bdf55 Compare April 5, 2022 08:07

cyriltovena reviewed Apr 6, 2022

View reviewed changes

cyriltovena approved these changes Apr 6, 2022

View reviewed changes

owen-d approved these changes Apr 6, 2022

View reviewed changes

trevorwhitney reviewed Apr 6, 2022

View reviewed changes

jeschkies approved these changes Apr 7, 2022

View reviewed changes

jeschkies reviewed Apr 7, 2022

View reviewed changes

ssncferreira and others added 3 commits April 7, 2022 20:50

Split by range on Instant queries POC v3

63e9ba6

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

Handle uneven split by duration

c7569c3

Register SplitByRangeMiddleware in roundtripper

473465c

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

ssncferreira and others added 24 commits April 7, 2022 20:50

Fix linting

60c5781

Calculate offset of downstream queries correctly

ce6663d

if the outer query range contains an offset as well. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix linting

a649930

Add optimization by moving the outer label grouping downstream

f2c6ac6

Add label grouping downstream optimization to rate and bytes_rate exp…

98ac288

…ressions

Add changelog entry

98f3dc4

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Simplify types in rangemapper

6d0eaa2

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! Simplify types in rangemapper

749eb6a

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Check in Map function if query is splittable by range

b3f2b21

Since this is the main function of the mapper, we can ensure here that only supported vector/range aggregations are handled. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Some code cleanups and variable renaming

0148380

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Extract duplicate code in range aggr mapping into function

aa6736f

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add topk to supported splittable vector aggregations

5037439

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Check if query is splittable by range before calling Map()

c4d3e3f

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add more function comments

3362f5c

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Rename RangeVectorMapper to RangeMapper

53f16be

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix incorrect import due to rebase

98cf3bc

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Add equivalence test cases with logfmt pipeline stage

f6d10db

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove limitation of pushing down vector aggr only if grouping is pre…

10f0208

…sent Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove TestRangeMappingEquivalenceMockMapper test

cf4be23

This test is essentially the same as the test Test_SplitRangeVectorMapping, just using a different representation of the result. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! Remove limitation of pushing down vector aggr only if grouping…

9cb483c

… is present Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

fixup! fixup! Remove limitation of pushing down vector aggr only if g…

93c0268

…rouping is present Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Fix linter errors

f607dfe

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Better naming of variable

e031615

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Split SplitRangeVectorMapping test into two

8267d00

to have the test for noop queries separated Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum force-pushed the instant-query-splitting-v3-optimizations branch from 699c354 to 8267d00 Compare April 7, 2022 18:51

Merge branch 'main' into instant-query-splitting-v3-optimizations

4225c90

owen-d merged commit 0bce8d9 into main Apr 7, 2022

owen-d deleted the instant-query-splitting-v3-optimizations branch April 7, 2022 22:18

RomainBelorgey mentioned this pull request Jul 25, 2022

Performance issues after upgrading from 2.5.0 to 2.6.1 #6764

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split by range of instant queries #5662

Split by range of instant queries #5662

ssncferreira commented Mar 17, 2022 •

edited by chaudum

Loading

chaudum Mar 29, 2022

owen-d left a comment

owen-d Apr 1, 2022

owen-d Apr 1, 2022

chaudum Apr 4, 2022

owen-d Apr 1, 2022

jeschkies Apr 4, 2022

chaudum commented Apr 4, 2022

owen-d commented Apr 4, 2022

chaudum commented Apr 4, 2022

jeschkies left a comment

jeschkies Apr 4, 2022

cyriltovena Apr 6, 2022

chaudum Apr 6, 2022

cyriltovena left a comment

trevorwhitney left a comment

trevorwhitney Apr 6, 2022

trevorwhitney Apr 6, 2022

jeschkies Apr 7, 2022

chaudum Apr 7, 2022

chaudum commented Apr 7, 2022

Split by range of instant queries #5662

Split by range of instant queries #5662

Conversation

ssncferreira commented Mar 17, 2022 • edited by chaudum 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaudum commented Apr 4, 2022

owen-d commented Apr 4, 2022

chaudum commented Apr 4, 2022

jeschkies left a 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

cyriltovena left a comment

Choose a reason for hiding this comment

trevorwhitney left a 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

chaudum commented Apr 7, 2022

ssncferreira commented Mar 17, 2022 •

edited by chaudum

Loading