*: Allow exposing multiple label-sets #1284

brancz · 2019-06-27T20:26:03Z

Changes

This PR adds that the Info gRPC service now exposes potentially multiple label-sets. For all store types except proxy this doesn't change anything for now, it just adds the currently single label-set as the only entry in the list of label-sets. The proxy type, from now exposes it's stores' labels merged with its configured selector.

I'd like to ask that the proxy.go changes are particularly carefully reviewed.

Verification

Adapted all tests and see them passing.

@squat @bwplotka @GiedriusS @povilasv @domgreen

povilasv

Definetely add changelog entry as this breaks our interface. labels -> labelsets in InfoResponse proto.

Would be amazing to see more docs, explaining how the store matching works, does store has too much all the labelsets only one, etc? Potential use cases.

pkg/store/storepb/rpc.proto

pkg/store/proxy.go

bwplotka

Initial review (did not finish yet). Overall LGTM, some suggestions so far (:

Will continue later.

bwplotka · 2019-06-28T08:16:31Z

pkg/query/storeset.go

-	s.mtx.RLock()
-	defer s.mtx.RUnlock()
+func (s *storeRef) Update(labelSets []storepb.LabelSet, minTime int64, maxTime int64) {
+	s.mtx.Lock()


pkg/query/storeset.go

pkg/store/storepb/rpc.proto

test/e2e/rule_test.go

pkg/query/storeset.go

pkg/store/tsdb.go

pkg/query/storeset.go

pkg/store/prometheus.go

bwplotka

I think there might be 2 major things to improve (let me know if I am wrong):

matching is quite odd, is AND intended?
I don't think we need fanout on Info

pkg/store/proxy.go

bwplotka · 2019-06-28T13:07:49Z

pkg/store/proxy.go

@@ -193,7 +266,7 @@ func (s *ProxyStore) Series(r *storepb.SeriesRequest, srv storepb.Store_SeriesSe

 			sc, err := st.Series(seriesCtx, r)
 			if err != nil {
-				storeID := fmt.Sprintf("%v", storepb.LabelsToString(st.Labels()))
+				storeID := storepb.LabelSetsToString(st.LabelSets())


pkg/store/proxy.go

pkg/store/storepb/custom.go

pkg/store/storepb/rpc.proto

brancz · 2019-06-28T19:41:28Z

I believe I have addressed all comments and rebased the branch to resolve conflicts with master. Let me know what you think 🙂 . Overall this is in a much better shape than the first time around I think! I'm feeling much more comfortable with it now. Thanks for the reviews!

bwplotka · 2019-07-01T13:16:00Z

CHANGELOG.md

@@ -19,6 +19,8 @@ We use *breaking* word for marking changes that are not backward compatible (rel

 - [#1248](https://github.com/improbable-eng/thanos/pull/1248) Add a web UI to show the state of remote storage.

+- [#1284](https://github.com/improbable-eng/thanos/pull/1284) Add support for multiple label-sets in Info gRPC service. This deprecates the single `Labels` slice of the `InfoResponse`.


Let's mention maybe what's the plan on deprecation. It looks like breaking change, while it is not. Worth to add details on that otherwise users will be like Ok, but then all the components has to be upgraded to 0.6.0 if I use 0.6.0 Querier?

bwplotka

LGTM, just very minor nits.

Thanks!

bwplotka · 2019-07-01T13:20:40Z

pkg/store/proxy.go

 	stores := s.stores()
+
+	if len(stores) == 0 {
+		res.MaxTime = math.MaxInt64


I am not sure about this. I know there is not regression to previous version (same logic), but does this make sense? ;p
Let's leave the comment maybe? // Edge case: we have all of the data if there are no stores. and add some TODO.

Essentially not sure if we no store we should do either

max time = MaxInt64 and min time = maxInt64 as nothing is there.
or

max time = MaxInt64 and min Time MinInt64 if we want to have whole range ;p

But we should not spend time on this in this PR I guess.

Would an issue be more appropriate than a TODO in code maybe?

I'm adding the "Edge case:" comment back and will open an issue to decide this as a follow up.

Thanks, makes sense

bwplotka · 2019-07-01T13:22:29Z

pkg/store/proxy.go

-	res.MaxTime = MaxTime
-	res.MinTime = MinTime
+	res.MaxTime = maxTime
+	res.MinTime = minTime

 	for _, l := range s.selectorLabels {


I assume we are not removing selector Labels for now?

It somewhat depends on our decision around the changelog. If we decide all queriers must be >=v0.6.0 if any are, then this would be safe to remove.

👍 Let's leave for now

bwplotka · 2019-07-01T13:23:18Z

pkg/store/proxy.go

+
+	res.LabelSets = make([]storepb.LabelSet, 0, len(labelSets))
+	for _, v := range labelSets {
+		res.LabelSets = append(res.LabelSets, storepb.LabelSet{Labels: v})


Worth to mention on interface that sets are not sorted?

Added to the proto message and generated structs.

pkg/store/proxy.go

pkg/store/storepb/rpc.proto

This is used for backward compatibility of the querier with store APIs that don't expose LabelSet yet.

bwplotka

Thanks just one suggestion more - hopefully last one (:

bwplotka · 2019-07-01T18:16:22Z

pkg/query/storeset.go

@@ -32,14 +33,14 @@ type StoreSpec interface {
 	// If metadata call fails we assume that store is no longer accessible and we should not use it.
 	// NOTE: It is implementation responsibility to retry until context timeout, but a caller responsibility to manage
 	// given store connection.
-	Metadata(ctx context.Context, client storepb.StoreClient) (labels []storepb.Label, mint int64, maxt int64, err error)
+	Metadata(ctx context.Context, client storepb.StoreClient) (labelSets []storepb.LabelSet, labels []storepb.Label, mint int64, maxt int64, err error)


I don't really get why not just replacing with labelSets.

We can hide the compatibility logic internally, right?

bwplotka · 2019-07-01T18:17:00Z

pkg/query/storeset.go

-	s.mtx.RLock()
-	defer s.mtx.RUnlock()
+func (s *storeRef) Update(labelSets []storepb.LabelSet, labels []storepb.Label, minTime int64, maxTime int64) {
+	if len(labelSets) == 0 && len(labels) > 0 {


I would hide it behind Metadata interface.

bwplotka · 2019-07-01T18:17:18Z

pkg/store/proxy.go

 	stores := s.stores()
+
+	if len(stores) == 0 {
+		res.MaxTime = math.MaxInt64


Thanks, makes sense

bwplotka · 2019-07-01T18:17:33Z

pkg/store/proxy.go

-	res.MaxTime = MaxTime
-	res.MinTime = MinTime
+	res.MaxTime = maxTime
+	res.MinTime = minTime

 	for _, l := range s.selectorLabels {


👍 Let's leave for now

bwplotka · 2019-07-01T19:03:06Z

Awesome! 🚢 it.

Thanks 👍

bwplotka requested review from bwplotka and povilasv June 28, 2019 08:16

povilasv reviewed Jun 28, 2019

View reviewed changes

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

povilasv reviewed Jun 28, 2019

View reviewed changes

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

bwplotka reviewed Jun 28, 2019

View reviewed changes

bwplotka requested changes Jun 28, 2019

View reviewed changes

brancz force-pushed the multi-labelsets branch 5 times, most recently from e900615 to c447905 Compare June 28, 2019 19:27

*: Allow exposing multiple label-sets

58dee8a

brancz force-pushed the multi-labelsets branch from c447905 to 58dee8a Compare June 28, 2019 19:37

pkg/store: Add more multi-label-sets matching tests

45e2334

bwplotka reviewed Jul 1, 2019

View reviewed changes

bwplotka approved these changes Jul 1, 2019

View reviewed changes

brancz added 3 commits July 1, 2019 17:22

Address comments

a652743

If LabelSet is empty, and Labels is not, use Labels as LabelSet

ef37293

This is used for backward compatibility of the querier with store APIs that don't expose LabelSet yet.

pkg/store/proxy.go: Correctly handle no label-sets discovered

fe1c7f1

bwplotka approved these changes Jul 1, 2019

View reviewed changes

pkg/query: Encapsulate LabelSet defaulting in spec.Metadata

b4b0082

bwplotka approved these changes Jul 1, 2019

View reviewed changes

bwplotka merged commit 05f81a5 into thanos-io:master Jul 1, 2019

brancz deleted the multi-labelsets branch July 1, 2019 19:07

This was referenced Jul 2, 2019

Decide & apply what time range query without stores should announce #1299

Closed

Thanos receive component umbrella issue #1093

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Allow exposing multiple label-sets #1284

*: Allow exposing multiple label-sets #1284

brancz commented Jun 27, 2019

povilasv left a comment

bwplotka left a comment

bwplotka Jun 28, 2019

bwplotka left a comment •

edited

Loading

bwplotka Jun 28, 2019

brancz commented Jun 28, 2019

bwplotka Jul 1, 2019

bwplotka left a comment

bwplotka Jul 1, 2019

brancz Jul 1, 2019

brancz Jul 1, 2019

bwplotka Jul 1, 2019

bwplotka Jul 1, 2019

brancz Jul 1, 2019

bwplotka Jul 1, 2019

bwplotka Jul 1, 2019

brancz Jul 1, 2019

bwplotka left a comment

bwplotka Jul 1, 2019

bwplotka Jul 1, 2019

bwplotka Jul 1, 2019

bwplotka Jul 1, 2019

bwplotka commented Jul 1, 2019

		@@ -19,6 +19,8 @@ We use breaking word for marking changes that are not backward compatible (rel

		- [#1248](https://github.com/improbable-eng/thanos/pull/1248) Add a web UI to show the state of remote storage.

		- [#1284](https://github.com/improbable-eng/thanos/pull/1284) Add support for multiple label-sets in Info gRPC service. This deprecates the single `Labels` slice of the `InfoResponse`.

*: Allow exposing multiple label-sets #1284

*: Allow exposing multiple label-sets #1284

Conversation

brancz commented Jun 27, 2019

Changes

Verification

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

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Jun 28, 2019

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

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

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 commented Jul 1, 2019

bwplotka left a comment •

edited

Loading