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] Fix performance issues affecting rules management #135311

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Jun 28, 2022

Summary

This PR fixes some of the performance issues identified here #134826.

  • Set the stale time of installed integration responses to 15 minutes, so we don't re-fetch them too frequently.
  • Refactor retrieval of the status of the prepackaged rules to use useQuery. That allows us to fetch the prepackaged rules statuses only once and re-fetch them only after server state mutations.
  • Refactored the installPrepackagedRules method to use the promise pool and respect the MAX_RULES_TO_UPDATE_IN_PARALLEL setting.
  • Wrapped some code paths in withSecuritySpan for better discoverability in APM.

Results

Before the rules management table took 950ms to render for 95% of users

Screenshot 2022-06-23 at 11 34 48

Now, the rules management table takes less than 300ms to render for 95% of users (3x faster)

Screenshot 2022-06-23 at 13 11 53

@xcrzx xcrzx added release_note:enhancement Feature:Detection Rules Security Solution rules and Detection Engine 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 v8.4.0 labels Jun 28, 2022
@xcrzx xcrzx self-assigned this Jun 28, 2022
@xcrzx xcrzx force-pushed the rules-management-perf-fixes branch 5 times, most recently from 982de5c to 5671e9c Compare June 30, 2022 08:59
@xcrzx xcrzx marked this pull request as ready for review July 5, 2022 15:54
@xcrzx xcrzx requested review from a team as code owners July 5, 2022 15:54
@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx requested review from banderror and YulNaumenko July 5, 2022 15:54
@elasticmachine
Copy link
Contributor

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

@xcrzx xcrzx removed request for banderror and YulNaumenko July 6, 2022 14:58
@xcrzx xcrzx marked this pull request as draft July 6, 2022 14:58
@xcrzx xcrzx marked this pull request as ready for review July 6, 2022 14:59
Comment on lines +54 to +61
/**
* We should use this hook to invalidate the prepackaged rules cache. For
* example, rule mutations that affect rule set size, like creation or deletion,
* should lead to cache invalidation.
*
* @returns A rules cache invalidation callback
*/
export const useInvalidatePrePackagedRulesStatus = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Appreciate the comment/context here. This is one of those things the team just needs to know about when working with mutations as it would be easy to add a mutation operation and not know you have to invalidate the PrePackagedStatus. If we guaranteed all rule access was going through a specific hook/API, we could invalidate at that layer, but not much else can be done, so thanks for the context here 👍

Comment on lines +73 to +83
const { data: prePackagedRulesStatus, isFetching } = usePrePackagedRulesStatus();
const { mutate: installPrePackagedRules, isLoading: loadingCreatePrePackagedRules } =
useInstallPrePackagedRules();

const createPrePackagedRules = useCallback(() => {
if (
canUserCRUD &&
hasIndexWrite &&
isAuthenticated &&
hasEncryptionKey &&
isSignalIndexExists
) {
installPrePackagedRules();
Copy link
Member

@spong spong Jul 7, 2022

Choose a reason for hiding this comment

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

The tech debt cleanup here is fantastic @xcrzx! Like finally paying off 150 year mortgage that has been following your family for generations, haha! 😅 Thanks for tackling this one -- so much cleaner and easier to grok the actual control flow with this, usePrePackagedRulesStatus, and useInstallPrePackagedRules. 💯💯💯

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, tested locally, and performed code review -- LGTM! 👍

Couple nits/questions around cache expiration timing (and will have to resolve conflicts with main coming from the recent schema changes), but everything else looks great @xcrzx! Thanks for the massive tech debt cleanup here, further APM instrumentation, and general perf cleanup as well. 🎉 🙌 🚀

@xcrzx xcrzx force-pushed the rules-management-perf-fixes branch 3 times, most recently from 427a3d1 to 99eb6a3 Compare July 8, 2022 13:13
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM!

@xcrzx xcrzx force-pushed the rules-management-perf-fixes branch 2 times, most recently from 05f7b35 to 759f0b2 Compare July 11, 2022 09:28
@xcrzx xcrzx enabled auto-merge (squash) July 11, 2022 09:31
@xcrzx xcrzx force-pushed the rules-management-perf-fixes branch from a32295b to 0278c47 Compare July 11, 2022 10:04
@xcrzx xcrzx force-pushed the rules-management-perf-fixes branch from 0278c47 to fcd603f Compare July 11, 2022 14:07
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3114 3116 +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 5.2MB 5.2MB -568.0B

History

  • 💔 Build #56576 failed 0278c47c0614a08a7af1c61aa0af6a30d65a07c2
  • 💚 Build #56360 succeeded 05f7b3589c7ccfbafad317b8d25705f391d5ce90
  • 💔 Build #56289 failed 99eb6a36236f575d5f5d08e56906adc1a20ce35c
  • 💔 Build #56245 failed 7dbedac78c0a515e859e91ebaadd407b335ed7a3
  • 💛 Build #54529 was flaky 5671e9ce71e4559530190d864304be6a5810cf77

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

cc @xcrzx

@xcrzx xcrzx merged commit c7a5b13 into elastic:main Jul 11, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 11, 2022
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:Detection Rules Security Solution rules and Detection Engine release_note:enhancement 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. v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants