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] add prevalence expanded section to expandable flyout #158606

Merged
merged 7 commits into from
Jun 13, 2023

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented May 26, 2023

Summary

This PR adds the Prevalence details section under the Insights section of the left expanded panel of the expandable flyout.

The PR introduces the following main changes:

  • the PrevalenceDetails placeholder component now retrieves the highlighted fields (using the same hook the PrevalenceOverview component was using, then displays a table with the calculated values
  • 2 new PrevalenceDetailsCountCell and PrevalenceDetailsPrevalenceCell components, that are using within the EuiBasicTable cell render methods
  • the above 2 cell components leverage a couple of hooks (useFetchUniqueByField, useFetchFieldValuePairWithAggregation and useFetchFieldValuePairByEventType) to fetch the data and do the necessary calculations. The hooks were very similar to the work done in the previous prevalence overview PR and were therefore made more generic and extracted to the shared folder at the root of the flyout folder. The hooks used within the right folder were replaced by these new generic ones

Some smaller changes were also made:

  • LeftFlyoutContext was renamed to LeftPanelContext to be consistent with the RightPanelContext used in the other folder

Data is being fetched using ReactQuery.

How to test

  • add xpack.securitySolution.enableExperimental: ['securityFlyoutEnabled'] to the kibana.json file
  • run yarn es snapshot --license trial, yarn test:generate and yarn start --no-base-path
  • go to the Alerts page, and click on the expand detail button on any row of the table
  • navigate to the Overview tab

How to generate correlations data

See the description of the previous PR and this comment to learn how to generate data.

Screenshot 2023-05-26 at 4 45 16 PM

https://github.com/elastic/security-team/issues/6423

TODO

  • implement e2e tests
  • verify interfaces and functions documentations

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-6423 branch 3 times, most recently from 75334bf to d722b0f Compare May 31, 2023 14:03
@PhilippeOberti PhilippeOberti marked this pull request as ready for review May 31, 2023 14:03
@PhilippeOberti PhilippeOberti requested review from a team as code owners May 31, 2023 14:03
@PhilippeOberti PhilippeOberti added release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.9.0 labels May 31, 2023
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-6423 branch from d722b0f to bc59e78 Compare May 31, 2023 19:19
Copy link
Contributor

@christineweng christineweng left a comment

Choose a reason for hiding this comment

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

Desk tested and it looks great!

I noticed the (%) in Host Prevalence (%) is cut off, unless i have kibana full screen AND on a ultra-wide monitor. In that case showing just numbers look a little off. I wonder if adding % to the numbers makes more sense to customers, wdyt?

image

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-6423 branch from bc59e78 to 45adce3 Compare June 7, 2023 14:54
@PhilippeOberti
Copy link
Contributor Author

Desk tested and it looks great!

I noticed the (%) in Host Prevalence (%) is cut off, unless i have kibana full screen AND on a ultra-wide monitor. In that case showing just numbers look a little off. I wonder if adding % to the numbers makes more sense to customers, wdyt?

I kept going back and fourth on this when I was developing it. I just added back the % in the cells directly and yeah it definitely looks better. Thanks!

Screenshot 2023-06-07 at 9 47 29 AM

Copy link
Contributor

@lgestc lgestc left a comment

Choose a reason for hiding this comment

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

looking good, one thing is that undefined returned inside LeftFlyoutContext, see my comment please:)

}

if (error) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

actually maybe we should return something here?:)

Copy link
Contributor Author

@PhilippeOberti PhilippeOberti Jun 7, 2023

Choose a reason for hiding this comment

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

I originally thought that having an empty cell would be ok, but I agree that showing some sort of error is better. I decided to return an error icon, let me know what you think... did you have something else in mind?
Screenshot 2023-06-07 at 2 40 43 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

this looks okay to me, but maybe @ferenrigue has some better idea:)

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-6423 branch 6 times, most recently from 2c8f2af to 64a8a5f Compare June 10, 2023 04:58
Copy link
Contributor

@christineweng christineweng left a comment

Choose a reason for hiding this comment

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

Desk tested and LGTM!

export const PREVALENCE_TABLE_ALERT_COUNT_COLUMN_TITLE = i18n.translate(
'xpack.securitySolution.flyout.prevalenceTableAlertCountColumnTitle',
{
defaultMessage: 'Alert Count',
Copy link
Contributor

@christineweng christineweng Jun 12, 2023

Choose a reason for hiding this comment

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

elastic styles prefer sentence case: i.e. 'Alert count'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, good point, fixed!

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-6423 branch 3 times, most recently from ce46f89 to d34e775 Compare June 12, 2023 18:22
- add doc
- lefft and right context more consistent
- add table column title translations
- more % to cells instead of column title
- remove unnecessary Storybook stories (analyzer graph and session view)
- replace as jest.Mock by jest.mocked where appropriate
- improve useMemo inside PrevalenceDetails
- cell components return error EuiIcon instead of null in case of errors
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #40 / Endpoint plugin test metadata apis list endpoints GET route "before all" hook for "should return one entry for each host with default paging"
  • [job] [logs] Defend Workflows Cypress Tests #1 / Endpoints page "before all" hook for "Loads the endpoints page"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4104 4108 +4

Async chunks

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

id before after diff
securitySolution 10.8MB 10.8MB +16.5KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 410 414 +4
total +6

References to deprecated APIs

id before after diff
securitySolution 607 609 +2

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 491 495 +4
total +6

History

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

@PhilippeOberti PhilippeOberti merged commit 39c68b5 into main Jun 13, 2023
@PhilippeOberti PhilippeOberti deleted the expanded-flyout-6423 branch June 13, 2023 22:17
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 13, 2023
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 release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants