- Notifications
You must be signed in to change notification settings - Fork3.8k
Enable tenant ID selection for multi-tenant queries.#5821
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| returnlogproto.MergeSeriesResponses(responses) | ||
| } | ||
| // See https://github.com/grafana/mimir/blob/114ab88b50638a2047e2ca2a60640f6ca6fe8c17/pkg/querier/tenantfederation/tenant_federation.go#L29-L69 |
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.
Should we share this code or is copying fine?
Uh oh!
There was an error while loading.Please reload this page.
pkg/querier/multi_tenant_querier.go Outdated
| } | ||
| // replaceMatchers traverses the passed epxression and replaces all matchers. | ||
| funcreplaceMatchers(expr syntax.SampleExpr,matchers []*labels.Matcher) syntax.SampleExpr { |
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.
@ssncferreira and@chaudum, I've added you because I saw a mapper in your recent changes. I'm wondering if we should add a visitor pattern to the AST. 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.
I'm not a too big fan of the visitor pattern, because it can be much harder to understand than a the simpleWalk() we have atm. Is there an issue the visitor pattern could solve, thatWalk() can't?
It works 🎉 |
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.
LGTM
just improve the LogQL parsing.
Signed-off-by: JordanRushing <rushing.jordan@gmail.com>
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.
Looks great! I've drafted a quick test for thesliceToSet() util function:JordanRushing@8922c29 -- feel free to bring it in to this PR but not blocking.
What this PR does / why we need it:
This pull request will add tenant ID filtering such as
{__tenant_id__="1"}to multi-tenant queries. It does not add filter such as{app="loki"} | __tenant_id__="1".We are parsing the select logs or select samples queries and find the matcher. It is then removed and the non-matching tenants are ignored.
Which issue(s) this PR fixes:
Fixes#5820
Special notes for your reviewer:
Checklist
CHANGELOG.mdabout the changes.