[Security Solution] expandable flyout - fix infinite loop in correlations

[PhilippeOberti]

Summary

QA found a nasty infinite loop bug in the correlation overview and details section of the expandable flyout. It only occurs in certain scenario, where the document has some specific data.
The problem comes from the fact that the useCorrelations hook is fetching the data in all scenarios, instead of taking into account wether or not we should actually fetch that data.

This PRs separates the useCorrelations hooks into 4 components for the right panel and another 4 components for the left panel, which each call their respective useFetch... hooks (useShowRelatedAlertsByAncestry, useShowRelatedAlertsBySameSourceEvent, useShowRelatedAlertsBySession and useShowRelatedCases). If the return value is true, the we fetch the data using the respective useFetch... hooks (useFetchRelatedAlertsByAncestry, useFetchRelatedAlertsBySameSourceEvent, useFetchRelatedAlertsBySession and useFetchRelatedCases).

Previous behavior

Screen.Recording.2023-08-15.at.12.54.26.PM.mov

New behavior

Screen.Recording.2023-08-15.at.12.56.48.PM.mov

The PR also does the following:

• clean up a bit a couple of test_ids and translations files by making some reusable methods to generate values instead of having many individual constants
• merge the correlations_cases_table and new related_cases files

The following file contains the a devtool console command to ingest an alert to reproduce the infinite loop
alert_infinite_loop_correlations.txt

#164394

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios

[christineweng]

The separated components look very similar, almost identical. i understand that it's because we don't want to fetch data when we don't need to. I think we can still achieve this at a higher level by adding a skip prop to the data fetching hooks.

const showAlertsByAncestry = useShowRelatedAlertsByAncestry(...);
const { loading, error, data, dataCount } = useFetchRelatedAlertsByAncestry({
    dataFormattedForFieldBrowser,
    scopeId,
    skip: !showAlertsByAncestry
  });

[PhilippeOberti]

so I implemented this approach then finally decided against it, for 2 reasons:

• the first one is some hooks we're using have that open but others don't. I tried to add it but then got scared of modifying somebody else's code after feature freeze and during BC time, which is super risky
• using this skip doesn't prevent some of the code in our hooks and the hooks we're leveraging to run some of their code, which isn't super efficient

Therefore I ended up going with a route similar to what I had shown yesterday, but a bit simpler and easier to read. I hope you'll be ok with it!

[christineweng]

I investigated this alert as well and found the following reasons that it was stuck in infinite loop:

1. alertsBySessionLoading is always true

   • when kibana.alert.original_event.id is not available, values fall back to an empty array. Despite being empty, it still triggers query building on this line and tries to run the dummy query.
   • Removing the fall back would fix it. same goes to this line

2. ancestryAlertsLoading is always true

   • this line should prevent the query from running if either id or schema is null - the alert in question returns null for both id and schema
   • there is a known issue with react query v4 that query.isLoading could be true when query is not enabled, so it was stuck in this line
   • query.isFetching or (enabled && query.isLoading) equals the true isLoading

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: f0cea46

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	4438	4444	+6

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.7MB	15.7MB	-3.6KB

History

• 💔 Build #152455 failed d3747db9b65f050b523db8490d663051509fc910
• 💔 Build #152328 failed 3d0c6717107947bbea4f40f342332f92341edc98
• 💔 Build #152250 failed f8e887f9b377d6862632fe4046106f95789ec152
• 💔 Build #151177 failed ff81273
• 💔 Build #150839 failed debbc33b48b25a06d1c8a4952a8d15cb4a94c4c1

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

[christineweng]

LGTM! Tested against the trouble alert and it works as expected.

One suggestion I have is to consolidate the 4 show.. hooks into 1. Most of the checks are very simple. Then we can have a boolean that checks if any insights can be shown, similar to this line

const canShowAtLeastOneInsight =
      showRelatedCases ||
      showSameSourceEvents ||
      showAlertsByAncestry ||
      showAlertsBySession;

[PhilippeOberti]

One suggestion I have is to consolidate the 4 show.. hooks into 1. Most of the checks are very simple. Then we can have a boolean that checks if any insights can be shown, similar to this line

Good idea, I'm implementing this in my next PR to save some build time and save time on rebase for the next 2-3 PRs!

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.10

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation