-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store: selector labels from store #1059
Conversation
pkg/store/prometheus.go
Outdated
return false, nil, nil | ||
} | ||
|
||
tms = append(tms[:i], tms[i+1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question here. Why do we remove the match label from request LabelMatcher
? I follow the implemete of the labelsMatches
method above. But I could not catch the point :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry what do you mean remove? You mean continue
part on line 315? This feels wrong. It's bit different to what we have here:
thanos/pkg/store/prometheus.go
Line 311 in d02488d
if extValue == "" { |
That's why.. can we have just one function for matching + selector labels? (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading the code, I'm sorry that I misunderstood the external_labels
that those should not be in the storepb.LabelMatcher
, because those would be added to any series. Here the selector-labels
are as same as the external_labels
used by store gateway sharding selector. We assume that all of series in the bucket queried by store
component have the selector-labels
pairs.
I have reused the matchesExternalLabels
function to check selector-labels
, and do translateMatcher
again to get the new labels.Matcher
for querying.
5215e60
to
877ff79
Compare
877ff79
to
9707e55
Compare
@jojohappy should we get back to this? Sorry for not reviewing this for some time - but make it non-draft - less chances the review will be postponed. I think we all agree to this so let's implement this. Thanks for helping! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direction is good - just some suggestions + can we rebase? (:
Thanks @jojohappy !
cmd/thanos/store.go
Outdated
@@ -50,11 +51,20 @@ func registerStore(m map[string]setupFunc, app *kingpin.Application, name string | |||
blockSyncConcurrency := cmd.Flag("block-sync-concurrency", "Number of goroutines to use when syncing blocks from object storage."). | |||
Default("20").Int() | |||
|
|||
selectorLabels := cmd.Flag("selector-label", "Store Gateway selector labels that will be exposed in info endpoint (repeated)."). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure this is the same, consistent for receive
and maybe rule
? Querier either dropped it already or will drop as we aggregate labels there from storeAPIs.
pkg/store/prometheus.go
Outdated
@@ -298,6 +298,32 @@ func labelsMatches(lset labels.Labels, ms []storepb.LabelMatcher) (bool, []store | |||
return true, newMatcher, nil | |||
} | |||
|
|||
func labelsMatchesAndTranslate(lset labels.Labels, ms []storepb.LabelMatcher) (bool, []labels.Matcher, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might work after few nits, but let's focus on reusing stuff. We do matching almost on every component. We for sure can figure out some helper/abstraction for this somehow critical path. (: More code reused = potentially fewer bugs and less code to read. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with you. I have delete the method and reuse the matchesExternalLabels
to check the labels matcher.
pkg/store/prometheus.go
Outdated
return false, nil, nil | ||
} | ||
|
||
tms = append(tms[:i], tms[i+1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry what do you mean remove? You mean continue
part on line 315? This feels wrong. It's bit different to what we have here:
thanos/pkg/store/prometheus.go
Line 311 in d02488d
if extValue == "" { |
That's why.. can we have just one function for matching + selector labels? (:
9707e55
to
633dfc3
Compare
@bwplotka I have updated. PTAL! After that, I will try to add some testings. |
9728f6d
to
c8c2559
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good so far!
Thanks for doing this @jojohappy. Super simple and nice.
There is an important item in Querier we need to look carefully and adjust, please see the comment below.
CHANGELOG.md
Outdated
@@ -15,6 +15,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel | |||
|
|||
- [#1358](https://github.com/thanos-io/thanos/pull/1358) Added `part_size` configuration option for HTTP multipart requests minimum part size for S3 storage type | |||
- [#1363](https://github.com/thanos-io/thanos/pull/1363) Thanos Receive now exposes `thanos_receive_hashring_nodes` and `thanos_receive_hashring_tenants` metrics to monitor status of hash-rings | |||
- [#1059](https://github.com/improbable-eng/thanos/pull/1059) Store: selector labels from store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [#1059](https://github.com/improbable-eng/thanos/pull/1059) Store: selector labels from store. | |
- [#1059](https://github.com/improbable-eng/thanos/pull/1059) Store: Added label selecting. This allows store gateway to serve only certain TSDB blocks from object store. |
cmd/thanos/store.go
Outdated
@@ -49,7 +50,15 @@ func registerStore(m map[string]setupFunc, app *kingpin.Application, name string | |||
blockSyncConcurrency := cmd.Flag("block-sync-concurrency", "Number of goroutines to use when syncing blocks from object storage."). | |||
Default("20").Int() | |||
|
|||
selectorLabels := cmd.Flag("store.selector-label", "Store Gateway selector labels that will be exposed in info endpoint (repeated)."). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm we have
selectorLabels := cmd.Flag("selector-label", "Query selector labels that will be exposed in info endpoint (repeated).").
PlaceHolder("<name>=\"<value>\"").Strings()
Maybe worth to be consistent with naming and remove store
prefix? essentially it has not much to do with store API for example.
} | ||
|
||
for _, lset := range s.selectorLabels { | ||
infoResp.Labels = append(infoResp.Labels, storepb.Label{Name: lset.Name, Value: lset.Value}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change we need to adjust Querier logic:
Line 248 in 865d5ec
// Check if the store has some external labels specified and if any if there are duplicates. |
Otherwise, sidecar + store gateway will be assumed a "duplicate" in Querier. Let's think about how we can fix this.
The storeAPI duplication detection logic is embedded in Querier to make sure user doesn't put many sidecars that upload blocks with the same external labels (common bug). I think we might want to change storeset
behavior to look on StoreAPI InfoResponse.StoreType
I only don't like the fact that for Querier all of this is no longer some StoreAPI
but actually Sidecar, Store...
I might think we actually would need to change StoreType to Producer, Browser
as that would be a good abstraction over different implementations.
Thoughts @povilasv @GiedriusS @brancz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something or shouldn't this just be the set of all labelsets seen in all blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
Or better: Let's maybe quickly write up some proposal as we want consistent thing everywhere we do selectors + maybe selecting prefixes as well.
cc @jojohappy
Signed-off-by: jojohappy <[email protected]>
Signed-off-by: jojohappy <[email protected]>
Signed-off-by: jojohappy <[email protected]>
Signed-off-by: jojohappy <[email protected]>
Signed-off-by: jojohappy <[email protected]>
Signed-off-by: jojohappy <[email protected]>
Signed-off-by: jojohappy <[email protected]>
82734c3
to
5cec0a2
Compare
I think we need this, but we need something we discussed here: #1245 (comment) I think we need short proposal maybe (?) |
@jojohappy are you happy to close this in the meantime to stop confusion? I will start some PR with the design it would be also awesome if you would join us in making this if you want (: |
That's OK.
Yes, I'd like to. |
@@ -56,12 +57,20 @@ func registerStore(m map[string]setupFunc, app *kingpin.Application, name string | |||
maxTime := model.TimeOrDuration(cmd.Flag("max-time", "End of time range limit to serve. Thanos Store serves only blocks, which happened eariler than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y."). | |||
Default("9999-12-31T23:59:59Z")) | |||
|
|||
selectorLabels := cmd.Flag("selector-label", "Query selector labels that will be exposed in info endpoint (repeated)."). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On each component that works on the object storage (e.g Store GW and Compactor), add --selector.relabel-config
(and corresponding --selector.relabel-config-file
) as in proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On top of this we want to make sure Info() endpoint has exact labels of blocks that we shard.
Signed-off-by: jojohappy [email protected]
Changes
Implements: #1034
Add
--selector
labels for store gateway.Verification
To see the store page to verify the announced labels field.