Skip to content
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

querier: Support store matchers and time range filter on labels API #3133

Merged
merged 4 commits into from
Sep 10, 2020

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Sep 7, 2020

Signed-off-by: Ben Ye yb532204897@gmail.com

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

Changes

This pr supports store matches for Labels API. It also supports time range based filtering in Querier to reduce unnecessary calls for labels API.

Verification

Signed-off-by: Ben Ye <yb532204897@gmail.com>

Add more unit tests in proxy store

Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24 yeya24 force-pushed the support-store-matchers-labels branch from 8037d3a to 7d3d7df Compare September 7, 2020 15:23
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@kakkoyun kakkoyun changed the title Support store matchers and time range filter on labels API querier: Support store matchers and time range filter on labels API Sep 8, 2020
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Just some nits.

pkg/api/query/v1_test.go Outdated Show resolved Hide resolved
pkg/api/query/v1_test.go Show resolved Hide resolved
pkg/api/query/v1.go Outdated Show resolved Hide resolved
@@ -270,10 +270,10 @@ func (s *ProxyStore) Series(r *storepb.SeriesRequest, srv storepb.Store_SeriesSe
ok, _ = storeMatches(st, r.MinTime, r.MaxTime, storeMatcher, r.Matchers...)
})
if !ok {
storeDebugMsgs = append(storeDebugMsgs, fmt.Sprintf("store %s filtered out", st))
storeDebugMsgs = append(storeDebugMsgs, fmt.Sprintf("Store %s filtered out", st))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor thing but why was this word capitalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought capitalized looks better.

Capitialized

level=warn ts=2020-09-08T20:59:57.731820598Z caller=proxy.go:310 err="No StoreAPIs matched for this query" stores="Store Addr: localhost:10905 LabelSets: [name:\"cluster\" value:\"test\" ] Mint: 1598450400000 Maxt: 1599408000000 filtered out;Store Addr: localhost:10903 LabelSets: [name:\"cluster\" value:\"test\" ] Mint: 1598299200000 Maxt: 9223372036854775807 filtered out"

Not Capitalized

level=warn ts=2020-09-08T21:02:28.654728738Z caller=proxy.go:310 err="No StoreAPIs matched for this query" stores="store Addr: localhost:10903 LabelSets: [name:\"cluster\" value:\"test\" ] Mint: 1598299200000 Maxt: 9223372036854775807 filtered out;store Addr: localhost:10905 LabelSets: [name:\"cluster\" value:\"test\" ] Mint: 1598450400000 Maxt: 1599537600000 filtered out"

stores="store Addr: localhost:10903

It looks weird to have store not capitalized but have Addr capitalized.

@@ -515,7 +515,8 @@ func storeMatches(s Client, mint, maxt int64, storeMatcher [][]storepb.LabelMatc
return false, nil
}
match, err := storeMatchMetadata(s, storeMatcher)
if err != nil || !match {
// Return result here if no matchers set.
if len(matchers) == 0 || err != nil || !match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't labelSetsMatch() already return true, nil if matchers is nil? In labelSetMatches we range over nil and because it's empty, we simply jump to the end and return true, nil, right?

Copy link
Contributor Author

@yeya24 yeya24 Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return labelSetsMatch(s.LabelSets(), matchers) If we don't return and then go to labelSetsMatch here, we need to first evaluate s.LabelSets() https://github.com/thanos-io/thanos/blob/master/pkg/query/storeset.go#L277.

I want to avoid this function because it needs to get the lock and alloc a slice.

CHANGELOG.md Outdated Show resolved Hide resolved
@yeya24
Copy link
Contributor Author

yeya24 commented Sep 8, 2020

Hello @GiedriusS I just update this pr. Please take a look again when you get a chance, thanks!

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One very minor thing and I think we can merge this.

pkg/api/query/v1_test.go Show resolved Hide resolved
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24 yeya24 force-pushed the support-store-matchers-labels branch from 15f4951 to c89ef0a Compare September 9, 2020 15:01
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥇 We can merge it when CI is green.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Merged the main branch into yours & fixed CHANGELOG conflicts, will merge on green. Let's please avoid force pushes in the future and rewriting history if possible otherwise it makes our lives a bit harder.

@GiedriusS GiedriusS merged commit e0b7f7b into thanos-io:master Sep 10, 2020
@yeya24 yeya24 deleted the support-store-matchers-labels branch September 10, 2020 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants