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][Detections] Implement in-memory rule management table #89877

Closed

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Feb 1, 2021

Related to: #50213

Summary

While support for filtering, sorting and searching is in progress on the alerting side (see #50213 (comment)), @peluja1012 and I decided to proceed with in-memory table approach for the time being. This should allow us to build basic filters and ship some UX improvements to our users sooner.

"In-memory table approach" simply means that we load all the detection rules on the client - all at once - and implement filtering, sorting and searching in our custom code.

Scope and plan

The goal of this PR is to implement in-memory table in a sort of hack-ish way as quickly as I can. So I tried to avoid any changes unnecessary for that. If you see a lot of code to improve in this PR - I totally agree with you!

I'm planning to address other topics in follow-up PRs (rough plan):

  • Add basic filters: by severity, risk score.
  • Add basic sorting: by severity, risk score.
  • Consider enhancing search bar by switching to EuiInMemoryTable.
  • Refactor the implementation.
  • Add full test coverage.
  • Fish and fix bugs (e.g. selection and switching between tables is not working properly).
  • Optimize performance.
    • API endpoint handlers (if possible)
    • rendering (number of renders, memoization/caching/equality, virtualized table)

Checklist

@banderror banderror requested review from a team as code owners February 1, 2021 15:22
@banderror banderror self-assigned this Feb 1, 2021
@banderror banderror added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Feb 1, 2021
@elasticmachine
Copy link
Contributor

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

@banderror banderror added Feature:Detection Rules Security Solution rules and Detection Engine refactoring release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team labels Feb 1, 2021
@elasticmachine
Copy link
Contributor

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

Comment on lines +11 to +21
export interface RulesTableFacade {
setRules(newRules: Rule[], newPagination: Partial<PaginationOptions>): void;
updateRules(rules: Rule[]): void;
updateOptions(filter: Partial<FilterOptions>, pagination: Partial<PaginationOptions>): void;
actionStarted(actionType: LoadingRuleAction, ruleIds: string[]): void;
actionStopped(): void;
setShowIdleModal(show: boolean): void;
setLastRefreshDate(): void;
setAutoRefreshOn(on: boolean): void;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming as "facade" and even introduction of this guy is a temporary measure. I'd revisit this later when I start refactoring rules management logic. If you have a better naming suggestion for short term, I'd be glad to rename though.

Comment on lines +55 to +65
if (
tableRef != null &&
tableRef.current != null &&
tableRef.current.changeSelection != null
) {
// for future devs: eui basic table is not giving us a prop to set the value, so
// we are using the ref in setTimeout to reset on the next loop so that we
// do not get a warning telling us we are trying to update during a render
window.setTimeout(() => tableRef?.current?.changeSelection([]), 0);
}
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 wasn't sure if this is necessary, so decided to leave as is at this point.

Comment on lines +20 to +32
// Why these values?
// If we request more rules than 10000, we'll get an error from Elasticsearch (most likely).
// https://www.elastic.co/guide/en/elasticsearch/reference/current/paginate-search-results.html
// https://www.elastic.co/guide/en/elasticsearch/reference/current/index-modules.html#dynamic-index-settings
// see `index.max_result_window`, default value is 10000.
// Example:
// In requests like `GET my-index/_search?from=40&size=20` it's not about items per page,
// but overall from+size count (requested page + all previous pages).
// If max_result_window=10000 this will fail: `GET my-index/_search?from=9990&size=20`.
// We load "all" rules in memory at once. So, because we always request page=1:
const MAX_RULES_TO_LOAD = 10000; // must not be increased
const NUM_RULES_TO_LOAD = MAX_RULES_TO_LOAD / 2; // must be < MAX_RULES_TO_LOAD
Copy link
Contributor Author

@banderror banderror Feb 1, 2021

Choose a reason for hiding this comment

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

5000 is an arguable value, please make suggestions if you will.

Unfortunately, no specific value can give us full confidence that all the rules will be loaded in all cases for all Stack deployments and users. For the time being, this should work, but we definitely need to switch back to server-side filtering later.

I should also note that loading our current 461 rules takes quite some time, and I'm not sure if loading 5000 real rules would be practical prom the perf/UX standpoint.

const projections = useProjectedState(state);

return {
state: { ...state, ...projections },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we decorate state with projected values (derived state) which allows us to leave most of the code in components untouched.

@banderror banderror requested a review from a team February 1, 2021 16:04
@banderror banderror force-pushed the rule-management-alerting-changes branch 2 times, most recently from 8b9d4f1 to 13df11c Compare February 3, 2021 12:14
@banderror banderror marked this pull request as draft February 3, 2021 15:57
@banderror banderror force-pushed the rule-management-alerting-changes branch 2 times, most recently from f0fcdde to 638a192 Compare February 8, 2021 11:08
@banderror banderror force-pushed the rule-management-alerting-changes branch 2 times, most recently from 1244a12 to e087f56 Compare February 11, 2021 13:34
@banderror
Copy link
Contributor Author

banderror commented Feb 12, 2021

In the team, we decided to not go this way of building the in-memory table as an interim solution. Alerting team found a promising approach to supporting filtering, searching and sorting for rule parameters in Alerting APIs (see #50213 (comment)), so we're going to invest our time in other work, e.g. in optimizing our rules-related endpoints, getting rid of "sidecar" saved objects, etc.

Closing this one.

@banderror banderror closed this Feb 12, 2021
@banderror banderror deleted the rule-management-alerting-changes branch February 12, 2021 17:34
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2190 2196 +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 7.5MB 7.6MB +4.3KB

History

  • 💚 Build #106028 succeeded e087f568b30828d98e1a0558c02cb86a12f1f555
  • 💔 Build #105185 failed 1244a1292e9523ce84316cc8bbafb43ec31b49ac
  • 💚 Build #104633 succeeded 638a1923501f86428fed3dea1a1f2c3a0bd3d2d2
  • 💚 Build #103767 succeeded f0fcddef9064b082b982466ee9188ecac70c0109
  • 💚 Build #103370 succeeded 13df11c8faadb53b3b21af90e1e3f8746bc3b4ef

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

spong pushed a commit that referenced this pull request Feb 18, 2021
…nt table (#91302)

**Related to:** #89877

## Summary

This is based on #89877 and the kind of pre-refactoring that has been done there.

Mainly this:

- consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc)
- removes some of the redundant state, leverages the reducer as the source of truth for state
- makes it easier to dispatch events, removes some of the noise

While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code).

### Checklist

- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 18, 2021
…nt table (elastic#91302)

**Related to:** elastic#89877

## Summary

This is based on elastic#89877 and the kind of pre-refactoring that has been done there.

Mainly this:

- consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc)
- removes some of the redundant state, leverages the reducer as the source of truth for state
- makes it easier to dispatch events, removes some of the noise

While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code).

### Checklist

- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine added a commit that referenced this pull request Feb 18, 2021
…nt table (#91302) (#91777)

**Related to:** #89877

## Summary

This is based on #89877 and the kind of pre-refactoring that has been done there.

Mainly this:

- consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc)
- removes some of the redundant state, leverages the reducer as the source of truth for state
- makes it easier to dispatch events, removes some of the noise

While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code).

### Checklist

- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
banderror added a commit to banderror/kibana that referenced this pull request Feb 24, 2021
…nt table (elastic#91302)

**Related to:** elastic#89877

## Summary

This is based on elastic#89877 and the kind of pre-refactoring that has been done there.

Mainly this:

- consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc)
- removes some of the redundant state, leverages the reducer as the source of truth for state
- makes it easier to dispatch events, removes some of the noise

While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code).

### Checklist

- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/rules_tables.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Security Solution rules and Detection Engine refactoring release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.12.0 v7.13.0 v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants