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] Address cyclomatic complexity issue in RulesTables component #92160

Open
banderror opened this issue Feb 22, 2021 · 5 comments
Assignees
Labels
Feature:Detection Rules Security Solution rules and Detection Engine 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. technical debt Improvement of the software architecture and operational architecture

Comments

@banderror
Copy link
Contributor

banderror commented Feb 22, 2021

RulesTables component kept slowly growing in terms of LOC and needed refactoring.

This PR #91925 has added a few changes so that the cyclomatic complexity limit has been exceed, and I had to temporarily disable the corresponding ESLint rule for the whole file.

While I'm feeling like the whole rules management table and its parent page component need to be fully refactored, this ticket aims to address this specific issue. Maybe there is an easier way to do just that without starting a full-blown refactoring (#92169).

@banderror banderror added technical debt Improvement of the software architecture and operational architecture Feature:Detection Rules Security Solution rules and Detection Engine Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 22, 2021
@banderror banderror self-assigned this Feb 22, 2021
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx
Copy link
Contributor

xcrzx commented May 3, 2021

It is not related to the component's complexity, but In regards to the slow table rendering performance, we could implement these improvements:

  • Introduce virtualization to render only rows that are currently visible to a user. This could cut up to 80% of rendering time for large tables (100+ rows). However, it might not be that easy and require migration from EuiTable to another component.
  • Reduce the overall amount of the table's nested DOM elements wherever possible. For example, hide elements for mobile view with js instead of CSS.
  • Optimize the table's state so that it doesn't trigger re-rendering of the entire tree on every change.

@xcrzx
Copy link
Contributor

xcrzx commented Jun 8, 2021

We also need to refactor the rule table’s state to avoid hacks like useValueChanged altogether. See related discussion: #100554 (comment)

Current problem:

  1. EuiBasicTable uses an array of table items (in our case, Rule[]) to keep track of currently selected rows.
  2. But our RulesTables uses an array of rule IDs to do the same.
  3. We synchronize the two states manually. If we receive a selection change event from EuiBasicTable, we map rules to their ids (Rule[] => string[]). And vice versa, if we change the selection state in RulesTable, we map IDs to rules. (string[] => Rule[]).
  4. That makes usage of plain useEffect impossible because the above transformations create a new array every time, which will lead to an infinite loop of re-renders.

We could solve the problem by refactoring the RulesTable state to store selected rule objects instead of just rule IDs and by dropping Rule[] => string[] maps. And by doing that, we could switch from useValueChanged to useEffect.

@xcrzx
Copy link
Contributor

xcrzx commented Jun 8, 2021

Another thing that we could refactor is getBatchItems(). It's a method that makes a bunch of API calls, but it is not a React component or hook. It doesn't allow to pass down an abort signal, as it wouldn't make sense because there is no lifecycle method to call abort().

We need to refactor getBatchItems() to a hook, like useBatchItems(), and implement proper request cancellation logic.

Original discussion: #100554 (comment)

@peluja1012 peluja1012 added the Team:Detection Rule Management Security Detection Rule Management Team label Sep 14, 2021
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 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. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

4 participants