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] Deduplicate requests to ML #153244

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Mar 16, 2023

Resolves: #134826

Summary

We've recently had a couple of SDHs related to the slow performance of Security Solution. The performance issues were related to slow responses from ML API. In this PR, I rewrite data-fetching hooks to React Query to leverage request deduplication and caching. That significantly reduces the number of outgoing HTTP requests to ML routes on page load.

Before
9 HTTP requests to ML endpoints on initial page load, 5 of which are duplicates.

Screenshot 2023-03-20 at 12 09 55

After
4 HTTP requests to ML endpoints on initial page load, no duplicates.

Screenshot 2023-03-20 at 11 59 33

@xcrzx xcrzx added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team labels Mar 16, 2023
@xcrzx xcrzx requested review from a team as code owners March 16, 2023 14:49
@xcrzx xcrzx self-assigned this Mar 16, 2023
@xcrzx xcrzx requested a review from jpdjere March 16, 2023 14:49
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx marked this pull request as draft March 16, 2023 14:50
@xcrzx xcrzx added release_note:skip Skip the PR/issue when compiling release notes v8.8.0 labels Mar 16, 2023
@xcrzx xcrzx force-pushed the deduplicate-ml-requests branch from a2b63d9 to b610099 Compare March 16, 2023 14:59
@xcrzx xcrzx force-pushed the deduplicate-ml-requests branch from b610099 to a033d3d Compare March 20, 2023 10:58
@xcrzx xcrzx added performance technical debt Improvement of the software architecture and operational architecture labels Mar 20, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3783 3785 +2

Async chunks

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

id before after diff
securitySolution 15.8MB 15.8MB -367.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

  • 💔 Build #113830 failed b610099041f4e1d82df356efe31a179fe7c10cc1

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

cc @xcrzx

@xcrzx xcrzx marked this pull request as ready for review March 20, 2023 13:22
@banderror banderror requested review from machadoum and e40pud March 21, 2023 10:50
@banderror
Copy link
Contributor

@machadoum @e40pud FYI guys, please take a look if you're interested

@banderror
Copy link
Contributor

@xcrzx Does it fully resolve #134826, meaning that the duplicated ML requests was the last issue to fix in this ticket?

@banderror banderror added the Feature:Security ML Jobs Security Solution ML Jobs label Mar 21, 2023
@xcrzx
Copy link
Contributor Author

xcrzx commented Mar 21, 2023

@xcrzx Does it fully resolve #134826, meaning that the duplicated ML requests was the last issue to fix in this ticket?

@banderror Yes, that was the last one. We can close the ticket after merging this PR.

Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

LGTM! Tanks for fixing this.

@jpdjere
Copy link
Contributor

jpdjere commented Mar 23, 2023

Great changes! Thanks for this. LGTM 👍

@xcrzx xcrzx merged commit 9669cec into elastic:main Mar 23, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 23, 2023
@xcrzx xcrzx deleted the deduplicate-ml-requests branch March 23, 2023 13:11
Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

It looks great! 🎉

I left a tiny comment about a hook that returns a new array on every render. It would be nice to cache it to avoid unnecessary HTTP requests on the entity analytics page.

addError(error, { title: i18n.SIEM_JOB_FETCH_FAILURE });
}
}, [addError, error]);
const securityJobs = jobs.filter(isSecurityJob);
Copy link
Member

Choose a reason for hiding this comment

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

Could you move job filter logic to inside useMemo and prevent unnecessary renders?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Security ML Jobs Security Solution ML Jobs performance release_note:skip Skip the PR/issue when compiling release notes sdh-linked Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Rules management performance and scalability issues
8 participants