[Security Solution] Add `threshold`, `machine_learning_job_id` and `anomaly_threshold` editable fields #200323

nikitaindik · 2024-11-16T11:26:56Z

Partially addresses: #171520

Summary

Changes in this PR:

threshold and machine_learning_job_id, anomaly_threshold are now editable in the Rule Upgrade flyout

Testing

Ensure the prebuiltRulesCustomizationEnabled feature flag is enabled.
To simulate the availability of prebuilt rule upgrades, downgrade a currently installed prebuilt rule using the PATCH api/detection_engine/rules API.
- Set version: 1 in the request body to downgrade it to version 1.
- Modify other rule fields in the request body as needed to test the changes.

elasticmachine · 2024-11-26T07:58:27Z

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

elasticmachine · 2024-11-26T07:58:29Z

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

elasticmachine · 2024-11-26T07:58:30Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

nikitaindik · 2024-12-03T14:31:17Z

@xcrzx I've addressed the feedback. Feel free to take another look!

xcrzx

Thanks for the fixes! I did another round of review, and the validation regression spotted earlier has been resolved.

rylnd

Excellent work here, @nikitaindik !

I first verified that the existing create/update rule flows work as expected, since those were modified here as well. Things look good there, besides a potential bug with threshold fields (showing more fields than we should); I'm going to ping @vitaliidm since he has more familiarity with threshold rules and that area was more significantly modified. ML rules changes look good, though.

There might be some cleanup around the forms here, now; I had one comment about whether empty schema definitions were required. Have you discussed this style of refactor with @sebelga at all? He may have thoughts on how best to factor/place things as we shift more toward this "field-first" approach to our forms.

I was also able to validate that the missing validations that @xcrzx referenced are now there, at least for the ML fields that I was exercising.

One small UX note: on the upgrade page it's not obvious that clicking the rule's name will bring up the "review" flow; I was expecting there to be a "review" button instead of (or in addition to) the disabled "upgrade rule" button.

All minor things in the broader context, here, so approving in the meantime. Hopefully @vitaliidm can confirm the threshold changes before this is merged.

...ement/components/rule_details/three_way_diff/final_edit/machine_learning_rule_field_edit.tsx

rylnd · 2024-12-03T22:18:13Z

...urity_solution/public/detection_engine/rule_creation_ui/components/threshold_input/styles.ts

+    @container (min-width: ${CONTAINER_BREAKPOINT}px) {
+      flex: 1;
+      max-width: calc(
+        100% - ${OPERATOR_WIDTH}px - ${INPUT_WIDTH}px - ${euiThemeVars.euiSizeL} -


Curious if

- ${euiThemeVars.euiSizeL} - ${euiThemeVars.euiSizeL}

is preferred over

- (${euiThemeVars.euiSizeL} * 2)

? And if so, why?

euiThemeVars.euiSizeL is actually a string with px at the end. 24px in this case, so I can't multiply it.

rylnd · 2024-12-03T22:31:59Z

...blic/detection_engine/rule_creation/components/create_ml_job_button/create_ml_job_button.tsx

+import * as i18n from './translations';
+
+const buttonClassName = css`
+  margin-top: 20px;


Magic number? Should we be referencing a theme variable instead?

We need 20px here to make the button flush with the dropdown. euiSizeM is 16px, euiSizeL is 24px.

But now as you pointed this out, I think the margin should live outside of this component since it's related to an external thing.

...rity_solution/public/detection_engine/rule_creation_ui/components/step_define_rule/index.tsx

rylnd · 2024-12-03T23:01:51Z

...rity_solution/public/detection_engine/rule_creation_ui/components/step_define_rule/index.tsx

+          )}
+          {isThresholdRule && (
+            <EuiFormRow data-test-subj="thresholdInput" fullWidth>
+              <ThresholdEdit esFields={indexPattern.fields as FieldSpec[]} path="threshold" />


Using indexPattern.fields instead of the filtered aggFields means we're now showing non-aggregatable fields in the threshold field select, right?

That's a great catch, Ryland! I somehow missed it.

Pushed the fix

...ity_solution/public/detection_engine/rule_creation_ui/components/step_define_rule/schema.tsx

..._solution/public/detection_engine/rule_creation/components/threshold_edit/threshold_edit.tsx

…elds

nikitaindik · 2024-12-04T13:50:49Z

@rylnd

One small UX note: on the upgrade page it's not obvious that clicking the rule's name will bring up the "review" flow; I was expecting there to be a "review" button instead of (or in addition to) the disabled "upgrade rule" button.

Will add to this to feedback to share this with Kseniia and Alex.

vitaliidm · 2024-12-04T14:32:02Z

@rylnd

Things look good there, besides a potential bug with threshold fields (showing more fields than we should); I'm going to ping @vitaliidm since he has more familiarity with threshold rules and that area was more significantly modified

can you give me more details on this one?

Meanwhile I tested PR a bit (did not review code, since Ryland already did this)

UI has changed. Width of threshold elements in particularly. Number inputs become very narrow, even on a wide screen.
Was this changed approved? Or side effect of refactoring?

old UI

new UI

As side effect of UI change, error message looks very crumbled and >= is not aligned in a middle of input components anymore. but gets dragged lower

new UI

old UI

nikitaindik · 2024-12-05T08:32:20Z

Hey, everyone! Thanks for reviewing and testing. There's still a minor issue with form element sizing that need to be addressed, but I urgently need to switch to high-priority Rule Customization tickets. So, I've set this PR to draft. I will reopen it and ping those who haven't approved it yet once I resolve the issues.

rylnd · 2024-12-05T22:51:52Z

can you give me more details on this one?

@vitaliidm I was referring to this comment about passing a wider set of fields to the threshold input. @nikitaindik said a fix was incoming but I've not yet seen it on this branch.

vitaliidm · 2024-12-06T10:32:28Z

@vitaliidm I was referring to this comment about passing a wider set of fields to the threshold input. @nikitaindik said a fix was incoming but I've not yet seen it on this branch.

Agree, fields should be filtered, only aggregatable ones should show up in threshold control

@nikitaindik , I also noticed, Suppress alerts checkbox label does not show selected field.
Refer to my screenshots #200323 (comment)

…elds

…y" fields are changed

nikitaindik · 2024-12-16T14:54:18Z

@vitaliidm

UI has changed. Width of threshold elements in particularly. Number inputs become very narrow, even on a wide screen.

Thanks for catching this! Actually, the width of the number inputs is similar to what’s on main at 1280px. I’ve attached a screenshot for reference: the left side shows this branch, and the right side shows main. I think that it’s helpful to give more space to field names since they can be quite long, whereas number values are usually short. What do you think about this approach?

As side effect of UI change, error message looks very crumbled and >= is not aligned in a middle of input components anymore. but gets dragged lower

Great observation, thanks for pointing this out! I’ve adjusted the margin to ensure that the >= operator stays properly aligned, even when more fields are added or an error message appears.

I also noticed, Suppress alerts checkbox label does not show selected field.
Refer to my screenshots #200323 (comment)

Indeed, there was a bug. Thanks! I've pushed a commit with the fix.

With all your feedback addressed, the PR is ready for another review. 😊

elasticmachine · 2024-12-16T15:52:13Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: db6e424
Cloud Deployment
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-200323-db6e424903d3
Security Deployment

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #6 / AgentBasedPackagePoliciesTable renders the table with package policies
[job] [logs] Serverless AI Assistant - Security Solution Cypress Tests / AI Assistant Conversations Changing conversations Correctly creates and titles new conversations, and allows title updates Correctly creates and titles new conversations, and allows title updates
[job] [logs] AI Assistant - Security Solution Cypress Tests / AI Assistant Conversations Changing conversations Correctly creates and titles new conversations, and allows title updates Correctly creates and titles new conversations, and allows title updates
[job] [logs] FTR Configs #113 / Upgrade Assistant Elasticsearch deprecation logs GET /api/upgrade_assistant/deprecation_logging /count should filter out the deprecation from Elastic products
[job] [logs] FTR Configs #113 / Upgrade Assistant Elasticsearch deprecation logs GET /api/upgrade_assistant/deprecation_logging /count should filter out the deprecation from Elastic products

History

💚 Build #256919 succeeded 0933e39
💚 Build #256481 succeeded 72b195a
💚 Build #255780 succeeded f4907b4
💚 Build #255779 succeeded a2b3c53

cc @nikitaindik

nikitaindik self-assigned this Nov 16, 2024

nikitaindik force-pushed the machine-learning-fields branch from c195e14 to ff1858e Compare November 17, 2024 18:08

maximpn mentioned this pull request Nov 18, 2024

[Security Solution] Rework Update flyout to display all field updates and build Three-Way-Diff field component #171520

Open

69 tasks

banderror added v8.18.0 and removed v8.17.0 labels Nov 21, 2024

nikitaindik force-pushed the machine-learning-fields branch from ff1858e to d13fb76 Compare November 26, 2024 07:22

nikitaindik marked this pull request as ready for review November 26, 2024 07:58

nikitaindik requested review from a team as code owners November 26, 2024 07:58

nikitaindik requested a review from rylnd November 26, 2024 07:58

nikitaindik requested a review from maximpn November 26, 2024 07:58

nikitaindik added 5 commits November 26, 2024 10:01

Add machine_learning_job_id

11fe8b5

Add anomaly_threshold

d57358b

Add threshold

6f74bda

Fix i18n

32b5477

Remove unused import

b6a9d92

nikitaindik force-pushed the machine-learning-fields branch from f198cad to b6a9d92 Compare November 26, 2024 09:01

nikitaindik requested a review from xcrzx November 26, 2024 09:02

banderror added the Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area label Nov 26, 2024

Fix displaying long field names in Threshold input

72b195a

nikitaindik requested a review from xcrzx December 3, 2024 14:31

xcrzx approved these changes Dec 3, 2024

View reviewed changes

rylnd approved these changes Dec 3, 2024

View reviewed changes

rylnd requested a review from vitaliidm December 3, 2024 23:39

nikitaindik added 3 commits December 4, 2024 13:03

Merge remote-tracking branch 'upstream/main' into machine-learning-fi…

068f879

…elds

Move "Create custom job" button margin outside of component

9e231bb

Move aggregatable fields computation into ThresholdEdit

0933e39

vitaliidm requested review from vitaliidm and removed request for vitaliidm December 4, 2024 14:32

banderror marked this pull request as draft December 4, 2024 16:22

Merge branch 'main' into machine-learning-fields

318b023

Merge branch 'main' into machine-learning-fields

26e947a

nikitaindik added 7 commits December 9, 2024 09:08

Merge branch 'main' into machine-learning-fields

a4e0c6e

Merge branch 'main' into machine-learning-fields

46daae5

Merge remote-tracking branch 'upstream/main' into machine-learning-fi…

f30cb6d

…elds

Merge branch 'main' into machine-learning-fields

c360516

Merge branch 'main' into machine-learning-fields

c175679

Update imports to reflect latest structure changes

0993cc0

Make sure alert suppression component updates when Threshold "Group b…

8a05118

…y" fields are changed

Update >= operator styles

db6e424

nikitaindik marked this pull request as ready for review December 16, 2024 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Add `threshold`, `machine_learning_job_id` and `anomaly_threshold` editable fields #200323

[Security Solution] Add `threshold`, `machine_learning_job_id` and `anomaly_threshold` editable fields #200323

nikitaindik commented Nov 16, 2024 •

edited

Loading

elasticmachine commented Nov 26, 2024

elasticmachine commented Nov 26, 2024

elasticmachine commented Nov 26, 2024

nikitaindik commented Dec 3, 2024

xcrzx left a comment

rylnd left a comment

rylnd Dec 3, 2024

nikitaindik Dec 4, 2024

rylnd Dec 3, 2024

nikitaindik Dec 4, 2024

rylnd Dec 3, 2024

nikitaindik Dec 4, 2024

nikitaindik Dec 4, 2024

nikitaindik commented Dec 4, 2024

vitaliidm commented Dec 4, 2024

nikitaindik commented Dec 5, 2024

rylnd commented Dec 5, 2024

vitaliidm commented Dec 6, 2024

nikitaindik commented Dec 16, 2024

elasticmachine commented Dec 16, 2024 •

edited

Loading

[Security Solution] Add threshold, machine_learning_job_id and anomaly_threshold editable fields #200323

Are you sure you want to change the base?

[Security Solution] Add threshold, machine_learning_job_id and anomaly_threshold editable fields #200323

Conversation

nikitaindik commented Nov 16, 2024 • edited Loading

Summary

Testing

elasticmachine commented Nov 26, 2024

elasticmachine commented Nov 26, 2024

elasticmachine commented Nov 26, 2024

nikitaindik commented Dec 3, 2024

xcrzx left a comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

rylnd Dec 3, 2024

Choose a reason for hiding this comment

nikitaindik Dec 4, 2024

Choose a reason for hiding this comment

rylnd Dec 3, 2024

Choose a reason for hiding this comment

nikitaindik Dec 4, 2024

Choose a reason for hiding this comment

rylnd Dec 3, 2024

Choose a reason for hiding this comment

nikitaindik Dec 4, 2024

Choose a reason for hiding this comment

nikitaindik Dec 4, 2024

Choose a reason for hiding this comment

nikitaindik commented Dec 4, 2024

vitaliidm commented Dec 4, 2024

old UI

new UI

new UI

old UI

nikitaindik commented Dec 5, 2024

rylnd commented Dec 5, 2024

vitaliidm commented Dec 6, 2024

nikitaindik commented Dec 16, 2024

elasticmachine commented Dec 16, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

History

[Security Solution] Add `threshold`, `machine_learning_job_id` and `anomaly_threshold` editable fields #200323

[Security Solution] Add `threshold`, `machine_learning_job_id` and `anomaly_threshold` editable fields #200323

nikitaindik commented Nov 16, 2024 •

edited

Loading

elasticmachine commented Dec 16, 2024 •

edited

Loading