[Alerting] Improve creation and editing of "Elasticsearch query" rule in Management #134763

jughosta · 2022-06-20T15:36:45Z

Summary

This PR brings an improved and more convenient UI for creating rules to Management page as we have it now on Discover page.

Steps to test:

From Stack Management page:
- Open "Rules and Connectors"
- Select "Elasticsearch query" from rules list
- Select how to configure this new rule: KQL/Lucene or Query DSL
- Check that both options work
From Discover page:
- Press "Alerts" > "Create search threshold rule"
- Check that it still allows to configure a rule in KQL/Lucene mode

Stack Management > New rule > Elasticsearch Query

"KQL or Lucene" is selected

"Query DSL" is selected

Discover > Alerts > Create search threshold rule

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

gmmorris · 2022-06-21T16:38:22Z

Added a deploy to Cloud label so I can play around with these changes.
Thanks @jughosta

jughosta · 2022-06-22T09:22:29Z

@elasticmachine merge upstream

jughosta · 2022-07-04T12:33:34Z

@ymao1 Great findings! Since they are also present on main branch I created 2 separate github issues:

#135641

Also, when I created a rule using a data view, then deleted the data view, I see this when editing the rule:

Should that be a link?

#135642

This is unrelated to this PR, but when I have a user who has no access to "Stack Rules" but does have access to "Discover", I can see the rule creation flyout but when I try to save the rule, I get a permissions error. Is this a known issue?

jughosta · 2022-07-04T13:29:42Z

x-pack/plugins/stack_alerts/public/alert_types/components/data_view_select_popover.tsx

          />
        </EuiFormRow>
+        {createDataView ? (
+          <EuiPopoverFooter paddingSize="none">


@andreadelrio Updated styles for the button in the popover:

@jughosta this needs a further reduction of padding. I'd remove the padding highlighted here with a CSS override.

Thanks @andreadelrio ! Updated:

Is there anything else blocking?

ymao1

LGTM! Nice job

kertal

Lot's of great fine-tuning since the last time I've checked 👍 While giving it another testing round I encountered a reason why we should not allow the user to create data views in the rule flyout. Users could create data views without time field which would lead to invalid rules. I therefore would suggest to not provide the functionality of adding data views in this area

One tiny nit, when I decided I don't wanna create this rule type, so returning to the selection of all rule types I'm getting an error message

jughosta · 2022-07-06T08:05:08Z

While giving it another testing round I encountered a reason why we should not allow the user to create data views in the rule flyout. Users could create data views without time field which would lead to invalid rules. I therefore would suggest to not provide the functionality of adding data views in this area

@kertal I would rather keep this functionality and add a separate validation message to notify that the selected data view requires to have a timestamp. Users can currently select an existing data view in flyout on Discover page too and we don't validate that:

We could also address it as a followup PR since this problem is already present on main branch.

One tiny nit, when I decided I don't wanna create this rule type, so returning to the selection of all rule types I'm getting an error message

This also present on main branch and I think is unrelated to "Elasticsearch query" work we do here. Can we create a separate issue for it?

jughosta · 2022-07-06T08:37:53Z

@elasticmachine merge upstream

kertal · 2022-07-06T10:35:48Z

While giving it another testing round I encountered a reason why we should not allow the user to create data views in the rule flyout. Users could create data views without time field which would lead to invalid rules. I therefore would suggest to not provide the functionality of adding data views in this area

@kertal I would rather keep this functionality and add a separate validation message to notify that the selected data view requires to have a timestamp. Users can currently select an existing data view in flyout on Discover page too and we don't validate that:
We could also address it as a followup PR since this problem is already present on main branch.

Makes sense!

One tiny nit, when I decided I don't wanna create this rule type, so returning to the selection of all rule types I'm getting an error message

This also present on main branch and I think is unrelated to "Elasticsearch query" work we do here. Can we create a separate issue for it?

Makes sense part II!

Will finish my review now, was delayed by a thunderstorm while walking to my hut

kertal

LGTM, testes again with cloud deployment, by creating a alert rule for KQL & Lucene , and another for Query DSL. Works as expected. Thx a lot for brining this over the finishing line! 🥳

jughosta · 2022-07-06T12:30:19Z

@kertal Thanks for review and testing! Created 2 more tickets based on your comments:

[Rules] Discarding a selected rule type in the Create Rule flyout does not allow to select another rule type #135804
[Rules] Add validation to prevent selecting a data view without a time field in "Elasticsearch query" rule flyout #135806

dimaanj

LGTM.

andreadelrio

Design changes LGTM.

kibana-ci · 2022-07-07T08:43:20Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`stackAlerts`	132	136	+4

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
`triggersActionsUi`	376	377	+1

Async chunks

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

id	before	after	diff
`stackAlerts`	205.3KB	209.6KB	+4.3KB
`triggersActionsUi`	866.7KB	866.7KB	+25.0B
total			+4.3KB

Page load bundle

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

id	before	after	diff
`stackAlerts`	13.4KB	13.8KB	+494.0B
`triggersActionsUi`	89.2KB	89.2KB	+32.0B
total			+526.0B

Unknown metric groups

API count

id	before	after	diff
`triggersActionsUi`	390	391	+1

History

💚 Build #55504 succeeded fd61f43
💚 Build #55007 succeeded 27451c2
💚 Build #54674 succeeded 0aae012
💔 Build #54644 failed 0aae012
💚 Build #54609 succeeded f5a8a31
💔 Build #54602 failed e202725

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

cc @jughosta

jughosta added 2 commits June 20, 2022 16:44

[Alerting][Discover] Add query form type selection for ES query

6d7f00d

[Alerting] Add labels

b27a92c

jughosta added release_note:enhancement Feature:Alerting backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.4.0 labels Jun 20, 2022

jughosta self-assigned this Jun 20, 2022

gmmorris requested a review from a team June 21, 2022 09:37

jughosta added 3 commits June 21, 2022 14:44

[Alerting] Unify common rule expressiosn

10c4ed0

[Alerting] Fix code style

6f2d4b6

[Alerting] More refactoring

1873c56

gmmorris added the ci:deploy-cloud label Jun 21, 2022

jughosta added 3 commits June 22, 2022 09:56

[Alerting] Show different UI based on user choice

baa0b29

[Alerting] Improve validation and reset rule params when view changes

bb7e0f1

Resolve conflicts

f03a99a

kibanamachine and others added 11 commits June 22, 2022 05:22

Merge branch 'main' into 134183-alert-management

38c5bfb

[Alerting] Fix button color

d64ceca

[Alerting] Revert tmp changes

c3638ea

[Alerting] Fix query input

1f6df47

[Alerting] Fix thresholdComparator and timeWindowUnit changes

01fed98

[Alerting] Unify UI for different query form types

9906708

[Alerting] Clean up translations

a92802b

[Alerting] Fix for tests

c81e760

[Alerting] Update help tooltips

f5d4f13

[Alerting] Preselect a default data view

a95db6d

[Alerting] Add validation tests

71c801e

This was referenced Jul 4, 2022

[Alerting] Improve editing of Elasticsearch query when data view is deleted #135641

Open

[Alerting] Fix permission to access Alerts menu on Discover #135642

Closed

jughosta added 3 commits July 4, 2022 14:36

[Alerting] Update validation message

20a2ad3

[Alerting] Update styles for Create a data view button

12ec63e

[Alerting] Add message about missing privilieges

27451c2

jughosta commented Jul 4, 2022

View reviewed changes

ymao1 approved these changes Jul 5, 2022

View reviewed changes

kertal reviewed Jul 6, 2022

View reviewed changes

kibanamachine and others added 2 commits July 6, 2022 04:38

Merge branch 'main' into 134183-alert-management

2c10e79

[Alerting] Reduce padding in data view selector

fd61f43

kertal self-requested a review July 6, 2022 10:54

kertal approved these changes Jul 6, 2022

View reviewed changes

kertal requested a review from andreadelrio July 6, 2022 11:07

This was referenced Jul 6, 2022

[Rules] Discarding a selected rule type in the Create Rule flyout does not allow to select another rule type #135804

Open

[Rules] Add validation to prevent selecting a data view without a time field in "Elasticsearch query" rule flyout #135806

Closed

dimaanj approved these changes Jul 6, 2022

View reviewed changes

andreadelrio approved these changes Jul 7, 2022

View reviewed changes

[Alerting] Update copy

5c27116

jughosta merged commit b46763f into elastic:main Jul 7, 2022

jughosta deleted the 134183-alert-management branch July 7, 2022 09:19

lcawl mentioned this pull request Jul 8, 2022

[Alerting] Add popover for Elasticsearch query rule type #135968

Merged

2 tasks

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] Improve creation and editing of "Elasticsearch query" rule in Management #134763

[Alerting] Improve creation and editing of "Elasticsearch query" rule in Management #134763

jughosta commented Jun 20, 2022 •

edited

Loading

gmmorris commented Jun 21, 2022

jughosta commented Jun 22, 2022

jughosta commented Jul 4, 2022 •

edited

Loading

jughosta Jul 4, 2022

andreadelrio Jul 5, 2022

jughosta Jul 6, 2022

ymao1 left a comment

kertal left a comment

jughosta commented Jul 6, 2022

jughosta commented Jul 6, 2022

kertal commented Jul 6, 2022

kertal left a comment

jughosta commented Jul 6, 2022

dimaanj left a comment

andreadelrio left a comment

kibana-ci commented Jul 7, 2022

API count

[Alerting] Improve creation and editing of "Elasticsearch query" rule in Management #134763

[Alerting] Improve creation and editing of "Elasticsearch query" rule in Management #134763

Conversation

jughosta commented Jun 20, 2022 • edited Loading

Summary

Checklist

gmmorris commented Jun 21, 2022

jughosta commented Jun 22, 2022

jughosta commented Jul 4, 2022 • edited Loading

jughosta Jul 4, 2022

Choose a reason for hiding this comment

andreadelrio Jul 5, 2022

Choose a reason for hiding this comment

jughosta Jul 6, 2022

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

jughosta commented Jul 6, 2022

jughosta commented Jul 6, 2022

kertal commented Jul 6, 2022

kertal left a comment

Choose a reason for hiding this comment

jughosta commented Jul 6, 2022

dimaanj left a comment

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 7, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

jughosta commented Jun 20, 2022 •

edited

Loading

jughosta commented Jul 4, 2022 •

edited

Loading