[Response Ops][Rule Form V2] Rule Form V2: Rule Definition #183325

JiaweiWu · 2024-05-13T17:16:28Z

Summary

Part 1 of 3 PRs of new rule form. This PR extracts the first section of the rule form, the rule definition, from the original PR. The purpose is to fix a few bugs (Such as improving the alert delay and the rule schedule input validation), and also try to make the PR much smaller for review. The design philosophy in the PR is to create components that are devoid of any fetching or form logic. These are simply dumb components.

I have also created a example plugin to demonstrate this PR. To access:

Run the branch with yarn start --run-examples
Navigate to http://localhost:5601/app/triggersActionsUiExample/rule_definition

And you should be able to play around with the components in this PR:

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2024-05-13T17:16:30Z

Pinging @elastic/response-ops (Team:ResponseOps)

…fix'

cnasikas

Thank you for this! Based on the talk we had offline some of my comments are not relevant any more 🙂. Please let me know when that's the case. I know that a lot of code was there from before and we moved things around. Now that we rewrite the rule form I think it is a good opportunity to enhance the types and the code when possible. Please let me know if the work of doing it is too much and is outside the scope of this PR. Also I noticed that most of components are not memoized (React.memo(MyComponent)). What do you think about that? Finally, what is the effort (not on this PR) of not exporting the types from:

packages/kbn-alerts-ui-shared/index.ts

packages/kbn-alerting-types/rule_types.ts

packages/kbn-alerts-ui-shared/src/rule_form/rule_definition/rule_definition.tsx

cnasikas · 2024-05-16T09:44:10Z

packages/kbn-alerts-ui-shared/src/rule_form/rule_definition/rule_schedule.test.tsx

+      <RuleSchedule
+        interval={'30s'}
+        minimumScheduleInterval={{
+          enforce: true,


Is it possible for a minimumScheduleInterval to not be enforced?

And what exactly mean if it is not enforced?

Then it means you can have any interval

Do we have any test for that?

It will come in a future PR, since nothing is enforcing this right now

packages/kbn-alerts-ui-shared/src/rule_form/rule_definition/rule_schedule.tsx

cnasikas · 2024-05-30T08:00:55Z

packages/kbn-alerts-ui-shared/src/rule_form/rule_definition/rule_alert_delay.tsx

+        isInvalid={errors.alertDelay?.length > 0}
+        append={ALERT_DELAY_TITLE_SUFFIX}
+        onChange={onAlertDelayChange}
+        onKeyDown={onKeyDown}


The logic with the onKeyDown and the regex validation is the same between the RuleAlertDelay and RuleSchedule. What about creating a new component called NumbericInput or similar that will have the common logic and the RuleAlertDelay and RuleSchedule components will use?

I think the similarities between the 2 components is not worth abstracting out. I don't think we have another input that will use this ATM. My usual guideline is if something is repeated 3 times then I will abstract it out.

cnasikas

Code LTGTM! I left some comments and questions on existing threads. I am wondering if we should merge on a feature branch instead of main? The reason is that it will help me in the end to see the whole picture of the architecture of the components, test it holistically, be able to add comments, and fix bugs if we find any.

cnasikas

As we discussed offline we will merge on main as the feature is behind a feature flag.

kibana-ci · 2024-05-31T01:23:02Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: f784200

Failed CI Steps

Jest Integration Tests #3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerting`	181	196	+15
`observability`	534	546	+12
`securitySolution`	5496	5508	+12
`triggersActionsUi`	735	747	+12
total			+51

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
`@kbn/alerting-types`	33	161	+128
`@kbn/alerts-ui-shared`	98	183	+85
total			+213

Async chunks

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

id	before	after	diff
`alerting`	92.6KB	96.0KB	+3.4KB
`observability`	296.9KB	300.5KB	+3.5KB
`securitySolution`	15.2MB	15.2MB	+3.4KB
total			+10.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/alerts-ui-shared`	2	4	+2

Page load bundle

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

id	before	after	diff
`alerting`	24.9KB	25.0KB	+4.0B
`observability`	152.2KB	152.3KB	+107.0B
`triggersActionsUi`	121.1KB	124.7KB	+3.6KB
total			+3.7KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/alerting-types`	33	161	+128
`@kbn/alerts-ui-shared`	110	196	+86
total			+214

References to deprecated APIs

id	before	after	diff
`@kbn/alerting-types`	0	24	+24
`alerting`	116	96	-20
total			+4

History

💚 Build #212983 succeeded 202c5ec
💔 Build #209694 failed 4143a3c
💔 Build #209661 failed 3cf8e5e

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

## Summary Issue: #179105 Related PR: #180539 Part 1: #183325 Part 2 of 3 PRs of new rule form. This PR depends on the code from part 1, so only merge this when part 1 has been merged. This PR extracts the last section of the rule form, the rule details, from the original PR. The design philosophy in the PR is to create components that are devoid of any fetching or form logic. These are simply dumb components. I have also created a example plugin to demonstrate this PR. To access: 1. Run the branch with yarn start --run-examples 2. Navigate to http://localhost:5601/app/triggersActionsUiExample/rule_details And you should be able to play around with the components in this PR: <img width="1281" alt="Screenshot 2024-05-13 at 9 44 14 PM" src="https://github.com/elastic/kibana/assets/74562234/7ca900e3-ca9a-4810-8b24-7c3ea41055d6"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Zacqary <zacqary.xeper@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

JiaweiWu added 2 commits May 13, 2024 10:04

Initial commit

0c08c14

Revert rule-data-utils

557b271

JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.15.0 labels May 13, 2024

JiaweiWu requested a review from a team as a code owner May 13, 2024 17:16

kibanamachine and others added 3 commits May 13, 2024 17:22

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

3cf8e5e

…fix'

Fix sandbox

21e65be

Merge branch 'main' into rule-form-v2-rule-definition

4143a3c

JiaweiWu mentioned this pull request May 14, 2024

[Response Ops][Rule Form V2] Rule Form V2: Rule Details #183352

Merged

1 task

cnasikas reviewed May 16, 2024

View reviewed changes

JiaweiWu added 2 commits May 29, 2024 15:49

Merge conflicts

d6b6287

address comments

202c5ec

cnasikas reviewed May 30, 2024

View reviewed changes

cnasikas approved these changes May 30, 2024

View reviewed changes

Move icontip to description

f784200

JiaweiWu merged commit dc3f76b into elastic:main May 31, 2024
35 checks passed

kibanamachine added the backport:skip This commit does not require backporting label May 31, 2024

cnasikas mentioned this pull request Jul 16, 2024

[RAM] [Rule Form V2] Add full-page create/edit form #179105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Response Ops][Rule Form V2] Rule Form V2: Rule Definition #183325

[Response Ops][Rule Form V2] Rule Form V2: Rule Definition #183325

JiaweiWu commented May 13, 2024

elasticmachine commented May 13, 2024

cnasikas left a comment

cnasikas May 16, 2024

JiaweiWu May 22, 2024

cnasikas May 22, 2024 •

edited

Loading

JiaweiWu May 30, 2024

cnasikas May 30, 2024

JiaweiWu May 30, 2024

cnasikas May 30, 2024

JiaweiWu May 30, 2024

cnasikas left a comment •

edited

Loading

cnasikas left a comment

kibana-ci commented May 31, 2024

API count

References to deprecated APIs

[Response Ops][Rule Form V2] Rule Form V2: Rule Definition #183325

[Response Ops][Rule Form V2] Rule Form V2: Rule Definition #183325

Conversation

JiaweiWu commented May 13, 2024

Summary

Checklist

elasticmachine commented May 13, 2024

cnasikas left a comment

Choose a reason for hiding this comment

cnasikas May 16, 2024

Choose a reason for hiding this comment

JiaweiWu May 22, 2024

Choose a reason for hiding this comment

cnasikas May 22, 2024 • edited Loading

Choose a reason for hiding this comment

JiaweiWu May 30, 2024

Choose a reason for hiding this comment

cnasikas May 30, 2024

Choose a reason for hiding this comment

JiaweiWu May 30, 2024

Choose a reason for hiding this comment

cnasikas May 30, 2024

Choose a reason for hiding this comment

JiaweiWu May 30, 2024

Choose a reason for hiding this comment

cnasikas left a comment • edited Loading

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

kibana-ci commented May 31, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

References to deprecated APIs

History

cnasikas May 22, 2024 •

edited

Loading

cnasikas left a comment •

edited

Loading