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

[Security Solution][Detection Engine] hides 'run query' label on ES|QL rule creation #167912

Merged
merged 7 commits into from
Oct 6, 2023

Conversation

vitaliidm
Copy link
Contributor

@vitaliidm vitaliidm commented Oct 3, 2023

Summary

  • hides Run query ⌘ + Enter label on ES|QL text editor when create/edit rule
  • On unified search side, passes property hideTextBasedRunQueryLabel to underlying TextBasedLangEditor component

Before

Screenshot 2023-10-03 at 17 16 09

After

Screenshot 2023-10-03 at 17 16 45

@vitaliidm vitaliidm self-assigned this Oct 3, 2023
@vitaliidm vitaliidm added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Engine Security Solution Detection Engine Area labels Oct 4, 2023
@vitaliidm vitaliidm marked this pull request as ready for review October 4, 2023 09:49
@vitaliidm vitaliidm requested review from a team as code owners October 4, 2023 09:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@stratoula
Copy link
Contributor

stratoula commented Oct 4, 2023

@vitaliidm I am sorry I am not very familiar with your code. Why are you using the unified search to render the ESQL editor and not immediately the editor?

As it is done in x-pack/plugins/stack_alerts/public/rule_types/es_query/expression/esql_query_expression.tsx

The editor is on it own plugin so it should be used instead of the unified search component.

@vitaliidm
Copy link
Contributor Author

vitaliidm commented Oct 4, 2023

@stratoula

We are already using unified search component in our rule form:

import { SearchBar } from '@kbn/unified-search-plugin/public';

for various rule types: query, new terms, threshold, etc. Since this component(unified search SearchBar) supports ES|QL and we got a lot of Security Solution logic build on top of SearchBar from '@kbn/unified-search-plugin/public' in this security_solution/public/common/components/query_bar/index.tsx. And we use other functionality that exist in the unified search component, not only query input, but filters as well. So if any of this will be added for ES|QL we would have support of it, instead of building our own in Security

Thats why, the decision was to use the same component, instead of importing new one and integrate it into the Security Solution.

@stratoula
Copy link
Contributor

I don't think we are going to add filters in ES|QL as filtering is already provided in the language. Unified search offers the editor but only as part of it (if you have the dataview picker and click Try it).

I would like to avoid using the unified search for rendering only the editor because then if we need more customizations (as the one you are proposing here) that have nothing to do with the unified search component then we need to pass all these props to unified search.

Is it a big change for you to render the editor instead for this case? We can sync and check it together if you want.

@vitaliidm
Copy link
Contributor Author

I don't think we are going to add filters in ES|QL as filtering is already provided in the language. Unified search offers the editor but only as part of it (if you have the dataview picker and click Try it).
I would like to avoid using the unified search for rendering only the editor because then if we need more customizations (as the one you are proposing here) that have nothing to do with the unified search component then we need to pass all these props to unified search.
Is it a big change for you to render the editor instead for this case? We can sync and check it together if you want.

If the only concern are customizations, if any new arise, we can reconsider to use ES|QL text editor directly again.
But at this point, I don't anticipate any new ones. We pretty much happy to be consistent with unified search

For us, we would like to be synchronized with the functionality available in unified search component, as this something we are getting for free and don't need to re-implement on our side. For example: selecting languages, using filters, using saving queries and so on. Instead of supporting 2 components, where one of them already built-in in another and available inside it.

Similarly, ES|QL also used in timeline: #166764, within unified search component, so it's not a unique use case

Happy to sync if needed

Copy link
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.

We synced offline, it is actually a usage of unified search (they want to click the editor and expand it exactly as it happens in Discover) so this property makes sense.

In the future if we decide to do more customizations we might need to organize the editor customization properties in an object property but I think it is ok for now.

@vitaliidm vitaliidm added backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.12.0 labels Oct 6, 2023
@vitaliidm vitaliidm enabled auto-merge (squash) October 6, 2023 09:19
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #24 / APM API tests alerts/transaction_error_rate.spec.ts basic no archive transaction error rate alert create rule with kql query "after all" hook for "shows alert count=1 in opbeans-node service"

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 13.0MB 13.0MB +150.0B
textBasedLanguages 148.7KB 148.7KB +19.0B
unifiedSearch 215.2KB 215.3KB +111.0B
total +280.0B

History

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

cc @vitaliidm

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2023
…L rule creation (elastic#167912)

## Summary

- hides **Run query ⌘ + Enter** label on ES|QL text editor when
create/edit rule
- On unified search side, passes property `hideTextBasedRunQueryLabel`
to underlying `TextBasedLangEditor` component

### Before

<img width="1012" alt="Screenshot 2023-10-03 at 17 16 09"
src="https://github.com/elastic/kibana/assets/92328789/8a063167-fd78-4afd-8e2a-0f1168e5a7eb">

### After

<img width="1099" alt="Screenshot 2023-10-03 at 17 16 45"
src="https://github.com/elastic/kibana/assets/92328789/59819074-aab3-4367-a398-739f718368d0">

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
(cherry picked from commit 01fdd67)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.11

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 6, 2023
…on ES|QL rule creation (#167912) (#168205)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Security Solution][Detection Engine] hides 'run query' label on
ES|QL rule creation
(#167912)](#167912)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Vitalii
Dmyterko","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-10-06T10:50:28Z","message":"[Security
Solution][Detection Engine] hides 'run query' label on ES|QL rule
creation (#167912)\n\n## Summary\r\n\r\n- hides **Run query ⌘ + Enter**
label on ES|QL text editor when\r\ncreate/edit rule\r\n- On unified
search side, passes property `hideTextBasedRunQueryLabel`\r\nto
underlying `TextBasedLangEditor` component\r\n\r\n### Before\r\n\r\n<img
width=\"1012\" alt=\"Screenshot 2023-10-03 at 17 16
09\"\r\nsrc=\"https://github.com/elastic/kibana/assets/92328789/8a063167-fd78-4afd-8e2a-0f1168e5a7eb\">\r\n\r\n###
After\r\n\r\n<img width=\"1099\" alt=\"Screenshot 2023-10-03 at 17 16
45\"\r\nsrc=\"https://github.com/elastic/kibana/assets/92328789/59819074-aab3-4367-a398-739f718368d0\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Stratoula Kalafateli
<[email protected]>","sha":"01fdd674891132ea9f3cac14e92b9933d3cbe53a","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Detections
and Resp","Team: SecuritySolution","backport:prev-minor","Team:Detection
Engine","v8.11.0","v8.12.0"],"number":167912,"url":"https://github.com/elastic/kibana/pull/167912","mergeCommit":{"message":"[Security
Solution][Detection Engine] hides 'run query' label on ES|QL rule
creation (#167912)\n\n## Summary\r\n\r\n- hides **Run query ⌘ + Enter**
label on ES|QL text editor when\r\ncreate/edit rule\r\n- On unified
search side, passes property `hideTextBasedRunQueryLabel`\r\nto
underlying `TextBasedLangEditor` component\r\n\r\n### Before\r\n\r\n<img
width=\"1012\" alt=\"Screenshot 2023-10-03 at 17 16
09\"\r\nsrc=\"https://github.com/elastic/kibana/assets/92328789/8a063167-fd78-4afd-8e2a-0f1168e5a7eb\">\r\n\r\n###
After\r\n\r\n<img width=\"1099\" alt=\"Screenshot 2023-10-03 at 17 16
45\"\r\nsrc=\"https://github.com/elastic/kibana/assets/92328789/59819074-aab3-4367-a398-739f718368d0\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Stratoula Kalafateli
<[email protected]>","sha":"01fdd674891132ea9f3cac14e92b9933d3cbe53a"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/167912","number":167912,"mergeCommit":{"message":"[Security
Solution][Detection Engine] hides 'run query' label on ES|QL rule
creation (#167912)\n\n## Summary\r\n\r\n- hides **Run query ⌘ + Enter**
label on ES|QL text editor when\r\ncreate/edit rule\r\n- On unified
search side, passes property `hideTextBasedRunQueryLabel`\r\nto
underlying `TextBasedLangEditor` component\r\n\r\n### Before\r\n\r\n<img
width=\"1012\" alt=\"Screenshot 2023-10-03 at 17 16
09\"\r\nsrc=\"https://github.com/elastic/kibana/assets/92328789/8a063167-fd78-4afd-8e2a-0f1168e5a7eb\">\r\n\r\n###
After\r\n\r\n<img width=\"1099\" alt=\"Screenshot 2023-10-03 at 17 16
45\"\r\nsrc=\"https://github.com/elastic/kibana/assets/92328789/59819074-aab3-4367-a398-739f718368d0\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Stratoula Kalafateli
<[email protected]>","sha":"01fdd674891132ea9f3cac14e92b9933d3cbe53a"}}]}]
BACKPORT-->

Co-authored-by: Vitalii Dmyterko <[email protected]>
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
…L rule creation (elastic#167912)

## Summary

- hides **Run query ⌘ + Enter** label on ES|QL text editor when
create/edit rule
- On unified search side, passes property `hideTextBasedRunQueryLabel`
to underlying `TextBasedLangEditor` component

### Before

<img width="1012" alt="Screenshot 2023-10-03 at 17 16 09"
src="https://github.com/elastic/kibana/assets/92328789/8a063167-fd78-4afd-8e2a-0f1168e5a7eb">

### After

<img width="1099" alt="Screenshot 2023-10-03 at 17 16 45"
src="https://github.com/elastic/kibana/assets/92328789/59819074-aab3-4367-a398-739f718368d0">

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
@vitaliidm vitaliidm deleted the detection-engine/hide-run-query branch March 4, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Detection Engine Security Solution Detection Engine Area Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.11.0 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants