Querier/Sidecar/StoreGW: Implement Info service and add `--endpoint` flag in Querier #4282

hitanshu-mehta · 2021-05-27T19:46:37Z

This PR implements a Unified Endpoint Proposal. Open for reviews :)

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

Changes

Implemented Info server.
Added Info server in Sidecar and StoreGW.
Added --endpoint flag in querier.
~~Modified storeset tests to verify discovery using endpoint flag.~~ Completely removed storeset and added endpointset query: add endpointset flow #4421

Verification

Manually tested using grpccurl.
~~Modified storeset tests to verify discovery using endpoint flag.~~

TODO

~~E2E tests.~~
Add Info server in remaining components.

pkg/info/info.go

pkg/query/storeset_test.go

squat

Great work so far; some ideas from our weekly 1-1

cmd/thanos/query.go

pkg/info/info.go

pkg/query/storeset.go

squat · 2021-05-28T10:38:54Z

pkg/query/storeset.go

+type InfoSpec interface {
+	// Addr returns InofAPI Address for the info spec. It is used as its ID.
+	Addr() string
+	// Metadata returns current labels, min/max ranges for StoreAPI and ExemplarAPI


Let's start with the smaller abstraction and add this later if we see we need to generalize it to all Info types only if they all need it really. Otherwise, we can keep it in the StoreSpec

bwplotka · 2021-06-02T19:58:33Z

Is it rdy for review? (:

hitanshu-mehta · 2021-06-03T03:20:50Z

I have to add e2e tests. Apart from that, PR is ready for review 🚀

yeya24 · 2021-06-06T02:01:47Z

cmd/thanos/store.go

+				MaxTime: maxt,
+			}
+		},
+		func() *infopb.ExemplarsInfo { return nil },


Can we use nil here? What's the difference between nil and a function returning nil?

Sorry missed it. Yes, we can pass nil as we are doing with other parameters. We are allowing to pass nil for convenience. #4282 (comment)

test/e2e/info_api_test.go

yeya24 · 2021-06-06T02:08:57Z

cmd/thanos/sidecar.go

+				return &infopb.ExemplarsInfo{
+					MinTime: time.Unix(math.MinInt64/1000+62135596801, 0).UTC().Unix(),
+					MaxTime: time.Unix(math.MaxInt64/1000-62135596801, 999999999).UTC().Unix(),
+				}


We can use global variable for the two values to avoid calculating them each time.

@hitanshu-mehta We are still calling .Unix() every time. Can't we just initialize v1.MinTime and v1.MaxTime as int64? Are they used anywhere else?

Currently, they are not being used anywhere else. Agree, I should've used int64. I'll correct it. Thank you!

The thing is we can have kind of constants somewhere in global level. Can we do that? It's still to be imp[roved?

hitanshu-mehta · 2021-06-13T16:50:36Z

I had to rename messages InfoRequest and InfoResponse because messages with these names are already defined in proto file of storeAPI. So, a warning for duplicate proto type registered was given. This warning message was also being printed in component-specific files when we run make docs.

GiedriusS

Just some comments/suggestions but overall LGTM 💪 nice work!

GiedriusS · 2021-06-15T20:13:56Z

pkg/query/storeset.go

@@ -312,6 +329,21 @@ func (s *storeRef) Update(labelSets []labels.Labels, minTime int64, maxTime int6
 	s.exemplar = exemplar
 }

+func (s *storeRef) UpdateWithStore(labelSets []labels.Labels, minTime int64, maxTime int64, storeType component.StoreAPI, store storepb.StoreClient, rule rulespb.RulesClient, target targetspb.TargetsClient, metadata metadatapb.MetadataClient, exemplar exemplarspb.ExemplarsClient) {


Nit: maybe this could be folded into Update and then we could simply add:

if store != nil { s.StoreClient = store }

This would save a bit of code duplication.

is it store at this point? sounds really like just endpointRef.

Also the amount of arguments sounds like code smell. I wonder if we hide those behind better abstractions

pkg/query/storeset_test.go

test/e2e/info_api_test.go

GiedriusS · 2021-06-15T20:37:33Z

I had to rename messages InfoRequest and InfoResponse because messages with these names are already defined in proto file of storeAPI. So, a warning for duplicate proto type registered was given. This warning message was also being printed in component-specific files when we run make docs.

Maybe let's call them InfoAPIRequest and InfoAPIResponse or something to differentiate between those two? We can discuss this in #thanos-dev, maybe someone has even better suggestions

yeya24

~~I am wondering how would this API work with routing only receivers? IMO routing only receivers shouldn't expose any store API. Can you add a test case for this? cc @yashrsharma44 @hitanshu-mehta~~

Edit: Sorry, this pr doesn't deal with receiver part.

bwplotka

Amazing work everyone!

I wonder only - what's the deprecation plan and how to not add complexity to storeset if it has to be removed at some point and instead focus on endpointset that supports old StoreAPI and new separate info

cmd/thanos/query.go

bwplotka · 2021-06-17T08:35:24Z

cmd/thanos/store.go

@@ -379,6 +382,25 @@ func runStore(
 			cancel()
 		})
 	}
+
+	infoSrv := info.NewInfoServer(


What's the deprecation plan for ths old Info methods? Can we unify implementation somehow at least?

Really sorry for delay.
Here is the deprecation plan I had in mind.

Remove old flags (i.e. --store, --rule, --metadata, --exemplar, --target) and all the boilerplate code we have for all commands. ( eg. duplication check, dnsProviders ...)

Remove storeset.go file ( I am working on to move new endpoint flow in new file as discussed here ) and all dependencies of this file from the code base.

Remove Info rpc from proto file of Store API and all its corresponding implementations. ( to be specific there 5 different implementations we have of StoreClient - BucketStore, MultiTSDBStore, PrometheusStore, ProxyStore and TSDBStore)

All these steps will be done in single PR. I think this plan should work .
Please let me know if im missing something :)

Can we unify implementation somehow at least?

Why do we need to unify old info methods?

When we want to change flag we usually deprecate flag first (add in help that it will be removed after next 2 releases). Then add new flags that works simultaneously. Fail if both new and old way is used. Map old flags to new one really before starting the program.

Agree

Generally makes sense, but we need to make sure that e.g old Querier having a client against old StoreAPI will work with new InfoAPI. Probably we will need to leave old Info Method (add deprecated comment) in old APIs. In the same time the new Querier should work with old APIs.

Potential flow: 1. Try (reflect)? InfoAPI end endpoint. If gRPC server does not support it we fallback to store, if store does it work then ruler ..... only for first call / error.

e2e test

IMPORTANT: This does not mean we need to implement separate info method for every server. We can implement one Info method and use the same one for both InfoAPI and e.g StoreAPI.

bwplotka · 2021-06-17T08:37:06Z

pkg/info/infopb/rpc.proto

@@ -23,12 +23,12 @@ option (gogoproto.goproto_sizecache_all) = false;
 /// Info represents the API that is responsible for gathering metadata about the all APIs supported by the component.
 service Info {
    /// Info returns the metadata (Eg. LabelSets, Min/Max time) about all the APIs the component supports.
-    rpc Info(InfoRequest) returns (InfoResponse);
+    rpc Info(InfoReq) returns (InfoResp);


First time I see this practice. While it helps with shorter name it violates coding style guide + consistency. Do we think it's worth it?

If I don't rename it then a warning message will also be printed in our documentations as I mentioned here. I was not able to find a way to remove them. @GiedriusS has suggested better names InfoAPIRequest and InfoAPIResponse.

This is because our pacakge is thanos. Do you mind changing this to thanos.info?

bwplotka · 2021-06-17T08:38:14Z

pkg/query/storeset.go

@@ -312,6 +329,21 @@ func (s *storeRef) Update(labelSets []labels.Labels, minTime int64, maxTime int6
 	s.exemplar = exemplar
 }

+func (s *storeRef) UpdateWithStore(labelSets []labels.Labels, minTime int64, maxTime int64, storeType component.StoreAPI, store storepb.StoreClient, rule rulespb.RulesClient, target targetspb.TargetsClient, metadata metadatapb.MetadataClient, exemplar exemplarspb.ExemplarsClient) {


is it store at this point? sounds really like just endpointRef.

Also the amount of arguments sounds like code smell. I wonder if we hide those behind better abstractions

bwplotka · 2021-06-17T08:40:01Z

pkg/query/storeset.go

@@ -597,6 +630,105 @@ func (s *StoreSet) getActiveStores(ctx context.Context, stores map[string]*store
 			level.Warn(s.logger).Log("msg", "ignored rule store", "address", ruleAddr)
 		}
 	}
+
+	// Gather healthy stores map concurrently using info addresses. Build new store if does not exist already.
+	for _, infoSpec := range s.infoSpec() {


All of this is baked in storeset but I would see it really part of endpoints

I would really just copy and add a new endpoint flow - it will be easier to main new thing than to try to understand later what to remove from mixed thing.

Then if someone wants to use endpoints store flag should be empty. WDYT?

Agree! I will refactor this code and maybe create a new flow in a different endpointset.go file. WDYT ?

bwplotka

Still minor nits, but looks really close to be done!

cmd/thanos/query.go

bwplotka · 2021-07-01T16:39:48Z

cmd/thanos/sidecar.go

+				return &infopb.ExemplarsInfo{
+					MinTime: time.Unix(math.MinInt64/1000+62135596801, 0).UTC().Unix(),
+					MaxTime: time.Unix(math.MaxInt64/1000-62135596801, 999999999).UTC().Unix(),
+				}


The thing is we can have kind of constants somewhere in global level. Can we do that? It's still to be imp[roved?

bwplotka · 2021-07-01T16:39:59Z

cmd/thanos/store.go

@@ -379,6 +382,25 @@ func runStore(
 			cancel()
 		})
 	}
+
+	infoSrv := info.NewInfoServer(


bwplotka · 2021-07-01T16:41:16Z

pkg/info/infopb/rpc.proto

@@ -23,12 +23,12 @@ option (gogoproto.goproto_sizecache_all) = false;
 /// Info represents the API that is responsible for gathering metadata about the all APIs supported by the component.
 service Info {
    /// Info returns the metadata (Eg. LabelSets, Min/Max time) about all the APIs the component supports.
-    rpc Info(InfoRequest) returns (InfoResponse);
+    rpc Info(InfoReq) returns (InfoResp);


This is because our pacakge is thanos. Do you mind changing this to thanos.info?

yeya24 · 2021-07-01T18:10:09Z

Quick question about this and our usecase. We have a central thanos query and multiple edge thanos sidecars. If I use --endpoint flag to specify sidecars, then is there a way to disable metadata or it is enabled automatically?
I'd like to have a way to disable metadata queries for performance purposes rather than enabling them automatically. Fanouting these queries to edges are too slow.

hitanshu-mehta · 2021-07-04T13:58:05Z

@yeya24 Currently there isn't any way to disable specific api queries like metadata, targets etc.

Signed-off-by: Hitanshu Mehta <[email protected]>

GiedriusS

Last three suggestions before merging 👍 Also, please, please, please avoid force-pushing and overriding previous commits because when this happens the reviewers need to look through the whole code again to see whether it is still the same. That's why there was this big delay before I got to this again 👍

pkg/store/prometheus.go

cmd/thanos/query.go

test/e2e/info_api_test.go

hitanshu-mehta · 2021-11-05T03:54:35Z

@GiedriusS Really sorry for the trouble. I will avoid force pushing from now on :)

Signed-off-by: Hitanshu Mehta <[email protected]>

GiedriusS · 2021-11-08T07:52:30Z

@GiedriusS Really sorry for the trouble. I will avoid force pushing from now on :)

CI failure seems related (:

Signed-off-by: Hitanshu Mehta <[email protected]>

GiedriusS

💪 next steps are to actually use the new InfoServer & add tests for it? Plus, we should probably also add --endpoint-strict? Finally, remove the old flags? (: And that feature to ignore certain service(-s) would be nice too.

Note to others that --endpoint with this PR is semantically equivalent to all of the other flags since we prefill the expected APIs (we don't check the InfoServer, yet) so it is completely safe to merge this!

With thanos-io#4282 merged, it is time to unhide these very useful pages from the navigation bar. 🎉 Signed-off-by: Giedrius Statkevičius <[email protected]>

With #4282 merged, it is time to unhide these very useful pages from the navigation bar. 🎉 Signed-off-by: Giedrius Statkevičius <[email protected]>

With thanos-io#4282 merged, it is time to unhide these very useful pages from the navigation bar. 🎉 Signed-off-by: Giedrius Statkevičius <[email protected]>

sonatype-lift bot reviewed May 27, 2021

View reviewed changes

pkg/info/info.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed May 27, 2021

View reviewed changes

pkg/query/storeset_test.go Outdated Show resolved Hide resolved

squat reviewed May 28, 2021

View reviewed changes

hitanshu-mehta force-pushed the info-service branch 6 times, most recently from 8c653cb to b688deb Compare June 2, 2021 09:01

hitanshu-mehta mentioned this pull request Jun 2, 2021

gRPC: Isolate Info to separate gRPC API so we can reuse discovery for different RPCs: Store, Rules and Targets #2600

Open

hitanshu-mehta force-pushed the info-service branch from b688deb to a46c007 Compare June 2, 2021 11:28

hitanshu-mehta force-pushed the info-service branch from a46c007 to 3a09ce5 Compare June 3, 2021 03:16

yeya24 reviewed Jun 6, 2021

View reviewed changes

hitanshu-mehta force-pushed the info-service branch from 85f6075 to 803e7b1 Compare June 8, 2021 19:11

hitanshu-mehta marked this pull request as ready for review June 13, 2021 16:55

hitanshu-mehta changed the title ~~[WIP] Implement Info service and add --endpoint flag in Querier~~ Querier/Sidecar/StoreGW: Implement Info service and add --endpoint flag in Querier Jun 13, 2021

GiedriusS reviewed Jun 15, 2021

View reviewed changes

yeya24 reviewed Jun 16, 2021

View reviewed changes

bwplotka reviewed Jun 17, 2021

View reviewed changes

yeya24 mentioned this pull request Jun 18, 2021

query: exemplar support file-sd #4346

Closed

bwplotka reviewed Jul 1, 2021

View reviewed changes

hitanshu-mehta force-pushed the info-service branch from 8ffaa2a to 03baf00 Compare July 1, 2021 17:37

hitanshu-mehta mentioned this pull request Jul 6, 2021

query: add endpointset flow #4421

Merged

2 tasks

hitanshu-mehta marked this pull request as draft July 6, 2021 21:22

hitanshu-mehta added 14 commits October 29, 2021 08:15

Add Info server in Sidecar and StoreGW

9ea5957

Signed-off-by: Hitanshu Mehta <[email protected]>

Add --endpoint in querier

a076da0

Signed-off-by: Hitanshu Mehta <[email protected]>

Add E2E test for Info API

61c68cb

Signed-off-by: Hitanshu Mehta <[email protected]>

Make min/max times global vaiables and some other minor changes

95b1ce5

Signed-off-by: Hitanshu Mehta <[email protected]>

Enhance e2e test

245e9c4

Signed-off-by: Hitanshu Mehta <[email protected]>

Rename InfoRequest and InfoResponse to InfoReq and InfoResp

27b9a21

Signed-off-by: Hitanshu Mehta <[email protected]>

Minor fixes and change package name for proto file for info service

7f94177

Signed-off-by: Hitanshu Mehta <[email protected]>

resolve merge conflicts

daff1e3

Signed-off-by: Hitanshu Mehta <[email protected]>

add deprecation warning in description of old flags

829af65

Signed-off-by: Hitanshu Mehta <[email protected]>

fix typo

797f3a8

Signed-off-by: Hitanshu Mehta <[email protected]>

export new Timestamps method

b9f677b

Signed-off-by: Hitanshu Mehta <[email protected]>

add functional options in intializer of info server

ceaf2b4

Signed-off-by: Hitanshu Mehta <[email protected]>

change e2e tests to use efficientgo/e2e

c3baaea

Signed-off-by: Hitanshu Mehta <[email protected]>

nits in info api e2e tests

9b9b68c

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta dismissed stale reviews from matej-g and GiedriusS via 9b9b68c October 29, 2021 02:47

hitanshu-mehta force-pushed the info-service branch from 5a894b5 to 9b9b68c Compare October 29, 2021 02:47

hitanshu-mehta requested a review from GiedriusS October 29, 2021 05:56

GiedriusS previously approved these changes Nov 3, 2021

View reviewed changes

pkg/store/prometheus.go Outdated Show resolved Hide resolved

cmd/thanos/query.go Outdated Show resolved Hide resolved

test/e2e/info_api_test.go Show resolved Hide resolved

minor nits

fec2e8b

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta dismissed GiedriusS’s stale review via fec2e8b November 5, 2021 10:27

fix docs

a330d23

Signed-off-by: Hitanshu Mehta <[email protected]>

GiedriusS approved these changes Nov 9, 2021

View reviewed changes

GiedriusS merged commit ac2cc26 into thanos-io:main Nov 9, 2021

GiedriusS mentioned this pull request Nov 9, 2021

ui: unhide previously hidden pages #4845

Merged

2 tasks

yeya24 pushed a commit that referenced this pull request Nov 10, 2021

ui: unhide previously hidden pages (#4845)

12ce536

With #4282 merged, it is time to unhide these very useful pages from the navigation bar. 🎉 Signed-off-by: Giedrius Statkevičius <[email protected]>

Querier/Sidecar/StoreGW: Implement Info service and add --endpoint flag in Querier #4282

Querier/Sidecar/StoreGW: Implement Info service and add --endpoint flag in Querier #4282

Conversation

hitanshu-mehta commented May 27, 2021 • edited Loading

Changes

Verification

TODO

squat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Jun 2, 2021

hitanshu-mehta commented Jun 3, 2021

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

hitanshu-mehta commented Jun 13, 2021

GiedriusS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiedriusS commented Jun 15, 2021

yeya24 left a comment • edited Loading

Choose a reason for hiding this comment

bwplotka 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

hitanshu-mehta Jul 4, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hitanshu-mehta Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka 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

yeya24 commented Jul 1, 2021 • edited Loading

hitanshu-mehta commented Jul 4, 2021 • edited Loading

GiedriusS left a comment

Choose a reason for hiding this comment

hitanshu-mehta commented Nov 5, 2021

GiedriusS commented Nov 8, 2021

GiedriusS left a comment

Choose a reason for hiding this comment

Querier/Sidecar/StoreGW: Implement Info service and add `--endpoint` flag in Querier #4282

Querier/Sidecar/StoreGW: Implement Info service and add `--endpoint` flag in Querier #4282

hitanshu-mehta commented May 27, 2021 •

edited

Loading

yeya24 left a comment •

edited

Loading

hitanshu-mehta Jul 4, 2021 •

edited

Loading

hitanshu-mehta Jun 21, 2021 •

edited

Loading

yeya24 commented Jul 1, 2021 •

edited

Loading

hitanshu-mehta commented Jul 4, 2021 •

edited

Loading