Skip to content

Add a query result limit to regex matcher #6845

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SungJin1212
Copy link
Member

@SungJin1212 SungJin1212 commented Jun 27, 2025

This PR implements a tenant-federation.max-tenant flag logic when the tenant-federation.regex-matcher-enabled flag is enabled.
Since the QFE uses a regex validator, which validates input regex and passes it to the Query scheduler or Querier, the QFE handler cannot validate the max tenant limit.
Instead of emitting an error when # of matched tenants over the tenant-federation.max-tenant, I adjusted # of tenants to query because # of matched tenants is unexpected if the user uses a regex like user-+.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@SungJin1212 SungJin1212 force-pushed the Add-result-limit-on-regex-matcher branch from a3ab0e4 to fd3d145 Compare June 27, 2025 13:59
@SungJin1212 SungJin1212 requested a review from yeya24 June 28, 2025 22:59
@@ -160,6 +162,11 @@ func (r *RegexResolver) getRegexMatchedOrgIds(orgID string) ([]string, error) {
return []string{"fake"}, nil
}

if r.maxTenant > 0 && len(matched) > r.maxTenant {
// adjust matched tenants to maxTenant
matched = matched[:r.maxTenant]
Copy link
Contributor

@yeya24 yeya24 Jun 30, 2025

Choose a reason for hiding this comment

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

The current behavior of too many tenant IDs in Query Frontend is to reject the query with too many tenants error. Should we align with the same behavior?

Capping the number of tenants and resuming the query seems a new behavior. I would say we should go with rejecting the query and ask people to explicitly match fewer tenants.

If we cap, we need to document how we reject tenants, and details like if we sort or not, etc. And also we need to return a warning to let users know that the tenants are truncated.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, let's go with your suggestion for retaining existing behavior.

@SungJin1212 SungJin1212 force-pushed the Add-result-limit-on-regex-matcher branch 2 times, most recently from 9f90f5d to 0d74844 Compare June 30, 2025 04:43
@SungJin1212 SungJin1212 force-pushed the Add-result-limit-on-regex-matcher branch from 0d74844 to 7e0ca92 Compare June 30, 2025 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants