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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Coverage Overview Dashboard #161556

Merged
merged 27 commits into from
Aug 8, 2023
Merged

[Security Solution] Coverage Overview Dashboard #161556

merged 27 commits into from
Aug 8, 2023

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Jul 10, 2023 •
edited
Loading

Summary

Addresses #158243

Creates first stage of the coverage overview dashboard and integrates it with the corresponding API.

Screenshots

Screenshot 2023-07-27 at 9 51 12 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team v8.10.0 labels Jul 10, 2023
@dplumlee dplumlee self-assigned this Jul 10, 2023
@dplumlee dplumlee marked this pull request as ready for review July 28, 2023 02:50
@dplumlee dplumlee requested a review from a team as a code owner July 28, 2023 02:50
@dplumlee dplumlee requested review from xcrzx and banderror July 28, 2023 02:50
@banderror banderror requested review from maximpn and removed request for banderror July 31, 2023 13:47
@banderror banderror added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area labels Jul 31, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@kibanamachine kibanamachine requested a review from a team as a code owner July 31, 2023 19:06
maximpn
maximpn reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@maximpn maximpn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@dplumlee thank you for implementing such a complex dashboard 👍

I like that you reused EUI components as much as possible to avoid building custom components and properly split the business logic between the components. It's clear what component responsible for what functionality.

Some time was spend to test the implementation so I revealed some problems

  • Enable All Disabled rules does nothing. Basically it should be quite straightforward to implement this functionality via bulk actions. It feel like it should be part of this PR. If you wanna split it just don't forget to add a note about it.
  • Dashboard's page opens with a delay when clicked on the dashboard's card on the security's dashboard page. It's better to double check on the cause as if it takes too much time without any visual updates users may complain. Or it can tell about performance issues.
  • There is no visual loading state when loading the page and switching between collapsed and expanded state. Switching doesn't happed instantly and may be an issue.
  • Everything related to available rules should be removed from UI as it's not required for milestone 1.

On top of that I've noticed

  • memo() component's wrapper used with useMemo() but it's unclear if it gives some improvement. I'd verify it as we may have a performance issue.
  • styled-components used for custom styles instead of emotion. As EUI plans to use only emotion we could follow the same way and avoid styled-component in the new code for now.
  • Unit tests testing the component renders don't actually give good coverage. Let's remove them or add useful scenarios.

...ty_solution/public/detection_engine/rule_management/api/hooks/use_fetch_coverage_overview.ts
*
* @returns A coverage overview cache invalidation callback
*/
export const useInvalidateFetchCoverageOverviewQuery = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

useInvalidateFetchCoverageOverviewQuery has to be call upon rule creation (prebuilt rules installation), deletion, enabling and disabling as it impacts the dashboard. On top of that rule editing may change MITRE ATT&CK bindings so it also should be taken into account.

For example check where useInvalidateFindRulesQuery used so it will help to find proper places for coverage overview invalidation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think all these locations have been taken care of now

...olution/public/detection_engine/rule_management_ui/pages/coverage_overview/filters_panel.tsx
);
};

export const CoverageOverviewFiltersPanel = memo(CoverageOverviewFiltersPanelComponent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously having all components wrapped in memo() and additionally use useMemo() in CoverageOverviewPageComponent won't improve performance. The main performance hit is reflow and we have to avoid it whenever possible so having an extra render usually might not be an issue.

Ideally we need to compare performance without using memo and useMemo and with them to see if we improve something as it doesn't work when references aren't stable and change each render.

It should be safe to get rid of memo() wrapper around components in the folder.

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 6, 2023

@elasticmachine merge upstream

@dplumlee dplumlee enabled auto-merge (squash) August 6, 2023 20:03
@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 6, 2023

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 7, 2023

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 7, 2023

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 7, 2023

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 7, 2023

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 7, 2023

@elasticmachine merge upstream

@dplumlee
Copy link
Contributor Author

dplumlee commented Aug 8, 2023

@elasticmachine merge upstream

@dplumlee dplumlee merged commit f4d8c07 into elastic:main Aug 8, 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 4375 4389 +14

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.6MB 15.6MB +21.7KB

History

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

cc @dplumlee

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 8, 2023
@dplumlee dplumlee deleted the attack-coverage-dashboard-grid branch August 8, 2023 03:12
@banderror banderror mentioned this pull request Aug 9, 2023
Closed
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Aug 9, 2023
@dplumlee @bryce-b
[Security Solution] Coverage Overview Dashboard (elastic#161556)
be11d9d
@banderror banderror mentioned this pull request Aug 23, 2023
Closed
@banderror banderror added release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Sep 6, 2023
@banderror banderror mentioned this pull request Sep 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximpn maximpn maximpn approved these changes

@xcrzx xcrzx Awaiting requested review from xcrzx xcrzx was automatically assigned from elastic/security-detection-rule-management

Assignees

@dplumlee dplumlee

Labels
backport:skip This commit does not require backporting Feature:Rule Management Security Solution Detection Rule Management area release_note:feature Makes this part of the condensed release notes 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.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dplumlee @elasticmachine @kibanamachine @kibana-ci @maximpn @banderror