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] Invalidate updated rules instead of marking them instantly stale #153529

Closed

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Mar 23, 2023

Addresses: #151151

Summary

It's an improved fix of #151151. It helps to reduce the number of fetch data requests by reusing cache data when it's possible.

Before:

Screen.Recording.2023-03-23.at.14.33.35.mov

After:

Screen.Recording.2023-03-23.at.14.35.38.mov

@maximpn maximpn added technical debt Improvement of the software architecture and operational architecture 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. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Mar 23, 2023
@maximpn maximpn self-assigned this Mar 23, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #1 / Detections : Page Filters Alert Page Filters Customization Add New Controls
  • [job] [logs] Security Solution Tests #1 / Detections : Page Filters Alert Page Filters Customization should not sync to the URL in edit mode but only in view mode

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @maximpn

@maximpn maximpn marked this pull request as ready for review March 23, 2023 15:51
@maximpn maximpn requested a review from a team as a code owner March 23, 2023 15:51
@maximpn maximpn requested a review from jpdjere March 23, 2023 15:51
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@maximpn maximpn requested a review from xcrzx March 23, 2023 15:59
Comment on lines -60 to +61
} else {
// We failed to receive the list of update rules, invalidate all
invalidateFindRulesQuery();
}
invalidateFindRulesQuery({ refetchType: 'none' });
Copy link
Contributor

Choose a reason for hiding this comment

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

It introduces a high probability that we will display stale data eventually. Refetch type none prevents hooks that are currently rendered to refetch their data. Therefore if there is no updatedRules in the response, they will continue to display outdated data.

@@ -35,9 +35,6 @@ export const useFetchRuleByIdQuery = (id: string, options?: UseQueryOptions<Rule
{
...DEFAULT_QUERY_OPTIONS,
...options,
// Mark this query as immediately stale helps to avoid problems related to filtering.
// e.g. enabled and disabled state filter require data update which happens at the backend side
staleTime: 0,
Copy link
Contributor

@xcrzx xcrzx Mar 24, 2023

Choose a reason for hiding this comment

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

I gave it another thought, and keeping the stale time to 0 seems more appropriate. Rules contain dynamic data, such as "last run", "status", etc., that we may not know when it updates. As a result, we should consider the data as always stale and fetch updates each time a data fetching hook mounts. This approach helps avoid inconsistencies when a single rule is displayed under different filters, which could lead to conflicting information. For example, a rule might appear to have been executed 6 minutes ago in one view while displaying an execution time 1 minute ago in another. We can ensure that the information displayed remains consistent and accurate by fetching updates every time a hook mounts.

@maximpn
Copy link
Contributor Author

maximpn commented Apr 6, 2023

After the careful investigation and discussion with @xcrzx the problem described in this comment was reproduced. To do that the following should be done

  • have as minimum one rule
  • enabled the rule with 1 second execution interval and 1 second look back time
  • checkout the rules table, filter rules by enabled, refresh it, then reset the filter
  • notice the difference in the lat run column, the value differs for the same rule but different filters activated

This way staleTime: 0 stays as the default caching option for the rules query. It invalidates the data right after it was fetched so each time a filter is activated it leads to sending a new request to fetch a fresh portion of the data. Any other ways to implement cache invalidation look more complex to implement and don't give clear benefits. So I close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Rule Management Security Solution Detection Rule Management area release_note:skip Skip the PR/issue when compiling 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. technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants