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.

Sign up for GitHub

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

Jump to bottom

Elasticsearch: Implement modify query using a Lucene parser #71954

Merged
merged 11 commits into from Jul 28, 2023
Merged

Elasticsearch: Implement modify query using a Lucene parser #71954

merged 11 commits into from Jul 28, 2023

Conversation

matyax
Copy link
Contributor

@matyax matyax commented Jul 19, 2023 •
edited

In previous PRs we improved the ability to add and remove filters in Elasticsearch using Log Details controls. Now, we're changing the regex-driven implementation to one using a Lucene parser to read and modify the query.

Additional changes:

  • Removed the feature flag elasticToggleableFilters which is currently not in use, replaced with toggleLabelsInLogsUI which controls access to this feature.
  • We are now using Lucene to parse terms and values in filters.

Which issue(s) does this PR fix?:

Fixes #61765
Fixes #72008

Special notes for your reviewer:

  • Enable the feature flag elasticToggleableFilters.
  • Play with the 🔍 icons from the Log Details component.

@matyax matyax requested review from a team as code owners July 19, 2023 14:43
@matyax matyax requested review from jackw and removed request for a team July 19, 2023 14:43
@matyax matyax added add to changelog no-backport Skip backport of PR labels Jul 19, 2023
@matyax matyax added this to the 10.1.x milestone Jul 19, 2023
@matyax
Copy link
Contributor Author

matyax commented Jul 20, 2023

BTW I will deal with the merge conflicts after the review because depending on how long it takes I might have to do it more than once otherwise.

@matyax matyax requested a review from gwdawson July 24, 2023 08:37
@matyax
Copy link
Contributor Author

matyax commented Jul 24, 2023

@gwdawson This could be a fun PR to review.

gwdawson
gwdawson approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@gwdawson gwdawson left a comment

Choose a reason for hiding this comment

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

i would like to see a couple of tests for removeNodeFromTree but other than that, it LGTM!
i think this is a great improvement. 🎉

ivanahuckova
ivanahuckova requested changes Jul 25, 2023
View reviewed changes
Copy link
Member

@ivanahuckova ivanahuckova left a comment •
edited

Choose a reason for hiding this comment

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

I tried to add this test related to whitespace between : - expect(queryHasFilter('(this:"that" OR test :test) AND label:value', 'test', 'test')).toBe(true);, but it returns false. It should return true, right?

@matyax
Copy link
Contributor Author

matyax commented Jul 25, 2023

We need to clear up those spaces, otherwise the parser fails, so probably. Will look into it. Thanks!

@matyax
Copy link
Contributor Author

matyax commented Jul 25, 2023

I tried to add this test related to whitespace between : - expect(queryHasFilter('(this:"that" OR test :test) AND label:value', 'test', 'test')).toBe(true);, but it returns false. It should return true, right?

According to the parser, that is not a valid query. We can workaround it with the normalization, but maybe we shouldn't support queries written in that way.

As for now, the code has been updated to support it.

@github-actions github-actions bot added the pr/external This PR is from external contributor label Jul 25, 2023
@matyax matyax requested review from grafanabot and a team as code owners July 25, 2023 14:01
@matyax matyax requested review from leandro-deveikis and removed request for a team July 25, 2023 14:01
@matyax matyax requested a review from mmandrus July 25, 2023 14:01
@matyax matyax requested a review from ivanahuckova July 25, 2023 14:02
@matyax
Copy link
Contributor Author

matyax commented Jul 25, 2023 •
edited

i would like to see a couple of tests for removeNodeFromTree

removeFilterFromQuery test cases are taking care of covering that code.

@github-actions
Copy link
Contributor

Backend code coverage report for PR #71954
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023 •
edited

Frontend code coverage report for PR #71954

Plugin Main PR Difference
elasticsearch 83.04% 83.08% .04%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023 •
edited

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)

Console output
Read our guideline

@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Jul 25, 2023
@gwdawson gwdawson mentioned this pull request Jul 27, 2023
Closed
@matyax matyax mentioned this pull request Jul 28, 2023
Closed
@matyax matyax merged commit 0d121ba into main Jul 28, 2023
19 of 20 checks passed
@matyax matyax deleted the matyax/elastic-query-parser branch July 28, 2023 12:49
@grafana-delivery-bot grafana-delivery-bot bot modified the milestones: 10.1.x, 10.2.x Jul 28, 2023
@matyax matyax mentioned this pull request Jul 31, 2023
Closed
sarahzinger pushed a commit that referenced this pull request Aug 1, 2023
@matyax @sarahzinger
Elasticsearch: Implement modify query using a Lucene parser (#71954)
f3dcf4d 
* Lucene: add dependency

* ModifyQuery: use Lucene parser to detect key:values in queries

* ModifyQuery: use Lucene parser to remove filters

* Remove test code

* Modify query: switch to recursive implementation

* Modify query: implement remove filter

* Update query normalizing function

* FlagElasticToggleableFilters: remove feature flag

* Remove unused feature flag from test

* Elasticsearch: escape quotes in filter values
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
@matyax @chauchausoup
Elasticsearch: Implement modify query using a Lucene parser (grafana#…
650f2a5 
…71954)

* Lucene: add dependency

* ModifyQuery: use Lucene parser to detect key:values in queries

* ModifyQuery: use Lucene parser to remove filters

* Remove test code

* Modify query: switch to recursive implementation

* Modify query: implement remove filter

* Update query normalizing function

* FlagElasticToggleableFilters: remove feature flag

* Remove unused feature flag from test

* Elasticsearch: escape quotes in filter values
@matyax matyax mentioned this pull request Oct 13, 2023
Merged
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
@dnhn dnhn mentioned this pull request Oct 24, 2023
Closed
@BrewTestBot BrewTestBot mentioned this pull request Oct 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanahuckova ivanahuckova ivanahuckova approved these changes

@gwdawson gwdawson gwdawson approved these changes

@jackw jackw Awaiting requested review from jackw jackw is a code owner automatically assigned from grafana/frontend-ops

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

@leandro-deveikis leandro-deveikis Awaiting requested review from leandro-deveikis leandro-deveikis was automatically assigned from grafana/grafana-operator-experience-squad

@mmandrus mmandrus Awaiting requested review from mmandrus mmandrus was automatically assigned from grafana/grafana-operator-experience-squad

Assignees
No one assigned
Labels
add to changelog area/backend area/frontend datasource/Elasticsearch levitate breaking change A label indicating a breaking change and assigned by Levitate. no-backport Skip backport of PR pr/external This PR is from external contributor type/docs
Projects
None yet
Milestone
10.2.0
Development

Successfully merging this pull request may close these issues.

Dashboards: Adhoc filter does not work with double quotes Logs: Easier search and filter from log lines
5 participants
@matyax @ivanahuckova @gwdawson @zerok @grafanabot