storeApi: add component information to gRPC's Info response (resolves #739) #750

adrien-f · 2019-01-18T18:31:02Z

Signed-off-by: Adrien Fillon adrien.fillon@cdiscount.com

This PR aims to resolve #739.

Changes

*: Adds a set of enums for each components of Thanos
storeAPI: Adds a component field to Info's response

Verification

adrien-f · 2019-01-18T18:33:35Z

Greetings everyone 👋

Before I go further on this PR, let me know if I take the right approach. I limited myself to active components, dunno if it's worth it to have some kind of Tool enum for the other CLI commands like the bucket command.

Also not an expert in Protobuf. And that multi-interface thing in component.go is hard to reason about (imho).

Cheers 🎉

adrien-f · 2019-01-19T22:30:33Z

BTW, which UI version do you prefer ?

A:

B:

/cc @metalmatze @jojohappy

bwplotka

Thanks a lot!

You were sceptical of this component desing on previous description. Are you sold now or do you have specific objections? (: I feel it kind of fits well.

Overall I like the direction! LGTM. Some suggestions.

Thanks for 2 different options. I really like both.. But B) might be more readable (:

pkg/component/component.go

pkg/store/storepb/rpc.proto

cmd/thanos/sidecar.go

pkg/component/component.go

adrien-f · 2019-01-25T18:01:53Z

I've completed every components for now ! I went with Option B. Here are some screenshots:

One thing I'd like some input is this bit: https://github.com/Cdiscount/thanos/blob/767eaaaeb59f72dc2b705386627a8c8948a31452/pkg/query/storeset.go#L312

I initially went with updating the StoreSpec.Metadata call but that messed with Gossip and the gossipSpec.Metadata in cmd/thanos/query.go. Not sure if that's the right thing to do (c).

Things left to do:

Testing with a Gossip based cluster and a mixed one.
Re-Review some things

bwplotka · 2019-01-29T12:48:11Z

Awesome, can we remove WIP from PR title? It should encourage reviewers more ;p

bwplotka · 2019-01-29T12:48:26Z

Also some minor rebase would be nice.

metalmatze · 2019-01-29T15:23:38Z

pkg/component/component.go

+	"github.com/improbable-eng/thanos/pkg/store/storepb"
+)
+
+type StoreAPI interface {


Go wants comments like this for exported functions:

Suggested change

type StoreAPI interface {

// StoreAPI is ...

type StoreAPI interface {

https://blog.golang.org/godoc-documenting-go-code

Exaclty this ^

metalmatze

Code looks good to me. I like the option B you went with. Thanks for taking care of this! 👍

adrien-f · 2019-02-01T16:20:12Z

That was some rebase from hell !

I'm pretty happy with how it looks 😄 Hope to add some more UI niceties soon !

GiedriusS · 2019-02-02T12:20:18Z

pkg/store/bucket_test.go

+	dir, err := ioutil.TempDir("", "prometheus-test")
+	testutil.Ok(t, err)
+
+	// TODO: might be worth creating a `testutil.NewBucketStore()`


I haven't actually executed this code but LGTM. Except for this comment - what do you mean by this? What would it do?

A bit like https://github.com/improbable-eng/thanos/blob/00619585c5dc3d0ce2275a8864e3b666d8375acc/pkg/testutil/prometheus.go#L76 to create a test instance.

Well the only benefit would be to use shorter function instead of (single line anyway) NewBucketStore(nil, nil, nil, dir, 2e5, 2e5, false, 20).

Let's remove this comment and go with what it is here 👍

jojohappy

LGTM 👍 I totally like option B. Thanks a lot!

adrien-f · 2019-02-03T12:06:59Z

Errrr, I messed up the latest merge 😕 Will fix this later today.

bwplotka

LGTM and thanks!

Some nits related to commentary and formatting.

There is one potential to use embedding interfaces, not sure why you replicate method but we might have diamond problem: https://joaodlf.com/go-the-diamond-problem.html but I think we can go with this as well 👍

bwplotka · 2019-02-04T11:48:29Z

cmd/thanos/rule.go

@@ -522,7 +523,7 @@ func runRule(
 		}
 		logger := log.With(logger, "component", "store")


Let's fix this and put proper type here as well (:

bwplotka · 2019-02-04T11:48:33Z

cmd/thanos/rule.go

@@ -193,7 +194,7 @@ func runRule(
 	peer cluster.Peer,
 	objStoreConfig *pathOrContent,
 	tsdbOpts *tsdb.Options,
-	component string,
+	component component.SourceStoreAPI,


There is no point for this argument... Let's just put use constant everywhere (:

bwplotka · 2019-02-04T20:32:23Z

cmd/thanos/sidecar.go

@@ -77,7 +78,7 @@ func registerSidecar(m map[string]setupFunc, app *kingpin.Application, name stri
 			objStoreConfig,
 			peer,
 			rl,
-			name,


Same here - let's just put constant in place everywhere in sidecar - no point to dynamically eval this variable

pkg/component/component.go

bwplotka · 2019-02-04T20:44:39Z

pkg/component/component.go

+	"github.com/improbable-eng/thanos/pkg/store/storepb"
+)
+
+// Component that implements gRPC StoreApi


So in Golang for public structs and methods needs to start with method or struct name (:

So here would be StoreAPI is an component that... (:

Not sure if it is written anywhere, but essentially golint (go get -u golang.org/x/lint/golint && golint ./pkg/component/....) will show you the guidance there. It is super strict and sometimes annoying so we don't use it in CI but for those struct and method comments it makes sense, so can we adjust comments in this file in this way? (:

And missing trailing period for all comments here (:

bwplotka · 2019-02-04T20:45:21Z

pkg/component/component.go

+	"github.com/improbable-eng/thanos/pkg/store/storepb"
+)
+
+type StoreAPI interface {


Exaclty this ^

bwplotka · 2019-02-04T20:48:02Z

pkg/component/component.go

+// Component that implements gRPC StoreApi and produce blocks of metrics
+type SourceStoreAPI interface {
+	implementsStoreAPI()
+	producesBlocks()


Again, we could use embedding here. For example:

type SourceStoreAPI interface { Source StoreAPI }

But I guess it will cause diamond problem as described here: https://joaodlf.com/go-the-diamond-problem.html

(with String()) - that's fine then - let's leave your version as it is.

pkg/component/component.go

bwplotka · 2019-02-04T20:55:44Z

pkg/store/storepb/rpc.proto

 message InfoResponse {
  repeated Label labels = 1 [(gogoproto.nullable) = false];
  int64 min_time        = 2;
  int64 max_time        = 3;
+  StoreType storeType = 4;


nice, just formatting is off

bwplotka · 2019-02-04T20:57:25Z

pkg/store/bucket_test.go

+	dir, err := ioutil.TempDir("", "prometheus-test")
+	testutil.Ok(t, err)
+
+	// TODO: might be worth creating a `testutil.NewBucketStore()`


Well the only benefit would be to use shorter function instead of (single line anyway) NewBucketStore(nil, nil, nil, dir, 2e5, 2e5, false, 20).

Let's remove this comment and go with what it is here 👍

Signed-off-by: Adrien Fillon <adrien.fillon@cdiscount.com>

bwplotka

LGTM!

bwplotka · 2019-02-06T16:41:49Z

Thanks for this!

Now we need to think about something to fix that: #748

bwplotka reviewed Jan 20, 2019

View reviewed changes

pkg/component/component.go Show resolved Hide resolved

pkg/store/storepb/rpc.proto Outdated Show resolved Hide resolved

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

pkg/component/component.go Show resolved Hide resolved

bwplotka added the priority: P1 label Jan 29, 2019

metalmatze reviewed Jan 29, 2019

View reviewed changes

metalmatze approved these changes Jan 29, 2019

View reviewed changes

bwplotka mentioned this pull request Feb 1, 2019

Cut v0.3.0 release. #797

Merged

3 tasks

adrien-f changed the title ~~WIP: *: add component information to gRPC's Info response (resolves #739)~~ storeApi: add component information to gRPC's Info response (resolves #739) Feb 1, 2019

GiedriusS approved these changes Feb 2, 2019

View reviewed changes

jojohappy approved these changes Feb 2, 2019

View reviewed changes

bwplotka approved these changes Feb 4, 2019

View reviewed changes

bwplotka added priority: P0 and removed priority: P1 labels Feb 6, 2019

storeApi: add component information to gRPC's Info response

36a61c7

Signed-off-by: Adrien Fillon <adrien.fillon@cdiscount.com>

bwplotka approved these changes Feb 6, 2019

View reviewed changes

bwplotka merged commit e1b0cb6 into thanos-io:master Feb 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storeApi: add component information to gRPC's Info response (resolves #739) #750

storeApi: add component information to gRPC's Info response (resolves #739) #750

adrien-f commented Jan 18, 2019 •

edited

adrien-f commented Jan 18, 2019

adrien-f commented Jan 19, 2019

bwplotka left a comment

adrien-f commented Jan 25, 2019 •

edited

bwplotka commented Jan 29, 2019

bwplotka commented Jan 29, 2019

metalmatze Jan 29, 2019

bwplotka Feb 4, 2019

metalmatze left a comment

adrien-f commented Feb 1, 2019

GiedriusS Feb 2, 2019 •

edited

adrien-f Feb 3, 2019

bwplotka Feb 4, 2019

jojohappy left a comment

adrien-f commented Feb 3, 2019

bwplotka left a comment

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka Feb 4, 2019

bwplotka left a comment

bwplotka commented Feb 6, 2019

	type StoreAPI interface {
	// StoreAPI is ...
	type StoreAPI interface {

		@@ -522,7 +523,7 @@ func runRule(
		}
		logger := log.With(logger, "component", "store")

storeApi: add component information to gRPC's Info response (resolves #739) #750

storeApi: add component information to gRPC's Info response (resolves #739) #750

Conversation

adrien-f commented Jan 18, 2019 • edited

Changes

Verification

adrien-f commented Jan 18, 2019

adrien-f commented Jan 19, 2019

bwplotka left a comment

Choose a reason for hiding this comment

adrien-f commented Jan 25, 2019 • edited

bwplotka commented Jan 29, 2019

bwplotka commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metalmatze left a comment

Choose a reason for hiding this comment

adrien-f commented Feb 1, 2019

GiedriusS Feb 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jojohappy left a comment

Choose a reason for hiding this comment

adrien-f commented Feb 3, 2019

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

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

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Feb 6, 2019

adrien-f commented Jan 18, 2019 •

edited

adrien-f commented Jan 25, 2019 •

edited

GiedriusS Feb 2, 2019 •

edited