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

Fixes auto suggest in security solution #145924

Merged
merged 6 commits into from
Nov 28, 2022

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Nov 21, 2022

Resolves #144229

Security solution does store the full DataView object on the client side of the siem app for performance reasons. The addition of the line return indexPattern instance of DataView to unified search breaks auto suggest kql query bar across all of security solution. By changing the check to only check for the necessary property indexPattern.fields, everything functions as expected again.

  • Fixes auto suggest bug
  • Adds a cypress test in security solution to confirm the fix

@stephmilovic stephmilovic added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Solution Platform Security Solution Platform Team v8.6.0 v8.7.0 labels Nov 21, 2022
@stephmilovic stephmilovic changed the title Fixes auto complete in security solution Fixes auto suggest in security solution Nov 21, 2022
@stephmilovic stephmilovic marked this pull request as ready for review November 21, 2022 21:32
@stephmilovic stephmilovic requested review from a team as code owners November 21, 2022 21:32
@elasticmachine
Copy link
Contributor

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

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.

This change looks good to me, @lukasolson wdyt?

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

Thanks Steph!! The test added LGTM. Just left a couple of nit comments.

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Hmm, just from a quick check of the autocomplete logic it looks like we need to make sure the data view object has other fields as well, such as title and timeFieldName (and possibly others). What is the data view in security solution an instance of? Maybe for now we could at least check for title as well. @mattkime Any thoughts on how to verify that the object here is data view-like?

@mattkime
Copy link
Contributor

I’d prefer to move away from using data view like objects. Perhaps security solution could create an ad hoc data view and skip loading the field list.

that said, I know that this need to ship quickly and I don’t want to slow this down

@stephmilovic
Copy link
Contributor Author

I’d prefer to move away from using data view like objects. Perhaps security solution could create an ad hoc data view and skip loading the field list.

@mattkime @lukasolson
we already have the field list. all that we have is fields and title. We do not have timeFieldName, wondering if we need to require that? if not passed, we can default to @timestamp

@dhurley14
Copy link
Contributor

@lukasolson @mattkime I briefly mention some complexity issues here. I think there is definitely an interest in moving as much of sourcerer to use ad-hoc data views as possible, as is the case with the effort in this draft PR by @YulNaumenko . However, after spending some time investigating how we could implement in-memory data views in sourcerer before reaching out to @mattkime, it became apparent that we would not have the time to implement these changes in sourcerer. At this stage of the release process I am afraid the necessary changes to accomplish a move to in-memory data views for sourcerer will introduce too many changes to the security solution and we will not have the capacity and requisite time to cover all edge cases where code depending on properties from the SourcererDataView type will need to be migrated to corresponding properties on the DataView class. I think the ask here is if it is possible to reduce the partitioning logic such that it is only checking for properties the unified search bar requires to function?

@stephmilovic
Copy link
Contributor Author

stephmilovic commented Nov 23, 2022

Just found/fixed this related bug. Thinking we should check for type and value properties as well?

FWIW, from what I can tell value is not even on the DataView class that unified search is checking:

return indexPattern instanceof KibanaDataView

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.

From Lukas' comment I understand that title and fields is sufficient. I would really like to understand why this doesn't work on security solution, is the dataviews not an instance of KibanaDataview?

I am approving as I understand it is a significant bug!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #4 / All exception lists - read only "before all" hook for "Displays missing privileges primary callout"

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
unifiedSearch 216.1KB 216.2KB +20.0B

Page load bundle

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

id before after diff
unifiedSearch 50.7KB 50.5KB -140.0B
Unknown metric groups

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 59 65 +6
osquery 109 115 +6
securitySolution 442 448 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 519 525 +6
total +21

History

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

@stephmilovic stephmilovic merged commit 3a6825c into elastic:main Nov 28, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 28, 2022
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.6

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 Nov 28, 2022
# Backport

This will backport the following commits from `main` to `8.6`:
- [Fixes auto suggest in security solution
(#145924)](#145924)

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

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

<!--BACKPORT [{"author":{"name":"Steph
Milovic","email":"stephanie.milovic@elastic.co"},"sourceCommit":{"committedDate":"2022-11-28T16:52:03Z","message":"Fixes
auto suggest in security solution
(#145924)","sha":"3a6825cf47e80dab9aeae373a1a93ab740e733eb","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:
SecuritySolution","Team:Security Solution
Platform","v8.6.0","v8.7.0"],"number":145924,"url":"https://github.com/elastic/kibana/pull/145924","mergeCommit":{"message":"Fixes
auto suggest in security solution
(#145924)","sha":"3a6825cf47e80dab9aeae373a1a93ab740e733eb"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145924","number":145924,"mergeCommit":{"message":"Fixes
auto suggest in security solution
(#145924)","sha":"3a6825cf47e80dab9aeae373a1a93ab740e733eb"}}]}]
BACKPORT-->

Co-authored-by: Steph Milovic <stephanie.milovic@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform Security Solution Platform Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.6.0 v8.7.0
Projects
None yet
10 participants