Approximate `quantile_over_time` #10417

jeschkies · 2023-09-01T05:47:29Z

What this PR does / why we need it:
This change shards quantile_over_time queries using t-digest or DDSketch approximations.

Outstanding

Replace generic return type of StepEvaluator with interface StepResult.
Send mapped query with quantile sketch expression from frontend to querier over the wire.
Serialize sketches. See Marshaler and Unmarshaler influxdata/tdigest#34
~~Benchmark.~~
Add feature flag.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

or ddksetch, including a test which compares the accuracy and sketch size in bytes of both options Signed-off-by: Callum Styan <[email protected]>

…roximation

dannykopping

Almost there, thanks for the changes 👍

docs/sources/configure/_index.md

pkg/logql/downstream.go

dannykopping · 2023-12-04T08:10:13Z

pkg/logql/quantile_over_time_sketch.go

@@ -0,0 +1,413 @@
+package logql


Looks like some of the error cases in this file, as well as the explain view, are not covered by tests

pkg/logql/quantile_over_time_sketch.go

pkg/logql/syntax/clone_test.go

pkg/querier/queryrange/roundtrip.go

cstyan · 2023-12-04T19:47:56Z

pkg/logql/syntax/clone_test.go

@@ -77,6 +80,20 @@ func TestClone(t *testing.T) {
 	}
 }

+func TestCLoneStringLabelFilter(t *testing.T) {


typo on CLone -> Clone

cstyan · 2023-12-04T19:50:57Z

pkg/logql/explain.go

+// MaxChildrenDisplay defines the maximum number of children that should be
+// shown by explain.
+const MaxChildrenDisplay = 3


should this be configurable in some way? we might hide information we want to see this way, right?

At some point yes. However, explain is not used anywhere.

cstyan · 2023-12-04T19:51:52Z

pkg/ingester/ingester.go

@@ -912,6 +912,7 @@ func (i *Ingester) QuerySample(req *logproto.SampleQueryRequest, queryServer log
 	_, ctx := stats.NewContext(queryServer.Context())
 	sp := opentracing.SpanFromContext(ctx)

+	// If the plan is empty we want all series to be returned.


so if we have a nil plan here it's intentional?

pkg/logql/downstream.go

…roximation

**What this PR does / why we need it**: This change makes the `StepEvaluator` more flexible for changes like grafana#10417 that introduce a different step result. With the interface `StepResult` we can support other vector types in the future. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e)

**What this PR does / why we need it**: Following grafana#11123 and in order to enable grafana#10417 the query frontend should send the serialized LogQL AST instead of the query string to the queriers. This enables the frontend to change the AST and inject expressions that are not expressible in LogQL. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](grafana@0d4416a) --------- Signed-off-by: Callum Styan <[email protected]> Co-authored-by: Callum Styan <[email protected]>

**What this PR does / why we need it**: This change shards `quantile_over_time` queries using t-digest or DDSketch approximations. It can be enabled with `querier.shard_aggregations=quantile_over_time`. Outstanding - [x] Replace generic return type of `StepEvaluator` with interface `StepResult`. - [x] Send mapped query with quantile sketch expression from frontend to querier over the wire. - [x] Serialize sketches. See influxdata/tdigest#34 - [x] Add feature flag. **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [x] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [x] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) --------- Signed-off-by: Callum Styan <[email protected]> Co-authored-by: Callum Styan <[email protected]>

jeschkies added 7 commits August 31, 2023 15:09

Vendor tdigest and ddsketch.

4ea54f7

Make StepEvaluator result generic.

9768704

Define tdigest batch iterator.

8303daa

Try generics.

a0b6b55

Join tdigest vector into a tdigest matrix.

5127581

Start testing.

b56cfc2

Add TDigestEvalExpr and TDigestMergeExpr.

a15bdc5

jeschkies requested review from dannykopping and cstyan September 1, 2023 05:47

pull-request-size bot added the size/L label Sep 1, 2023

jeschkies changed the title ~~Karsten/quantile approximation~~ Approximate quantile_over_time Sep 1, 2023

jeschkies and others added 3 commits September 1, 2023 11:19

Define TDigestMergeExpr.

da18856

implement interface for quantile sketch, which can be backed by tdigest

2ab4285

or ddksetch, including a test which compares the accuracy and sketch size in bytes of both options Signed-off-by: Callum Styan <[email protected]>

Merge and evaluate tdigest sketches.

09dfd87

pull-request-size bot added size/XL and removed size/L labels Sep 11, 2023

jeschkies added 7 commits September 11, 2023 15:18

Skip optimization.

1c45001

Introduce quantile sketch expr.

b052d70

Parse query before execution.

f095b02

Run tests.

0f53ba9

Use more samples.

bee822d

Correct tests.

a8e1cc4

Create separate test for sketches with relative error.

2359aac

pull-request-size bot added size/XXL and removed size/XL labels Sep 12, 2023

jeschkies added 5 commits September 12, 2023 12:48

Satisfy linter.

4d71095

Remove unused code.

dfaca21

Merge remote-tracking branch 'grafana/main' into karsten/quantile-app…

e35b769

…roximation

quantile_over_time should be sharded without grouping.

2e053c4

Use constant for "true"

a656d7a

jeschkies added 4 commits December 1, 2023 16:03

Test quantile matrix roundtriop

39a5b2f

Rename from proto method

db33dc9

Test probabilisitc quantile serialization

db511ad

Merge remote-tracking branch 'grafana/main' into karsten/quantile-app…

274a1e1

…roximation

jeschkies requested review from dannykopping and cstyan December 1, 2023 15:40

Merge branch 'main' into karsten/quantile-approximation

bdc8e1e

dannykopping reviewed Dec 4, 2023

View reviewed changes

cstyan reviewed Dec 4, 2023

View reviewed changes

jeschkies added 4 commits December 6, 2023 11:02

Merge remote-tracking branch 'grafana/main' into karsten/quantile-app…

fe9c94a

…roximation

Update shard aggregations

1dc2c81

Address some comments

121db5d

Check for empty query plan

75ad077

jeschkies requested review from dannykopping and cstyan December 6, 2023 10:46

jeschkies added 4 commits December 6, 2023 11:47

Test a few error cases

c0eaf3d

Merge remote-tracking branch 'grafana/main' into karsten/quantile-app…

48ebd00

…roximation

Merge remote-tracking branch 'grafana/main' into karsten/quantile-app…

fbdd7d6

…roximation

Merge branch 'main' into karsten/quantile-approximation

95aba11

cstyan approved these changes Dec 11, 2023

View reviewed changes

jeschkies added 2 commits December 11, 2023 09:11

Handle empty plan

158e10a

Merge remote-tracking branch 'grafana/main' into karsten/quantile-app…

2572167

…roximation

jeschkies merged commit f67fff3 into grafana:main Dec 11, 2023
8 checks passed

jeschkies deleted the karsten/quantile-approximation branch December 11, 2023 08:58

grafanabot mentioned this pull request Dec 11, 2023

[Release Notes Appender] Add PR #10417: Approximate quantile_over_time #11424

Closed

grafanabot added a commit that referenced this pull request Dec 11, 2023

Add PR #10417 to release notes for next release

33aa44c

dannykopping approved these changes Dec 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Approximate `quantile_over_time` #10417

Approximate `quantile_over_time` #10417

jeschkies commented Sep 1, 2023 •

edited

Loading

dannykopping left a comment

dannykopping Dec 4, 2023

cstyan Dec 4, 2023

jeschkies Dec 6, 2023

cstyan Dec 4, 2023

jeschkies Dec 6, 2023

cstyan Dec 4, 2023

jeschkies Dec 6, 2023

Approximate quantile_over_time #10417

Approximate quantile_over_time #10417

Conversation

jeschkies commented Sep 1, 2023 • edited Loading

dannykopping left a comment

Choose a reason for hiding this comment

dannykopping Dec 4, 2023

Choose a reason for hiding this comment

cstyan Dec 4, 2023

Choose a reason for hiding this comment

jeschkies Dec 6, 2023

Choose a reason for hiding this comment

cstyan Dec 4, 2023

Choose a reason for hiding this comment

jeschkies Dec 6, 2023

Choose a reason for hiding this comment

cstyan Dec 4, 2023

Choose a reason for hiding this comment

jeschkies Dec 6, 2023

Choose a reason for hiding this comment

Approximate `quantile_over_time` #10417

Approximate `quantile_over_time` #10417

jeschkies commented Sep 1, 2023 •

edited

Loading