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

Alerting: Prevent search from locking the browser #87132

Merged
merged 4 commits into from May 2, 2024

Conversation

gillesdemey
Copy link
Member

This PR prevents the following scenarios:

  1. very long search strings from exploding the regex permutations
  2. a large amount of search terms (whitespace separated) from locking the browser

this prevents two things
1. very long search strings from exploding the regex permutations
2. a large amount of search terms (whitespace separated) from locking the browser
@gillesdemey gillesdemey added this to the 11.1.x milestone Apr 30, 2024
@gillesdemey gillesdemey requested a review from a team as a code owner April 30, 2024 13:49
@gillesdemey gillesdemey requested review from tomratcliffe, konrad147 and soniaAguilarPeiron and removed request for a team April 30, 2024 13:49
@leeoniya
Copy link
Contributor

leeoniya commented Apr 30, 2024

i would suggest tweaking this further.

for the use case linked in the issue, searching for node_namespace_pod_container:container_cpu_usage_seconds_total:sum_irate was clearly copy-pasted. no one typed this out.

i would recommend detecting such cases and using a different uFuzzy instance configured with the default intraMode: 0 to avoid incurring the cost of trying to match that monstrosity while accounting for spelling mistakes -- intraMode: 1 will needlessly construct a huge regexp here, even without permutations.

@sidewinder12s
Copy link

i would suggest tweaking this further.

for the use case linked in the issue, searching for node_namespace_pod_container:container_cpu_usage_seconds_total:sum_irate was clearly copy-pasted. no one typed this out.

i would recommend detecting such cases and using a different uFuzzy instance configured with the default intraMode: 0 to avoid incurring the cost of trying to match that monstrosity while accounting for spelling mistakes -- intraMode: 1 will needlessly construct a huge regexp here, even without permutations.

Correct, I had copy pasted it when I ran into some of these locking issues.

gillesdemey and others added 2 commits May 2, 2024 12:07
Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

code changes LGTM!

this is a bit nicer to read and also makes it more functional
@gillesdemey gillesdemey self-assigned this May 2, 2024
@gillesdemey gillesdemey enabled auto-merge (squash) May 2, 2024 11:29
@gillesdemey gillesdemey merged commit 1d06f33 into main May 2, 2024
15 checks passed
@gillesdemey gillesdemey deleted the alerting/fix-search-oom branch May 2, 2024 11:41
@gillesdemey gillesdemey added backport v10.4.x backport v11.0.x Mark PR for automatic backport to v11.0.x labels May 2, 2024

This comment was marked as resolved.

This comment was marked as resolved.

@ryan-grafana
Copy link

Approved to back port

@ryan-grafana ryan-grafana added the product-approved Pull requests that are approved by product/managers and are allowed to be backported label May 2, 2024
@gillesdemey gillesdemey added backport v10.4.x backport v11.0.x Mark PR for automatic backport to v11.0.x and removed backport v10.4.x backport v11.0.x Mark PR for automatic backport to v11.0.x labels May 2, 2024
grafana-delivery-bot bot pushed a commit that referenced this pull request May 2, 2024
grafana-delivery-bot bot pushed a commit that referenced this pull request May 2, 2024
gillesdemey added a commit that referenced this pull request May 2, 2024
Alerting: Prevent search from locking the browser (#87132)

(cherry picked from commit 1d06f33)

Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
gillesdemey added a commit that referenced this pull request May 2, 2024
Alerting: Prevent search from locking the browser (#87132)

(cherry picked from commit 1d06f33)

Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/alerting Grafana Alerting area/frontend backport v10.4.x backport v11.0.x Mark PR for automatic backport to v11.0.x product-approved Pull requests that are approved by product/managers and are allowed to be backported type/bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants