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] Allow users to edit max_signals field for custom rules #179680

Merged
merged 62 commits into from
May 3, 2024

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Mar 29, 2024

Resolves: #173593
Fixes: #164234

Summary

Adds a number component in the create and edit rule forms so that users are able to customize the max_signals value for custom rules from the UI. Also adds validations to the rule API's for invalid values being passed in.

This PR also exposes the xpack.alerting.rules.run.alerts.max config setting from the alerting framework to the frontend and backend so that we can validate against it as it supersedes our own max_signals value.

Flaky test run (internal)

Screenshots

Form component

Screenshot 2024-04-08 at 11 02 12 PM

Details Page

Screenshot 2024-04-08 at 11 04 04 PM

Error state

Screenshot 2024-04-08 at 11 01 55 PM

Warning state

Screenshot 2024-04-16 at 3 20 00 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee added release_note:enhancement 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 Team:Detection Rule Management Security Detection Rule Management Team v8.14.0 labels Mar 29, 2024
@dplumlee dplumlee self-assigned this Mar 29, 2024
@dplumlee
Copy link
Contributor Author

dplumlee commented Apr 1, 2024

How do we feel about selecting the name for max_signals to be "Max signals"? Obviously that's the field name but seeing as this is the first time (other than maybe this) we're exposing this value to users in the UI and not just the API, do we want to call it as such? The field was created back when we were still using the signals language instead of alerts. cc @joepeeples @approksiu

@joepeeples
Copy link
Contributor

How do we feel about selecting the name for max_signals to be "Max signals"? Obviously that's the field name but seeing as this is the first time (other than maybe this) we're exposing this value to users in the UI and not just the API, do we want to call it as such? The field was created back when we were still using the signals language instead of alerts. cc @joepeeples @approksiu

Haha, jinx, we both asked the same thing just now. 😜 I think "Max alerts" is a more meaningful label to users — I assume we can't also rename the field itself (due to breaking changes, etc)? It might be a little odd to have different names in the UI and in the data, but I don't know if users are likely to notice, if max_signals isn't directly exposed to them.

@dplumlee
Copy link
Contributor Author

dplumlee commented Apr 1, 2024

@joepeeples yeah, we can't easily rename the field without a lot of problems but in the one prior example we have of referring to the value in the UI, we use "max alerts" so I reckon that's probably a good enough solution. It won't break anything for API users anyways so I don't think we would run into too much confusion adding it to the rule forms. I'll go ahead and do that then unless @approksiu had another idea in mind.

@banderror banderror changed the title [Security Solution] Max signals form component [Security Solution] Allow users to edit max_signals field for custom rules Apr 2, 2024
@dplumlee
Copy link
Contributor Author

dplumlee commented Apr 2, 2024

/ci

@dplumlee dplumlee added ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Serverless Project labels Apr 2, 2024
@dplumlee dplumlee marked this pull request as ready for review April 2, 2024 21:45
@dplumlee dplumlee requested review from a team as code owners April 2, 2024 21:45
@dplumlee dplumlee requested review from e40pud and jpdjere April 2, 2024 21:45
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@jpdjere
Copy link
Contributor

jpdjere commented May 3, 2024

@dplumlee I'm trying to get the Serverless URL and credentials to allow for testing, but for whatever reason that step failed. I'll rerun CI.

@jpdjere
Copy link
Contributor

jpdjere commented May 3, 2024

/ci

@jpdjere jpdjere removed ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment ci:project-deploy-security Create a Security Serverless Project labels May 3, 2024
@kibana-ci
Copy link
Collaborator

kibana-ci commented May 3, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5477 5479 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 829 830 +1

Async chunks

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

id before after diff
securitySolution 13.7MB 13.7MB +3.4KB

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5407 +5407
total size - 8.8MB +8.8MB

Page load bundle

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

id before after diff
alerting 23.7KB 23.9KB +201.0B
securitySolution 83.3KB 83.4KB +43.0B
total +244.0B
Unknown metric groups

API count

id before after diff
alerting 861 862 +1

History

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

cc @dplumlee

@jpdjere jpdjere enabled auto-merge (squash) May 3, 2024 18:36
@jpdjere jpdjere merged commit f841763 into elastic:main May 3, 2024
41 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience enhancement New value added to drive a business result Feature:Rule Creation Security Solution Detection Rule Creation Feature:Rule Details Security Solution Detection Rule Details Feature:Rule Edit release_note:feature Makes this part of the condensed release notes release_note:fix 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. v8.15.0
Projects
None yet