-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Replace EUI filtering with custom in-memory filtering in Add Rules and Rule Upgrade tables #159700
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Thanks for the PR, @jpdjere! 👍 I've run some local tests and everything is working as expected. I've noted a few minor suggestions for further enhancements, but none of them are critical.
startTransaction({ name: ADD_PREBUILT_RULES_TABLE_ACTIONS.FILTER }); | ||
setFilterOptions((filters) => ({ | ||
...filters, | ||
filter: filterString.trim(), | ||
})); |
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.
Filtering is a synchronous operation that doesn't initiate HTTP requests. Therefore, starting an APM transaction just before this operation might inadvertently capture any unrelated HTTP requests that follow. I suggest we remove these transactions to avoid capturing irrelevant data.
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.
Thanks! fixing this
|
||
const handleSelectedTags = useCallback( | ||
(newTags: string[]) => { | ||
if (!isEqual(newTags, selectedTags)) { |
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'm wondering whether this isEqual
check is necessary. How frequently is the callback invoked with identical tags?
<EuiFilterGroup> | ||
<TagsFilterPopover | ||
onSelectedTagsChanged={handleSelectedTags} | ||
selectedTags={selectedTags ?? []} |
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.
Nit: according to the provider code, selectedTags
should never be undefined. Can we adjust our types to reflect that?
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.
Yes, thanks. Fixed the types here 👍
Pick<FilterOptions, 'filter' | 'tags'> | ||
>; | ||
|
||
export const useFilterPrebuiltRulesToUpgrade = ({ |
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.
Is it possible for us to reuse the rule filtering method from the add rule context here?
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.
So I actually wanted to create a reusable hook for both adding and upgrading rules cause the filtering logic is the same. The problem is that the type of rules
is slightly different in these two cases, because in the install
case, all the properties of the rule (that I need to filter on) are top-level properties (i.e name
and tags
).
In the upgrade case, those are nested within rule
:
export interface RuleUpgradeInfoForReview {
id: RuleObjectId;
rule_id: RuleSignatureId;
rule: DiffableRule;
diff: PartialRuleDiff;
revision: number;
}
So I tried making it generic for both like:
export const useFilterPrebuiltRulesToInstallOrUpgrade = ({
rules,
filterOptions,
}: {
rules: RuleInstallationInfoForReview[] | Array<RuleUpgradeInfoForReview['rule']>;
filterOptions: AddOrUpgradePrebuiltRulesTableFilterOptions;
}) => {
...
but that means that the Rule Upgrade table will be populated by DiffableRule
-type objects instead of RuleUpgradeInfoForReview
, and that means additional changes among the context, which I think we don't want just to make this reusable.
I'd like to discuss this with you anyways to understand what are you thinking, but I'd rather merge this PR as is with two separate hooks and refactor next week after FF.
7fef3a3
to
e332470
Compare
18d7e79
to
bc0c631
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @jpdjere |
Summary
1. Replaces the EUI out-of-the-box filtering by rule name and tags used in the initial implementation with custom in-memory filtering.
This aligns the look-and-feel of the Rules Management table with the new Add Elastic Rules and Rule Upgrades table
2. Adds a CTA in the Add Elastic RUles table when all rules have been installed, that navigates the user back to the Rules Management table.
Checklist
Delete any items that are not applicable to this PR.
For maintainers