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

Query: fix exemplar proxying for receivers with multiple tenants #7326

Merged

Conversation

pedro-stanaka
Copy link
Contributor

@pedro-stanaka pedro-stanaka commented May 2, 2024

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

Changes

  • Exported function that checks for matches on a group of labelsets from ProxyStore
  • Simplify logic and remove named loop from exemplars proxy.
  • Fixed the case where we needed to keep iterating on the store labelsets to see if we had a match on other tenant (external labels)

Verification

  • Adjusted tests with a repro case

@pedro-stanaka pedro-stanaka force-pushed the fix/exemplars-store-multitsdb branch from 4218d63 to 5ae2d70 Compare May 2, 2024 16:32
@pedro-stanaka pedro-stanaka changed the title Query: exemplar proxying broken for multitsdb Query: fix exemplar proxying for receivers with multiple tenants May 2, 2024
@pedro-stanaka pedro-stanaka marked this pull request as ready for review May 2, 2024 16:33
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
When using the exemplars proxy to search for exemplars on receivers, if one receiver had tenants that did not match the selector on the external label it would get
skipped completely even if it had a tenant that actually matched

Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
@pedro-stanaka pedro-stanaka force-pushed the fix/exemplars-store-multitsdb branch from e25690e to 20a608a Compare May 3, 2024 07:21
pkg/exemplars/proxy.go Outdated Show resolved Hide resolved
for m := range matcherSet {
labelMatchers = append(labelMatchers, m)
for _, m := range matcherSet {
if isExternalLabel(m.Name, extLbls) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should propagate labels matchers here, but check them in tsdb.go. Otherwise all tenants in a receiver will match whenever a single tenant matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it will fail when proxying via sidecar? Because we are also passing the query as is there. We could leave this as a follow up and maintain behavior for now.

I think the best approach is to cover on all places, but would require ppl upgrading all components if they are running on distributed mode. Otherwise exemplars queries will not match anything.

Signed-off-by: Pedro Tanaka <pedro.stanaka@gmail.com>
@fpetkovski fpetkovski merged commit 07838b8 into thanos-io:main May 6, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants