-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 a rule management filters internal endpoint #146826
Conversation
7118ec5
to
9c32857
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
704a22a
to
98740a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thank you, @maximpn 🙌 We're finally moving away from aggregating all rule tags in memory 🎉
I looked through the code and left some initial comments. And I'm going to check out the PR and test it locally later this week.
...rity_solution/common/detection_engine/rule_management/api/rules/info/response_schema.test.ts
Outdated
Show resolved
Hide resolved
...rity_solution/common/detection_engine/rule_management/api/rules/info/response_schema.test.ts
Outdated
Show resolved
Hide resolved
...urity_solution/public/detection_engine/rule_management/api/hooks/use_bulk_action_mutation.ts
Outdated
Show resolved
Hide resolved
...urity_solution/public/detection_engine/rule_management/api/hooks/use_create_rule_mutation.ts
Outdated
Show resolved
Hide resolved
...lugins/security_solution/server/lib/detection_engine/rule_management/api/rules/info/route.ts
Outdated
Show resolved
Hide resolved
.../security_solution/common/detection_engine/rule_management/api/rules/info/response_schema.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/detection_engine/rule_management/logic/translations.ts
Outdated
Show resolved
Hide resolved
..._solution/public/detection_engine/rule_management_ui/components/rules_table/rules_tables.tsx
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/security_and_spaces/group1/get_rules_info.ts
Outdated
Show resolved
Hide resolved
x-pack/test/detection_engine_api_integration/security_and_spaces/group1/get_rules_info.ts
Outdated
Show resolved
Hide resolved
98740a4
to
4127eb7
Compare
9e096e8
to
4093f5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked out and tested this PR locally. The new endpoint seems to be working as expected; the rules table filters reflect changes in rules properly. The new endpoint works perceivably faster.
I left some minor suggestions, but overall, the implementation looks good to me 👍
...ecurity_solution/server/lib/detection_engine/rule_management/api/tags/read_tags/read_tags.ts
Show resolved
Hide resolved
...ecurity_solution/server/lib/detection_engine/rule_management/api/tags/read_tags/read_tags.ts
Outdated
Show resolved
Hide resolved
...curity_solution/common/detection_engine/rule_management/api/rules/filters/response_schema.ts
Outdated
Show resolved
Hide resolved
...ins/security_solution/server/lib/detection_engine/rule_management/api/rules/filters/route.ts
Outdated
Show resolved
Hide resolved
...ins/security_solution/server/lib/detection_engine/rule_management/api/rules/filters/route.ts
Show resolved
Hide resolved
9ebc153
to
4dcc047
Compare
@elasticmachine merge upstream |
8d4c693
to
9755803
Compare
@elasticmachine merge upstream |
@pmuellr I've added unit and functional tests to cover the tags limit. To be honest I think 50 tags is quite low limit since the number of tags can easily be higher. To address this and the other issues there is a task I'm working on in parallel which will allow to optimize the aggregate method to use with arbitrary aggregations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx for the changes!
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @maximpn |
Addresses: #137428
Summary
Adds a new internal lightweight endpoint to fetch rules related information like the number of installed prebuilt rules, the number of custom rules and etc.
Details
This PR adds a quite simple and lightweight endpoint for fetching rules related information which is
UI has been updated accordingly. The result of the endpoint are mostly used in the rules table filter but not limited to.
The added endpoint doesn't implement full aggregation for fetching rule numbers so it's planned to be done in the following PR.
Comparison
The following screenshots from the browser's network tab demonstrate that the new endpoint is faster which is good since it's intended to be updated executed relatively often whenever the rules are updated.
Prebuilt rules endpoint which was used for fetching rules related information
The new endpoint
Checklist