[Security Solution] Update in-memory rules table implementation feature switch logic #124043

xcrzx · 2022-01-28T16:08:31Z

Summary

Addresses https://github.com/elastic/security-team/issues/1972#issuecomment-1021832714

Changed experimental rules table sorting capabilities feature flag.

Before

A threshold value configured per space (3000 default). Experimental sorting was on by default.

After

A boolean flag configured per user (persisted in the local storage). Experimental sorting is now off by default.

Follow-up:

Implement optimistic updates instead of cache invalidation for rule activation/deactivation
Implement optimistic updates instead of cache invalidation for bulk operations (need support on the API side)

elasticmachine · 2022-02-01T11:26:09Z

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

elasticmachine · 2022-02-01T11:26:10Z

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

vitaliidm · 2022-02-01T12:42:24Z

...security_solution/public/detections/pages/detection_engine/rules/all/rules_table_toolbar.tsx

+  grid-template-columns: 1fr auto;
+  align-items: center;
+  grid-gap: 16px;
+  box-shadow: inset 0 -1px 0 #d3dae6;


can we use color from euiTheme?

Yes, sure. I'll change that in a follow-up

vitaliidm · 2022-02-01T12:50:59Z

Interesting observation:
Sort results for rule name with switched on and switched off feature are different

Switch on in memory

Switch off in memory

xcrzx · 2022-02-01T12:59:15Z

Interesting observation: Sort results for rule name with switched on and switched off feature are different

We use localeCompare on the front end, unavailable on the database level. So the sorting will be a bit different when it comes to lower and upper case letters.

banderror · 2022-02-01T12:54:29Z

...tion/public/detections/containers/detection_engine/rules/rules_table/rules_table_context.tsx

-  /**
-   * Use this method to update rules data cached by react-query.
-   * It is useful when we receive new rules back from a mutation query (bulk edit, etc.);
-   * we can merge those rules with the existing cache to avoid an extra roundtrip to re-fetch updated rules.
-   */
-  const updateRules = useCallback(
-    (newRules: Rule[]) => {
-      queryClient.setQueryData<ReturnType<typeof useFindRules>['data']>(
-        getFindRulesQueryKey({ isInMemorySorting, filterOptions, sortingOptions, pagination }),
-        (currentData) => ({
-          rules: mergeRules(currentData?.rules || [], newRules),
-          total: currentData?.total || 0,
-        })
-      );
-
-      /**
-       * Unset loading state for all new rules
-       */
-      const newRuleIds = newRules.map((r) => r.id);
-      const newLoadingRuleIds = loadingRules.ids.filter((id) => !newRuleIds.includes(id));
-      setLoadingRules({
-        ids: newLoadingRuleIds,
-        action: newLoadingRuleIds.length === 0 ? null : loadingRules.action,
-      });
-    },


I think my only concern with this PR is that updating the cache of loaded rules has been replaced by cache invalidation. I'd expect that in the in-memory mode, this can lead to continuous reloading of all the rules when the user is actively working with the table (e.g. enabling/disabling rules, editing tags, etc).

What was the reason for this change?

If the reason is that now we have two caches that are maintained simultaneously (allRules and pagedRules in useFindRules), could we change the implementation of updateRules so that it updates both of them?

I'd expect that in the in-memory mode, this can lead to continuous reloading of all the rules when the user is actively working with the table (e.g. enabling/disabling rules, editing tags, etc).

This change does not affect editing operations because they never had optimistic updates. Therefore, only rule (de)activation is affected. So while cache invalidation is not an ideal option, I don't think it will significantly impact the table's performance.

If the reason is that now we have two caches that are maintained simultaneously (allRules and pagedRules in useFindRules), could we change the implementation of updateRules so that it updates both of them?

Yes, we have already discussed it on yesterday's sync. A more intelligent cache update algorithm would be a better option. But given the timeframe before FF, there's no chance I can implement it in this PR.

Synced on that with @xcrzx in more detail, the agreement is that in general, it makes sense to update the cache instead of invalidating it (when we enable/disable a rule, update it, etc), but

we never had it in the implementation of the table previously for most of the rule actions, except the enable/disable action where we have optimistic updates

it's too late to change that in this PR

Also, the agreement is that updating the rules cache will go as a follow-up performance optimization:

Implement optimistic updates instead of cache invalidation for rule activation/deactivation

Implement optimistic updates instead of cache invalidation for bulk operations (need support on the API side)

banderror · 2022-02-01T12:58:36Z

x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.tsx

@@ -56,6 +57,7 @@ export const RuleSwitchComponent = ({
  const [myEnabled, setMyEnabled] = useState(enabled ?? false);
  const [, dispatchToaster] = useStateToaster();
  const rulesTableContext = useRulesTableContextOptional();
+  const invalidateRules = useInvalidateRules();


If we keep the invalidation logic, I'd suggest exposing invalidateRules from rulesTableContext to make the usage easier.

No, invalidateRules should work in places where the rules table context is not available, as the rule editing page.

Synced on that with @xcrzx in more detail.

Pros for keeping useInvalidateRules and useFindRules separate from the table context (and thus exposed to other components):

React query provider is registered at the very top level of the Security Solution's components tree, which makes the data shareable between pages. Rules table context, on the other hand, is registered at the rules page level. The idea is to decouple the "data" itself (the rules cache) from the table state so that the data could be reused in other places. Currently, we don't have any other views that would read rules from the cache, but in theory, this would be possible. In this case, the other view would have its own react context and wouldn't depend on the rules table context.

Organising data caches hierarchically using cache keys (e.g. like in this implementation, by defining a common "prefix": [FIND_RULES_QUERY_KEY, 'all'] and [FIND_RULES_QUERY_KEY, 'paged', filterOptions, sortingOptions, pagination]) is a practice recommended by developers of react-query.

Cons:

The paged rules cache contains table filters, sorting, and pagination options, meaning that it is inherently coupled with the rules table state. This might make its sharing with other views not desirable, depending on the requirements for that view. This is a minor con because the other view could still define its own prefix like [FIND_RULES_QUERY_KEY, 'other'].

As an action item here that could be addressed in a follow-up PR, useFindRules, useInvalidateRules etc could be combined into a single hook.

kibana-ci · 2022-02-01T13:18:09Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	4.7MB	4.7MB	-516.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	245.9KB	245.9KB	-84.0B

History

💔 Build #20868 failed dbbb3a93ef3a9154199327e1d45b472f598219bd
💔 Build #20775 failed 7328e2ec09358dcdc73f4fcf83b45afe63b98060
💔 Build #20433 failed 50e34e44b731da48cec51eae99a6284a53e0326d

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

cc @xcrzx

vitaliidm · 2022-02-01T14:56:25Z

We use localeCompare on the front end, unavailable on the database level. So the sorting will be a bit different when it comes to lower and upper case letters.

Wondering, won't it be confusing for users with no context of how experimental sorting feature works? Should we maybe put some note in description of experimental feature in toggle?

banderror · 2022-02-01T14:58:34Z

@vitaliidm could you please open a bug ticket for your observation regarding server-side sorting?

Should we maybe put some note in description of experimental feature in toggle?

We could add this bug to a "known issues" section in the release notes.

xcrzx · 2022-02-01T15:00:03Z

Wondering, won't it be confusing for users with no context of how experimental sorting feature works? Should we maybe put some note in description of experimental feature in toggle?

I think this is fine. And to be honest, the way the backend sorting works right now looks like a bug to me. So I'll create a ticket to see if we can fix that.

banderror

I haven't tested the PR, but code changes LGTM 👍
Thank you for the quick change of the feature switch implementation!

xcrzx self-assigned this Jan 28, 2022

xcrzx added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team labels Jan 28, 2022

xcrzx force-pushed the in-memory-rules-table-v2 branch 3 times, most recently from dbbb3a9 to 6aaa382 Compare February 1, 2022 09:51

Update in-memory rules table implementaion feature switch logic

c3afce6

xcrzx force-pushed the in-memory-rules-table-v2 branch from 6aaa382 to c3afce6 Compare February 1, 2022 11:20

xcrzx marked this pull request as ready for review February 1, 2022 11:26

xcrzx requested a review from a team as a code owner February 1, 2022 11:26

xcrzx added release_note:skip Skip the PR/issue when compiling release notes v8.1.0 backport:skip This commit does not require backporting labels Feb 1, 2022

vitaliidm reviewed Feb 1, 2022

View reviewed changes

banderror reviewed Feb 1, 2022

View reviewed changes

banderror approved these changes Feb 1, 2022

View reviewed changes

xcrzx merged commit 5b4be37 into elastic:main Feb 1, 2022

banderror mentioned this pull request Feb 2, 2022

[Security Solution][Detections] In-memory Rule Management and Monitoring tables #119601

Closed

3 tasks

banderror mentioned this pull request Feb 14, 2022

[Security Solution] Update rules cache instead of invalidating it #125574

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Update in-memory rules table implementation feature switch logic #124043

[Security Solution] Update in-memory rules table implementation feature switch logic #124043

xcrzx commented Jan 28, 2022 •

edited

Loading

elasticmachine commented Feb 1, 2022

elasticmachine commented Feb 1, 2022

vitaliidm Feb 1, 2022

xcrzx Feb 1, 2022

vitaliidm commented Feb 1, 2022

xcrzx commented Feb 1, 2022

banderror Feb 1, 2022

xcrzx Feb 1, 2022

banderror Feb 1, 2022

banderror Feb 1, 2022

xcrzx Feb 1, 2022

banderror Feb 1, 2022 •

edited

Loading

kibana-ci commented Feb 1, 2022

vitaliidm commented Feb 1, 2022

banderror commented Feb 1, 2022

xcrzx commented Feb 1, 2022

banderror left a comment

[Security Solution] Update in-memory rules table implementation feature switch logic #124043

[Security Solution] Update in-memory rules table implementation feature switch logic #124043

Conversation

xcrzx commented Jan 28, 2022 • edited Loading

Summary

Before

After

elasticmachine commented Feb 1, 2022

elasticmachine commented Feb 1, 2022

vitaliidm Feb 1, 2022

Choose a reason for hiding this comment

xcrzx Feb 1, 2022

Choose a reason for hiding this comment

vitaliidm commented Feb 1, 2022

Switch on in memory

Switch off in memory

xcrzx commented Feb 1, 2022

banderror Feb 1, 2022

Choose a reason for hiding this comment

xcrzx Feb 1, 2022

Choose a reason for hiding this comment

banderror Feb 1, 2022

Choose a reason for hiding this comment

banderror Feb 1, 2022

Choose a reason for hiding this comment

xcrzx Feb 1, 2022

Choose a reason for hiding this comment

banderror Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Feb 1, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

vitaliidm commented Feb 1, 2022

banderror commented Feb 1, 2022

xcrzx commented Feb 1, 2022

banderror left a comment

Choose a reason for hiding this comment

xcrzx commented Jan 28, 2022 •

edited

Loading

banderror Feb 1, 2022 •

edited

Loading