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

check for stream selectors to have atleast one equality matcher #3216

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does / why we need it:
To avoid users doing a query of death by selecting all the streams or very broad queries we want to make users have at least one equality matcher in the selectors.

Special notes for your reviewer:
We also consider foo=~"bar|baz" as an equality matcher since it is internally converted to equality matchers.

Checklist

  • Tests updated

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

The code looks good, but I'm not sure about our goal here.

For instance, this seems a valid query, but we'd limit it: {job=~"loki-prod.*/querier"}.

What if we instead limited queries which had no matchers, such as returned by the cortex function SplitFiltersAndMatchers?

That would disallow queries such as {job=~".*"} but allow ones like {job=~".+"}

pkg/querier/querier_test.go Outdated Show resolved Hide resolved
pkg/querier/querier.go Outdated Show resolved Hide resolved
@sandeepsukhani
Copy link
Contributor Author

The code looks good, but I'm not sure about our goal here.

For instance, this seems a valid query, but we'd limit it: {job=~"loki-prod.*/querier"}.

What if we instead limited queries which had no matchers, such as returned by the cortex function SplitFiltersAndMatchers?

That would disallow queries such as {job=~".*"} but allow ones like {job=~".+"}

Thats interesting. This would mean we would only allow matchers which would not make us fetch all the chunks for the whole query duration. I am not against this but I think it would be hard to explain it to the users because {job="foo"} would work but {job=""} will not.

@owen-d
Copy link
Member

owen-d commented Feb 10, 2021

For an error message, what about: Queries require at least one regexp or equality matcher that does not have an empty-compatible value. For instance, app=~".*" does not meet this requirement, but app=~".+" will.

@sandeepsukhani sandeepsukhani force-pushed the stream-selector-atleast-one-equality-matcher branch from 5c0339a to f35fe44 Compare February 16, 2021 08:47
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 16, 2021
@sandeepsukhani sandeepsukhani force-pushed the stream-selector-atleast-one-equality-matcher branch from f35fe44 to 5a1f2f1 Compare February 16, 2021 09:06
@sandeepsukhani sandeepsukhani force-pushed the stream-selector-atleast-one-equality-matcher branch from 5a1f2f1 to e849251 Compare February 16, 2021 09:10
@stale
Copy link

stale bot commented Mar 19, 2021

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Mar 19, 2021
@sandeepsukhani sandeepsukhani added the keepalive An issue or PR that will be kept alive and never marked as stale. label Mar 20, 2021
@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Mar 20, 2021
@owen-d owen-d merged commit b152bc2 into grafana:master Mar 30, 2021
cyriltovena added a commit to cyriltovena/loki that referenced this pull request Mar 31, 2021
Minor refactoring missing.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
cyriltovena added a commit that referenced this pull request Mar 31, 2021
Minor refactoring missing.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
cyriltovena added a commit to cyriltovena/loki that referenced this pull request Mar 31, 2021
1- It currently 500.
2- Happens too late, at ingester level and querier. (better at the frontend)
3- Improve API surface level, we don't need to expose this validation for anything else than ParseLogSelector.

see grafana#3216

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
cyriltovena added a commit that referenced this pull request Mar 31, 2021
1- It currently 500.
2- Happens too late, at ingester level and querier. (better at the frontend)
3- Improve API surface level, we don't need to expose this validation for anything else than ParseLogSelector.

see #3216

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive An issue or PR that will be kept alive and never marked as stale. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants