[ResponseOps][Alerts] Fix DSL filter issues in search bar #193623
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
js-jankisalvi
added
release_note:skip
js-jankisalvi
changed the title
|
Pinging @elastic/response-ops (Team:ResponseOps) |
10 tasks
There was a problem hiding this comment.
@cnasikas nice catch! and thanks @js-jankisalvi for fixing it!
When I tested locally, the first filter was added correctly, but I wasn't able to add a second one. It worked fine in the Discover.
Here is what happens:
Screen.Recording.2024-09-25.at.17.19.13.mov
jcger
reviewed
Sep 26, 2024
|
jcger
reviewed
Sep 30, 2024
markov00
requested changes
Oct 8, 2024
There was a problem hiding this comment.
I believe there is something wrong with the current unified search custom DSL filter implementation and we should fix that behaviour instead of adding this changes that to me looks like a workaround.
Let me discuss this with the @elastic/kibana-visualizations team and we will be back with a solution
packages/kbn-alerts-ui-shared/src/rule_form/rule_actions/rule_actions_alerts_filter.test.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-alerts-ui-shared/src/alerts_search_bar/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-alerts-ui-shared/src/alerts_search_bar/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/kbn-alerts-ui-shared/src/rule_form/rule_actions/rule_actions_alerts_filter.test.tsx
Outdated
Show resolved
Hide resolved
| return { | ||
| $state, | ||
| meta, | ||
| query: filter?.query ? { ...filter.query } : { ...rest }, |
There was a problem hiding this comment.
Can you please explain what you are trying to achieve here?
Filter type has only 3 properties: $state, meta, query?.
When deconstructing a filter the rest parameter should be just the query param.
In this ternary we are saying that if filter.query doesn't exist then use filter.query that doesn't exist.
So I'm not sure what we are updating here.
It looks like the Filter objects are "hiding" something more hidden from the current Typing. This is definitely not great and cause tech debt that is hard to find
There was a problem hiding this comment.
On an subsequent check I see that the filter is currently constructed in the wrong way and in general it doesn't conform to its type: Filter
type Filter = {
$state?: {
store: FilterStateStore;
};
meta: FilterMeta;
query?: Record<string, any>;
};
The parameter query is not populated with the custom DSL, but the custom DSL always pollutes the rest of the object. I see this as a wrong implementation (not yours but the current unified search implementation) that needs to be fixed to make it work correctly in all cases.
IMHO what we are trying to push here is a workaround of a bigger problem, than an actual fix.
There was a problem hiding this comment.
The parameter query is not populated with the custom DSL, but the custom DSL always pollutes the rest of the object.
Exactly!
Above changes had to be done because when filters are updated via onFiltersUpdate, the DSL filter value is not wrapped in a query which throws validation error in the rule's action as it is expected to be a query object.
filter_query.bug.mov
IMHO what we are trying to push here is a workaround of a bigger problem, than an actual fix.
Agree, however as per my understanding not all plugins/ scenarios expects custom DSL value to be wrapped in a query object. Applying DSL filter to Alerts table works fine without query object.
Hence I fixed it wherever it was needed.
CC @cnasikas
There was a problem hiding this comment.
if you don't mind I'd like to see what could happen if I change your code like that: 9c8b5a3
let the CI run and see of this change trigger any error in the use of the unified search. This definitely work with your use case, but I'd like to check if this causes problem in the surrounding
Comment on lines
+122
to
+127
| const { $state, meta, ...rest } = filter; | ||
| return { | ||
| $state, | ||
| meta, | ||
| query: filter?.query ? { ...filter.query } : { ...rest }, | ||
| }; |
There was a problem hiding this comment.
Same comment as above: rest is query from the Filter type, so what are we trying to achieve here?
There was a problem hiding this comment.
same reason, validation fails without query object.
...triggers_actions_ui/public/application/sections/alerts_search_bar/alerts_search_bar.test.tsx
Outdated
Show resolved
Hide resolved
adcoelho
reviewed
Oct 8, 2024
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]Async chunks
Page load bundle
History
|
This reverts commit b663e2b.
…tersUpdated method
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11316832775 |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 13, 2024
…3623) ## Summary Resolves elastic#183908 Resolves elastic#192557 This PR fixes an issue of DSL filter in <details><summary>Search bar in Alerts page of Stack management</summary> <img width="1237" alt="image" src="https://github.com/user-attachments/assets/b9b380d2-80a7-4754-95f2-6b6831923c4d"> </details> <details><summary>Search bar in Alerts page of Observability</summary> <img width="1237" alt="image" src="https://github.com/user-attachments/assets/68ceb97f-b958-47f4-b356-757cbafd9170"> </details> <details><summary>Search bar in "If alerts matches query" action filter</summary> <img width="1281" alt="image" src="https://github.com/user-attachments/assets/1f99ca43-c4b5-4f52-b50f-914f1e205c5b"> </details> <details><summary>Search bar in Maintenance window page</summary> <img width="1272" alt="image" src="https://github.com/user-attachments/assets/ffaa317d-c14b-45a2-9d02-00f7a10239ab"> </details> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### How to verify - Create a DSL filter in above mentioned pages - Verify that filter is applied properly --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 122647b)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Oct 13, 2024
) (#196033) # Backport This will backport the following commits from `main` to `8.x`: - [[ResponseOps][Alerts] Fix DSL filter issues in search bar (#193623)](#193623) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Janki Salvi","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-13T18:42:04Z","message":"[ResponseOps][Alerts] Fix DSL filter issues in search bar (#193623)\n\n## Summary\r\n\r\nResolves https://github.com/elastic/kibana/issues/183908\r\nResolves https://github.com/elastic/kibana/issues/192557\r\n\r\nThis PR fixes an issue of DSL filter in\r\n<details><summary>Search bar in Alerts page of Stack\r\nmanagement</summary>\r\n<img width=\"1237\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/b9b380d2-80a7-4754-95f2-6b6831923c4d\">\r\n</details> \r\n\r\n<details><summary>Search bar in Alerts page of Observability</summary>\r\n<img width=\"1237\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/68ceb97f-b958-47f4-b356-757cbafd9170\">\r\n</details> \r\n\r\n<details><summary>Search bar in \"If alerts matches query\" action\r\nfilter</summary>\r\n<img width=\"1281\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/1f99ca43-c4b5-4f52-b50f-914f1e205c5b\">\r\n</details> \r\n\r\n<details><summary>Search bar in Maintenance window page</summary>\r\n<img width=\"1272\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/ffaa317d-c14b-45a2-9d02-00f7a10239ab\">\r\n</details> \r\n\r\n\r\n### Checklist\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### How to verify\r\n- Create a DSL filter in above mentioned pages\r\n- Verify that filter is applied properly\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"122647b7c286f5c6a429e73166ff2dd542779f04","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[ResponseOps][Alerts] Fix DSL filter issues in search bar","number":193623,"url":"https://github.com/elastic/kibana/pull/193623","mergeCommit":{"message":"[ResponseOps][Alerts] Fix DSL filter issues in search bar (#193623)\n\n## Summary\r\n\r\nResolves https://github.com/elastic/kibana/issues/183908\r\nResolves https://github.com/elastic/kibana/issues/192557\r\n\r\nThis PR fixes an issue of DSL filter in\r\n<details><summary>Search bar in Alerts page of Stack\r\nmanagement</summary>\r\n<img width=\"1237\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/b9b380d2-80a7-4754-95f2-6b6831923c4d\">\r\n</details> \r\n\r\n<details><summary>Search bar in Alerts page of Observability</summary>\r\n<img width=\"1237\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/68ceb97f-b958-47f4-b356-757cbafd9170\">\r\n</details> \r\n\r\n<details><summary>Search bar in \"If alerts matches query\" action\r\nfilter</summary>\r\n<img width=\"1281\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/1f99ca43-c4b5-4f52-b50f-914f1e205c5b\">\r\n</details> \r\n\r\n<details><summary>Search bar in Maintenance window page</summary>\r\n<img width=\"1272\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/ffaa317d-c14b-45a2-9d02-00f7a10239ab\">\r\n</details> \r\n\r\n\r\n### Checklist\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### How to verify\r\n- Create a DSL filter in above mentioned pages\r\n- Verify that filter is applied properly\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"122647b7c286f5c6a429e73166ff2dd542779f04"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193623","number":193623,"mergeCommit":{"message":"[ResponseOps][Alerts] Fix DSL filter issues in search bar (#193623)\n\n## Summary\r\n\r\nResolves https://github.com/elastic/kibana/issues/183908\r\nResolves https://github.com/elastic/kibana/issues/192557\r\n\r\nThis PR fixes an issue of DSL filter in\r\n<details><summary>Search bar in Alerts page of Stack\r\nmanagement</summary>\r\n<img width=\"1237\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/b9b380d2-80a7-4754-95f2-6b6831923c4d\">\r\n</details> \r\n\r\n<details><summary>Search bar in Alerts page of Observability</summary>\r\n<img width=\"1237\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/68ceb97f-b958-47f4-b356-757cbafd9170\">\r\n</details> \r\n\r\n<details><summary>Search bar in \"If alerts matches query\" action\r\nfilter</summary>\r\n<img width=\"1281\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/1f99ca43-c4b5-4f52-b50f-914f1e205c5b\">\r\n</details> \r\n\r\n<details><summary>Search bar in Maintenance window page</summary>\r\n<img width=\"1272\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/ffaa317d-c14b-45a2-9d02-00f7a10239ab\">\r\n</details> \r\n\r\n\r\n### Checklist\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### How to verify\r\n- Create a DSL filter in above mentioned pages\r\n- Verify that filter is applied properly\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"122647b7c286f5c6a429e73166ff2dd542779f04"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Janki Salvi <[email protected]>
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #183908
Resolves #192557
This PR fixes an issue of DSL filter in
Search bar in Alerts page of Stack management
Search bar in Alerts page of Observability
Search bar in "If alerts matches query" action filter
Search bar in Maintenance window page
Checklist
How to verify