Skip to content

[Unified Search] Supports complex filters with AND/OR relationships#143928

Merged
stratoula merged 224 commits intomainfrom
combined-filter-relation
Dec 19, 2022
Merged

[Unified Search] Supports complex filters with AND/OR relationships#143928
stratoula merged 224 commits intomainfrom
combined-filter-relation

Conversation

@nlatipov
Copy link
Copy Markdown
Contributor

@nlatipov nlatipov commented Oct 25, 2022

Describe the feature:

Closes #144775

This PR allows users to create more than one filter at a time. It enhances the query builder by enabling users to create multiple filters simultaneously. It adds the capability to nest queries and use the logical OR operator in filter pills.

image

Tasks:

  • Add the ability to add/edit multiple filters in one form:
    • Replace the current implementation of adding and editing a filter with a filtersBuilder - Vis-Editor;
    • Add combined filter support to Data plugin (mapAndFlattenFilters) - App-Services;
    • Add the ability to update data in the Data plugin when updating values ​​in the filters builder - App-Services;
    • Add hide Edit as Query DSL in popover case the filter inside FiltersBuilder is combinedFilter - App-Services;
  • Update filter badge to display nested filters:
    • Replace the current badge filter implementation with a new one - Vis-Editor;
    • Clean up FilterLabel component after replace FIlterBadge component - Vis-Editor;
  • When editing filters, those filters that belong to the same filter group should be edited - Vis-Editor;
  • Update jest and functional tests with new functionality - Vis-Editor;
  • Fix drag and drop behavior - Vis-Editor;

@nlatipov nlatipov changed the title Combined filter relation [Unified Search] Combined filter relation Oct 26, 2022
@nlatipov nlatipov self-assigned this Oct 26, 2022
@nlatipov nlatipov added release_note:skip Skip the PR/issue when compiling release notes Feature:Unified search Unified search related tasks v8.6.0 WIP Work in progress labels Oct 26, 2022
Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This looks great. Congrats for all this effort, it was hard I know! I tested:

  • Visualize with complex filters and add this ti the dashboard
  • TSVB with complex filters and add this ti the dashboard
  • Maps with complex filters and add this ti the dashboard
  • Vega with complex filters and add this ti the dashboard
  • Migration of dashboard with filters from 8.6
  • Migration of saved query from 8.6
  • Enable and disable the courier:ignore... setting
    Everything seems to work fine.

Before merging can we only fix this #143928 (comment)?

Also @lukasolson and @ppisljar are you also ok?

@walterra
Copy link
Copy Markdown
Contributor

I noticed an issue with saved searches when using the new filter feature:

  • In Discover I created a saved search based on a new filter
  • In the Machine Learning plugin I then used the saved search instead of a plain data view to access e.g. Data View Visualizer, Explain Log Rate Spikes etc.

The filter values show up as Warning like this:

image

It works if there's only one filter (also checked this works on main currently like this):

image

Note this only happens with saved searches, it works great as expected when creating new filters on those pages in the search bar on the fly.


Saved searches can also be used in Visualize Legacy, but the filter don't show up there, see in this GIF how they are missing from the Visualize Legacy view but reappear if you move to Discover:

@stratoula
Copy link
Copy Markdown
Contributor

Great find @walterra

In visualize works fine. Here is an unlinked saved search example, so I guess something goes wrong with the dataview in ml cc @Kunzetsov

image

@alexwizp
Copy link
Copy Markdown
Contributor

@walterra I think that the root cause of this problem lies in how the ML plugin works with SavedSearch saved object. We debugged this code and found that instead of indexPattern obj, indexPatternRef gets into the filter. This means that the references were not injected.

Let's move on to the ML code and x-pack/plugins/ml/public/application/util/index_utils.ts file. I'm interested in the getDataViewAndSavedSearch and loadSavedSearchById methods. I see that we are returning and working with raw value which is not correct

My recommendation is to use the getSavedSearch method from @kbn/saved-search-plugin/public. Using this method will ensure that you have created and are using the correct SavedSearch type object. And since this is not related to this PR, let's do it as part of a new issue

plus I see that with one filter, this also does not work quite right 😢

@qn895
Copy link
Copy Markdown
Member

qn895 commented Dec 16, 2022

My recommendation is to use the getSavedSearch method from @kbn/saved-search-plugin/public. Using this method will ensure that you have created and are using the correct SavedSearch type object. And since this is not related to this PR, let's do it as part of a new issue

Thanks @alexwizp for the hint! I've created a new issue for ML here with the issues surrounding saved searches. Tested other functionalities of the filters (global pins) and ML changes LGTM.

@Kuznietsov Kuznietsov requested review from andreadelrio and removed request for a team December 16, 2022 16:17
Copy link
Copy Markdown
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design changes LGTM, great job! Looking forward to iterating as needed. Pending: add Technical Preview badge.

@Kuznietsov Kuznietsov requested a review from a team as a code owner December 16, 2022 19:43
@timductive
Copy link
Copy Markdown
Member

Just adding a few comments here. Overall the experience is great, awesome work on this big PR.
I noticed a few minor nits, nothing blocking but perhaps some things to think about.

  1. There's no autocomplete on the Value field in the screenshot below. Obviously, I can guess that bytes should be numeric but if I try to type alpha characters nothing happens. It would be nice to either, have the kql dropdown with some proposed values or at least an error message when I attempt to input an invalid value.
    image

  2. I wonder if the "Edit filter" option should be at the top of the list here. I am much more frequently editing than pinning and I have to spend an extra second reading the options here.
    image

  3. Sometimes the Preview shows the infinity symbol and sometimes its text
    image
    image

  4. Also, can we detect if the value is datetime and prompt the value with a datepicker?
    image

  5. There's some funkiness with pinning and switching between Discover and Lens. I can maybe recreate over zoom but if i just pin and unpin a bunch and switch between the apps, it seems like the state is getting confused..
    Below in Lens i only have 1 pinned filter
    image
    Then I switch to Discover and I have 2 pinned filters
    image

@Kuznietsov
Copy link
Copy Markdown
Contributor

@elasticmachine merge upstream

@Kuznietsov
Copy link
Copy Markdown
Contributor

@timductive, thanks for your review.

  1. There's no autocomplete on the Value field in the screenshot below. Obviously, I can guess that bytes should be numeric but if I try to type alpha characters nothing happens. It would be nice to either, have the kql dropdown with some proposed values or at least an error message when I attempt to input an invalid value.

Autocomplete is possible only for keywords.

Screen.Recording.2022-12-19.at.09.43.33.mov

  1. Sometimes the Preview shows the infinity symbol and sometimes its text.

That is a formatter for values of datetime format. If we'll change that formatter, it will affect all the Kibana.


  1. There's some funkiness with pinning and switching between Discover and Lens. I can maybe recreate over zoom but if i just pin and unpin a bunch and switch between the apps, it seems like the state is getting confused..
    Below in Lens i only have 1 pinned filter

I guess, that bug is related to the behavior of filters and can be reproducible on main. We have decided to fix it in a separate PR.

Screen.Recording.2022-12-15.at.17.26.30.mov

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 458 459 +1
unifiedSearch 172 235 +63
total +64

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/es-query 170 187 +17
unifiedSearch 106 105 -1
total +16

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
@kbn/es-query 1 2 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 503.1KB 503.2KB +62.0B
securitySolution 12.6MB 12.6MB -168.0B
unifiedSearch 216.2KB 249.7KB +33.5KB
total +33.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 406.4KB 406.7KB +376.0B
kbnUiSharedDeps-srcJs 2.2MB 2.2MB +2.1KB
unifiedSearch 50.6KB 51.7KB +1.0KB
total +3.5KB
Unknown metric groups

API count

id before after diff
@kbn/es-query 227 244 +17

async chunk count

id before after diff
unifiedSearch 14 18 +4

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 61 67 +6
osquery 109 115 +6
securitySolution 439 445 +6
unifiedSearch 20 23 +3
total +23

References to deprecated APIs

id before after diff
@kbn/es-query 115 130 +15

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 70 76 +6
osquery 110 117 +7
securitySolution 516 522 +6
unifiedSearch 20 23 +3
total +24

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @andreadelrio @Kunzetsov @nlatipov

@stratoula
Copy link
Copy Markdown
Contributor

@timductive thanx a lot for taking the time to review it! To add to Yaroslav's comments:

I wonder if the "Edit filter" option should be at the top of the list here. I am much more frequently editing than pinning and I have to spend an extra second reading the options here.

We didn't do any changes here, we keep the logic that exists in main. I find a very good idea if @andreadelrio agrees. Def something we could consider an a follow up PR

can we detect if the value is datetime and prompt the value with a datepicker?

Same here, we keep the logic that exists in main. We coud detect it, I am quite certain. I find it a great idea tbh. I created a enhancement request here #147745

Copy link
Copy Markdown
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Investigations codeowners review. Thanks! LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Unified search Unified search related tasks loe:x-large Extra Large Level of Effort release_note:feature Makes this part of the condensed release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Unified search] Create complex filters (ANDs/ORs)