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 filter and search bar #163498

Merged
merged 10 commits into from
Aug 14, 2023
Merged

[Security Solution] Coverage overview dashboard filter and search bar #163498

merged 10 commits into from
Aug 14, 2023

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Aug 9, 2023 •
edited
Loading

Summary

Addresses: #158246 and #158245

Adds the MVP filters and search bar to the coverage overview dashboard page

Screenshots

Screenshot 2023-08-09 at 11 07 21 AM
Screenshot 2023-08-09 at 11 07 51 AM

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 backport:skip This commit does not require backporting Team:Detections and Resp Security Detection Response Team Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team v8.10.0 labels Aug 9, 2023
@dplumlee dplumlee self-assigned this Aug 9, 2023
@dplumlee dplumlee marked this pull request as ready for review August 11, 2023 03:41
@dplumlee dplumlee requested review from a team as code owners August 11, 2023 03:41
@dplumlee dplumlee requested a review from maximpn August 11, 2023 03:41
@elasticmachine
Copy link
Contributor

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

dplumlee
dplumlee commented Aug 11, 2023
View reviewed changes
...urity_solution/public/detection_engine/rule_management_ui/pages/coverage_overview/reducer.ts Outdated
switch (action.type) {
case 'setShowExpandedCells': {
const { value } = action;
return { ...state, showExpandedCells: value };
}
case 'setRuleStatusFilter': {
const { value } = action;
if (value.length === 0 || value.length === 2) {
Copy link
Contributor Author

@dplumlee dplumlee Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@maximpn This weird hardcoded edge case with the length === 2 is caused by the api not taking both Enabled and Disabled as filters (because of the way the ternary written is structured here), even though we seem to have the other rule filter be an OR query join instead of an AND. Is this the expected action?

Copy link
Contributor

Choose a reason for hiding this comment

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

As empty activity filter produces the same result as as all selected activity values it makes sense to omit the filter param in the request when all values are selected. I don't think there should be an ability to do the same thing in two ways. But you are right the API endpoint doesn't handle right requests when activity is set to ['enabled', 'disabled']. I thought to change it during milestone 2 but it makes sense to do it earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR fixes the problem.

maximpn
maximpn reviewed Aug 11, 2023
View reviewed changes
Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@dplumlee thank you for nice coverage overview dashboard filters implementation 👍 I played with the dashboard having 3K+ rules and it works as charm 🙌

Though I have concerns about code organization. It should conform with the other code in particular CoverageOverviewDashaboardContext should be similar to contexts added as a part of Rule Immutability/Customization epic.

@banderror banderror added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. and removed backport:skip This commit does not require backporting labels Aug 11, 2023
@elasticmachine
Copy link
Contributor

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

@dplumlee dplumlee requested a review from maximpn August 14, 2023 01:38
maximpn
maximpn reviewed Aug 14, 2023
View reviewed changes
Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@dplumlee thank you for addressing my comments 🙏 I've done the second review and left some comments. Mostly my concern is about passing filter to RuleActivityFilterComponent and `RuleSourceFilterComponent.

...urity_solution/public/detection_engine/rule_management_ui/pages/coverage_overview/reducer.ts Outdated
switch (action.type) {
case 'setShowExpandedCells': {
const { value } = action;
return { ...state, showExpandedCells: value };
}
case 'setRuleStatusFilter': {
const { value } = action;
if (value.length === 0 || value.length === 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR fixes the problem.

maximpn
maximpn approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@dplumlee the recent version LGTM 👍

There some little things left but it doesn't look critical. Approving in advance to unlock merging back the PR before FF.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Defend Workflows Cypress Tests #1 / Endpoint Policy Response from Fleet Agent Details page should display policy response with errors should display policy response with errors

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4401 4404 +3

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 +6.7KB

History

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

cc @dplumlee

vitaliidm
vitaliidm approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

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

Detection engine area LGTM

@dplumlee dplumlee merged commit ab133a7 into elastic:main Aug 14, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 14, 2023
@dplumlee dplumlee deleted the coverage-overview-filters branch August 14, 2023 17:43
This was referenced Aug 15, 2023
Closed
Closed
bryce-b pushed a commit that referenced this pull request Aug 22, 2023
@dplumlee @bryce-b
[Security Solution] Coverage overview dashboard filter and search bar (…
3822a93 
…#163498)
@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

@vitaliidm vitaliidm vitaliidm approved these changes

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.
7 participants
@dplumlee @elasticmachine @kibana-ci @maximpn @vitaliidm @banderror @kibanamachine