Query Frontend: add query instant tripperware for query sharding #5561

yeya24 · 2022-08-02T07:08:59Z

Signed-off-by: Ben Ye ben.ye@bytedance.com

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

This pr adds a simple tripperware for sharding instant queries.

Verification

fpetkovski · 2022-08-02T07:40:07Z

pkg/queryfrontend/request.go

@@ -133,6 +137,97 @@ func (r *ThanosQueryRangeRequest) String() string { return "" }
 // which is not used in thanos.
 func (r *ThanosQueryRangeRequest) ProtoMessage() {}

+type ThanosQueryInstantRequest struct {


Do we have a way to use the same object for instant and range queries? That way we wouldn't have to duplicate code for these two use cases.

I prefer to split the two types. Having them in the same struct is okay but we need a flag to maintain whether it is instant query and range query, and check the flag when needed.
Compared to having two types, this is not that clear and require other code, too.

Got it, makes sense 👍

fpetkovski · 2022-08-02T16:18:33Z

One question, do you think it makes sense to have separate sharding number for query range and query instant? It would mean we change --query-frontend.vertical-shards to

--query-range.vertical-shards
--query-instant.vertical-shards

which is a "breaking change", but we also haven't released the feature yet.

yeya24 · 2022-08-02T16:40:32Z

One question, do you think it makes sense to have separate sharding number for query range and query instant? It would mean we change --query-frontend.vertical-shards to
--query-range.vertical-shards
--query-instant.vertical-shards
which is a "breaking change", but we also haven't released the feature yet.

What about using the same NumShards flag?

type Config struct {
	QueryRangeConfig
	LabelsConfig
	DownstreamTripperConfig

	CortexHandlerConfig    *transport.HandlerConfig
	CompressResponses      bool
	CacheCompression       string
	RequestLoggingDecision string
	DownstreamURL          string
	ForwardHeaders         []string
	NumShards              int
}

Now the NumShards is defined in the root level instead of inside QueryRangeConfig so I think it is totally okay to use it in instant queries.
Maintaining two flags are also okay but I cannot think of a usecase for setting vertical shards differently for different query types.

fpetkovski · 2022-08-02T16:49:14Z

Got it. Let's start with a single flag and we can revisit if a use case comes up.

yeya24 · 2022-08-03T01:57:22Z

After we vendor the cortex package internally, is there a way to import the cortex proto file we vendored? Tried multiple times but didn't find a way to get it working with existing genproto.sh script.

GiedriusS · 2022-08-03T07:37:33Z

After we vendor the cortex package internally, is there a way to import the cortex proto file we vendored? Tried multiple times but didn't find a way to get it working with existing genproto.sh script.

Yeah, I think it hasn't been updated. You probably need to add -I option that points to the Cortex directory.

yeya24 · 2022-08-09T07:00:05Z

Still cannot get it working by adding the Cortex dir to the proto dependency. Wondering what's the future plan on this.
Are we allowed to modify the vendored Cortex code for some use cases? We may need to build the Cortex proto ourselves.

fpetkovski · 2022-08-09T07:41:30Z

Could you share the command that fails for you currently?

yeya24 · 2022-08-13T08:25:48Z

Could you share the command that fails for you currently?

Thanks. I figured out a way to compile the Cortex protobuf instead and make changes on it directly. Not sure if it is a acceptable way but I got it working finally. Seems query response parsing and instant query sharding are working properly on my local tests.

I will add more tests soon.

sonatype-lift · 2022-08-13T08:42:22Z

pkg/queryfrontend/queryinstant_codec.go

+		body, _ := ioutil.ReadAll(r.Body)
+		return nil, httpgrpc.Errorf(r.StatusCode, string(body))
+	}
+	log, ctx := spanlogger.New(ctx, "ParseQueryInstantResponse") //nolint:ineffassign,staticcheck


SA4006: this value of ctx is never used

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

pkg/queryfrontend/queryinstant_codec.go

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

yeya24 · 2022-08-15T16:53:15Z

This is ready for another round of review. @fpetkovski @GiedriusS

fpetkovski

This looks good, my only concern is that we need to modify the vendored Cortex files. I am not sure though what's the strategy for keeping up to date with upstream Cortex.

Maybe @kakkoyun or @bwplotka have an idea?

fpetkovski · 2022-08-16T11:16:37Z

internal/cortex/cortexpb/cortex.proto

@@ -7,7 +7,7 @@ package cortexpb;

 option go_package = "cortexpb";

-import "github.com/gogo/protobuf/gogoproto/gogo.proto";
+import "gogoproto/gogo.proto";


I wonder if we can do this rewrite in go.mod or with some sed magic in the Makefile.

My concern is that the change can be lost the next time we update Cortex.

yeya24 · 2022-08-17T03:42:56Z

This looks good, my only concern is that we need to modify the vendored Cortex files. I am not sure though what's the strategy for keeping up to date with upstream Cortex.

Yeah, I see your concern. I am not sure about this as well. If needed I can try to avoid modifying the Cortex code. But this causes more code duplication. For example, we need to copy Cortex unexposed functions like merging query stats, etc.

kakkoyun · 2022-08-17T04:39:12Z

Even though it's not ideal for syncing up with the upstream Cortex, we'll need to modify the vendored Cortex files. If I'm not mistaken, @GiedriusS also working on a PR that modifies the vendored files. It'll be a half-automated process to upgrade Cortex in Thanos, but I don't see any other options. If someone has a brilliant idea to do this, please let us know :)

fpetkovski

Nice, just one more question from my side. Looking forward to using this!

fpetkovski · 2022-08-17T06:17:11Z

pkg/queryfrontend/queryinstant_codec.go

+		return nil, err
+	}
+
+	if r.FormValue(queryv1.MaxSourceResolutionParam) == "auto" {


Does downsampling apply to instant queries?

Yeah from the instant query API I can see the param is used.

GiedriusS

LGTM, it will be a pain to keep Cortex fork up-to-date, though, however it's inevitable and I can't think of a better strategy at the moment. 💪 good work!

fpetkovski · 2022-08-18T06:16:57Z

Amazing stuff, one of our instant queries went from 36s to 8s.

yeya24 · 2022-08-18T06:23:36Z

Whoops, I didn't expect the PR got merged so quickly. I haven't added some unit tests and changelog, too. I will add them in follow up prs.

One important thing I want to make sure is to have PromQL compatibility test for Thanos Query behind the query frontend to make sure no regression. Since the pr is already large I will do it in another pr, too.

…nos-io#5561) * add query instant tripperware for query sharding Signed-off-by: Ben Ye <ben.ye@bytedance.com> * fix lint Signed-off-by: Ben Ye <ben.ye@bytedance.com> * working version for instant query sharding Signed-off-by: Ben Ye <ben.ye@bytedance.com> * tidy up Signed-off-by: Ben Ye <ben.ye@bytedance.com> * lint Signed-off-by: Ben Ye <ben.ye@bytedance.com> * fix unit test Signed-off-by: Ben Ye <ben.ye@bytedance.com> * remove ioutil Signed-off-by: Ben Ye <ben.ye@bytedance.com> * fix E2E test Signed-off-by: Ben Ye <ben.ye@bytedance.com> * add e2e test for instant query sharding Signed-off-by: Ben Ye <ben.ye@bytedance.com> Signed-off-by: Ben Ye <ben.ye@bytedance.com>

pull-request-size bot added the size/XL label Aug 2, 2022

fpetkovski reviewed Aug 2, 2022

View reviewed changes

pull-request-size bot added size/XXL and removed size/XL labels Aug 13, 2022

sonatype-lift bot reviewed Aug 13, 2022

View reviewed changes

pkg/queryfrontend/queryinstant_codec.go Outdated Show resolved Hide resolved

yeya24 force-pushed the query-instant-tripperware branch 2 times, most recently from 074ba82 to 2603a96 Compare August 13, 2022 18:03

Ben Ye added 7 commits August 13, 2022 11:14

add query instant tripperware for query sharding

c2726ae

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

fix lint

2666632

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

working version for instant query sharding

a4160f2

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

tidy up

b3ac1b1

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

lint

a34c0f5

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

fix unit test

19e1dae

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

remove ioutil

303bc68

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

yeya24 force-pushed the query-instant-tripperware branch from 2603a96 to 303bc68 Compare August 13, 2022 18:15

Ben Ye added 2 commits August 13, 2022 21:52

fix E2E test

4b1c942

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

add e2e test for instant query sharding

9c533a2

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

fpetkovski reviewed Aug 16, 2022

View reviewed changes

fpetkovski reviewed Aug 17, 2022

View reviewed changes

GiedriusS approved these changes Aug 17, 2022

View reviewed changes

GiedriusS merged commit 13a2eb8 into thanos-io:main Aug 17, 2022

yeya24 deleted the query-instant-tripperware branch August 18, 2022 06:23

yeya24 mentioned this pull request Aug 19, 2022

Query Frontend: Add tests and fix previous bugs for instant query tripperware #5612

Merged

2 tasks

GiedriusS mentioned this pull request Aug 30, 2022

remote read: support sharding prometheus/prometheus#11231

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Frontend: add query instant tripperware for query sharding #5561

Query Frontend: add query instant tripperware for query sharding #5561

yeya24 commented Aug 2, 2022

fpetkovski Aug 2, 2022

yeya24 Aug 2, 2022

fpetkovski Aug 2, 2022

fpetkovski commented Aug 2, 2022

yeya24 commented Aug 2, 2022

fpetkovski commented Aug 2, 2022

yeya24 commented Aug 3, 2022

GiedriusS commented Aug 3, 2022

yeya24 commented Aug 9, 2022

fpetkovski commented Aug 9, 2022

yeya24 commented Aug 13, 2022

sonatype-lift bot Aug 13, 2022

yeya24 commented Aug 15, 2022

fpetkovski left a comment

fpetkovski Aug 16, 2022

yeya24 commented Aug 17, 2022 •

edited

kakkoyun commented Aug 17, 2022

fpetkovski left a comment

fpetkovski Aug 17, 2022

yeya24 Aug 17, 2022

GiedriusS left a comment

fpetkovski commented Aug 18, 2022

yeya24 commented Aug 18, 2022

Query Frontend: add query instant tripperware for query sharding #5561

Query Frontend: add query instant tripperware for query sharding #5561

Conversation

yeya24 commented Aug 2, 2022

Changes

Verification

fpetkovski Aug 2, 2022

Choose a reason for hiding this comment

yeya24 Aug 2, 2022

Choose a reason for hiding this comment

fpetkovski Aug 2, 2022

Choose a reason for hiding this comment

fpetkovski commented Aug 2, 2022

yeya24 commented Aug 2, 2022

fpetkovski commented Aug 2, 2022

yeya24 commented Aug 3, 2022

GiedriusS commented Aug 3, 2022

yeya24 commented Aug 9, 2022

fpetkovski commented Aug 9, 2022

yeya24 commented Aug 13, 2022

sonatype-lift bot Aug 13, 2022

Choose a reason for hiding this comment

yeya24 commented Aug 15, 2022

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski Aug 16, 2022

Choose a reason for hiding this comment

yeya24 commented Aug 17, 2022 • edited

kakkoyun commented Aug 17, 2022

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski Aug 17, 2022

Choose a reason for hiding this comment

yeya24 Aug 17, 2022

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

fpetkovski commented Aug 18, 2022

yeya24 commented Aug 18, 2022

yeya24 commented Aug 17, 2022 •

edited