[Monitoring] Enable out of the box alerts modal #101565

estermv · 2021-06-08T09:27:12Z

Summary

Closes #100133. This PR includes the following changes:

Remove auto-creation of default rules in stack monitoring
Adds a modal that allows the user to create the default rules:

If there are already alerts created, the modal won't appear.
After the user answers "Yes" or "No" they shouldn't see the modal again. (we use localStorage to save if the user answered the modal)
Adds a "Rules and Alerts" in the navigation bar, with an option to create the default rules.

Test cases

When monitoring is disabled

Everything should work as before - modal should not appear and alerts should not be created

When there is no monitoring data

Everything should work as before - modal should not appear and alerts should not be created

When the user has existing rules

They shouldn't see any modal

When the user doesn't have existing rules

They should see the modal the first time they visit a cluster overview page

When the user doesn't have existing rules and selects "Yes" on the rules modal

Rules should be created
The user shouldn't see the modal anymore

When the user doesn't have existing rules and selects "No" on the rules modal

Rules shouldn't be created
The user shouldn't see the modal anymore

When the user doesn't have existing rules and selects "Remind me later" or closes the modal with the X button:

Rules shouldn't be created
The user should see the modal in a new page session (this is controlled by storing a variable in the sessionStorage)

When the user selects "Create default rules" from the dropdown in the navigation bar

Rules should be created if they don't exist

Technical notes

Alerts are created by the api/monitoring/v1/alerts/enable endpoint. The backend didn't change, so the behavior of creating alerts should be the same as before.

There are a lot of test files modified. This is because the alerts modal appears on the cluster overview page that is the entry point of them and clicks happen on the modal overlay until it's closed. I tried to find a generic way to close it (or never open it), but I couldn't. I'll open a separate issue to look deeper into it.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

estermv · 2021-06-22T13:36:59Z

@elasticmachine merge upstream

kibanamachine · 2021-06-22T13:37:00Z

merge conflict between base and head

…toring-alerts-opt-in

ravikesarwani · 2021-06-23T14:12:09Z

@estermv One additional test case I would also suggest adding to the list it to make sure the flow is working correctly when monitoring is not enabled on the cluster and/or there is no monitoring data. The functionality differs on Cloud (ESS) and self-managed so we probably should try out on both.

estermv · 2021-06-23T14:29:05Z

@estermv One additional test case I would also suggest adding to the list it to make sure the flow is working correctly when monitoring is not enabled on the cluster and/or there is no monitoring data.

@ravikesarwani good point! Just added them and I'll also test them locally before opening the PR.

The functionality differs on Cloud (ESS) and self-managed so we probably should try out on both.

In which ways it differs? I'm not familiar with it.

ravikesarwani · 2021-06-23T14:53:52Z

In which ways it differs? I'm not familiar with it.

I haven't tried it out recently but recollect functionality around the following lines:
Cloud: In Cloud it knows that the cluster is in ESS and puts out a message asking customer to enable monitoring from the Cloud console.
Self-managed: If security is not enabled it asks user to enable security before stack monitoring can be used.
It gives out a message asking users to use metricbeat for setting up monitoring. It also has a "single click button" in this case to enable legacy Elasticsearch self monitoring (to get started quickly in a test environment).

estermv · 2021-06-23T16:46:41Z

@elasticmachine merge upstream

estermv · 2021-06-28T08:38:48Z

It gives out a message asking users to use metricbeat for setting up monitoring. It also has a "single click button" in this case to enable legacy Elasticsearch self monitoring (to get started quickly in a test environment).

Then this is for the messaging for when the user doesn't have monitoring enabled yet, it is not affected by the changes in this PR.

estermv · 2021-06-28T08:51:37Z

@elasticmachine merge upstream

elasticmachine · 2021-06-28T08:53:49Z

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

mgiota · 2021-06-28T15:13:22Z

x-pack/plugins/monitoring/public/services/enable_alerts_modal.js

+
+export function enableAlertsModalProvider($http, $window, $injector) {
+  function shouldShowAlertsModal(alerts) {
+    const modalHasBeenShown = $window.sessionStorage.getItem('ALERTS_MODAL_HAS_BEEN_SHOWN');


@estermv Is it on purpose that you use sessionStorage here instead of localStorage as you do later on for decisionMade?

Yes, it's to easily manage the Remind me later option. The modal will appear again to the user in the next page session. This gives a nice "remind me later" feeling on when to show the modal again. (We can always add a specific timer later if we see this don't work)

When the user selects Yes or No we don't want the modal to appear again, so I use the localStorage as "permanent" storage. (Of course, there are some edge cases, like the user cleaning the localStorage or using a different browser, but we left this out from the scope of this PR)

simianhacker

LGTM! Good Job! Thank you for including the checklist for the test scenarios, it was very helpful.

katefarrar

This looks good overall. I have a few UX observations / questions...

Once a user selects "Yes" from the modal and creates the rules they are then able to go to the "Alerts and rules" dropdown and "Create default rules". That makes it seem like the default rules were never created. Instead, could the language say "Re-create default rules"?
If a user click "Create default rules" from the "Alerts and rules" dropdown they don't receive any type of confirmation until there is a toast notification, which doesn't happen right away. Instead could the default rules modal pop up again and require them to select "Yes" so it's clear they have taken an action?
Can we add the "Manage rules" link that goes to app/management/insightsAndAlerting/triggersActions/rules to the "Alerts and rules" dropdown (same functionality the other Observability apps):

Several toast notifications continue to pop up even if only one alert was created. Is that intended? For example, if I select "Yes" for the default rules to be created and then enter setup mode, only 1 alert shows even though I've received several toast notifications saying a new alert was created.

estermv · 2021-06-29T10:13:34Z

@katefarrar thanks a lot for your review!

* Once a user selects "Yes" from the modal and creates the rules they are then able to go to the "Alerts and rules" dropdown and "Create default rules". **That makes it seem like the default rules were never created. Instead, could the language say "Re-create default rules"?**

Should it say always "Re-create default rules"? If the user answers "No" in the modal, do we want to show "Create ..."? If we want both and change it base on the user answer, it's more complicated, since the dropdown appears on every page, I'm not sure if we can know from all pages if the user has alerts created, we could create a separate issue to investigate this.
If we can have always "Re-create default rules" then it's easy to change.

* If a user click "Create default rules" from the "Alerts and rules" dropdown they don't receive any type of confirmation until there is a toast notification, which doesn't happen right away. **Instead could the default rules modal pop up again and require them to select "Yes" so it's clear they have taken an action?**

The modal is not prepared to be opened from the dropdown, if we want this it should be implemented as a separate issue.

* Can we add the "Manage rules" link that goes to `app/management/insightsAndAlerting/triggersActions/rules` to the "Alerts and rules" dropdown (same functionality the other Observability apps):

It was not clear what should be in this dropdown (there was a bit of discussion in the original issue regarding this). If I remember correctly, the major problem with your suggestion was that users can't edit the stack monitoring alerts from there, they can only delete them. We have a meeting later to discuss the changes of this PR, I'll bring this there and add it in case we have an agreement.

* Several toast notifications continue to pop up even if only one alert was created. Is that intended? **For example, if I select "Yes" for the default rules to be created and then enter setup mode, only 1 alert shows even though I've received several toast notifications saying a new alert was created.**

Not sure what you mean by this. Toast notifications appear each time you select to create alerts and this is intended since it's similar as it worked before. With the auto-creation of alerts, the toast notification appeared each time the user visits the stack monitoring page although nothing was happening if the user already had alerts created. So, for the first version, we thought that we could keep it the same.

kibanamachine · 2021-06-29T10:43:29Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`monitoring`	562	565	+3

Async chunks

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

id	before	after	diff
`monitoring`	731.2KB	737.8KB	+6.6KB

Page load bundle

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

id	before	after	diff
`monitoring`	45.5KB	45.5KB	+26.0B

History

💚 Build #134686 succeeded 0d5fe17
💚 Build #134493 succeeded 1dc3e18
💚 Build #133673 succeeded 9baade3
💔 Build #133562 failed de38d9f
💔 Build #133554 failed 2c8a545

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

* Remove api call to create alerts * Add enable alerts modal * Update modal title * Add simple alerts dropdown * change alerts modal design * refactor alerts modal provider * Add alerts dropdown * Show toast after alert creation and add error handling * Do not show alerts modal if alerts already exist * Fix stack monitoring test * Fix more stack monitoring tests and types * Fix tests after merge * Attempt to fix stack monitoring tests * remove console.log * Change text * Remove commented comment * Update docs for stack monitoring alerts * Fix docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-29T15:41:46Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

) * [Monitoring] Enable out of the box alerts modal (#101565) * Remove api call to create alerts * Add enable alerts modal * Update modal title * Add simple alerts dropdown * change alerts modal design * refactor alerts modal provider * Add alerts dropdown * Show toast after alert creation and add error handling * Do not show alerts modal if alerts already exist * Fix stack monitoring test * Fix more stack monitoring tests and types * Fix tests after merge * Attempt to fix stack monitoring tests * remove console.log * Change text * Remove commented comment * Update docs for stack monitoring alerts * Fix docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> * Removed strangely re-appearing test This test was removed a long time ago in this commit: 5feb376 -- we are not sure why it reappeared. Co-authored-by: Ester Martí Vilaseca <ester.martivilaseca@elastic.co> Co-authored-by: Jason Rhodes <jason.rhodes@elastic.co>

estermv added 11 commits June 1, 2021 18:12

Remove api call to create alerts

22b0067

Add enable alerts modal

766e0bf

Update modal title

736264d

Add simple alerts dropdown

e650ca3

change alerts modal design

7ef9d42

refactor alerts modal provider

8f1f893

Add alerts dropdown

28b0578

Show toast after alert creation and add error handling

7f7796f

Do not show alerts modal if alerts already exist

3de8db8

Fix stack monitoring test

258a9d1

Fix more stack monitoring tests and types

3f64d0a

estermv added 4 commits June 22, 2021 18:51

Merge remote-tracking branch 'upstream/master' into 100133-stack-moni…

42ba572

…toring-alerts-opt-in

Fix tests after merge

17ed5cb

Attempt to fix stack monitoring tests

2c8a545

remove console.log

de38d9f

Merge branch 'master' into 100133-stack-monitoring-alerts-opt-in

9baade3

estermv added 2 commits June 28, 2021 10:49

Change text

89a4d7c

Remove commented comment

a445413

estermv marked this pull request as ready for review June 28, 2021 08:51

estermv requested a review from a team June 28, 2021 08:51

Merge branch 'master' into 100133-stack-monitoring-alerts-opt-in

1dc3e18

estermv requested a review from a team June 28, 2021 08:51

estermv added release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.0.0 labels Jun 28, 2021

estermv added auto-backport Deprecated - use backport:version if exact versions are needed v7.14.0 labels Jun 28, 2021

This was referenced Jun 28, 2021

[Stack monitoring] Add rules modal to listing page #103456

Closed

[Stack Monitoring] Change out of the box alerts to be opt-in, rather than auto-created #100133

Closed

estermv requested a review from katefarrar June 28, 2021 14:52

mgiota reviewed Jun 28, 2021

View reviewed changes

simianhacker self-requested a review June 28, 2021 15:57

Update docs for stack monitoring alerts

0d5fe17

simianhacker approved these changes Jun 28, 2021

View reviewed changes

katefarrar suggested changes Jun 28, 2021

View reviewed changes

Fix docs

b646194

katefarrar approved these changes Jun 29, 2021

View reviewed changes

estermv merged commit 4640253 into elastic:master Jun 29, 2021

estermv deleted the 100133-stack-monitoring-alerts-opt-in branch June 29, 2021 15:35

kibanamachine mentioned this pull request Jun 29, 2021

[7.x] [Monitoring] Enable out of the box alerts modal (#101565) #103694

Merged

This was referenced Jun 30, 2021

[Monitoring] Change wording of the create option in the "Alerts and rules" dropdown #103920

Open

[Monitoring] Open modal when user selects "Create default rules" #104051

Open

[Monitoring] Add "Manage rules" option to "Alerts and rules" dropdown #104052

Closed

neptunian mentioned this pull request Jul 6, 2021

[Stack Monitoring] [Test Scenario] Out of the box alerting #104440

Closed

35 tasks

ravikesarwani mentioned this pull request Aug 5, 2021

redundant out-of-the box alerts are being created in every space #107785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Monitoring] Enable out of the box alerts modal #101565

[Monitoring] Enable out of the box alerts modal #101565

estermv commented Jun 8, 2021 •

edited by simianhacker

Loading

estermv commented Jun 22, 2021

kibanamachine commented Jun 22, 2021

ravikesarwani commented Jun 23, 2021

estermv commented Jun 23, 2021

ravikesarwani commented Jun 23, 2021 •

edited

Loading

estermv commented Jun 23, 2021

estermv commented Jun 28, 2021

estermv commented Jun 28, 2021

elasticmachine commented Jun 28, 2021

mgiota Jun 28, 2021

estermv Jun 28, 2021

simianhacker left a comment

katefarrar left a comment •

edited

Loading

estermv commented Jun 29, 2021

kibanamachine commented Jun 29, 2021

kibanamachine commented Jun 29, 2021

[Monitoring] Enable out of the box alerts modal #101565

[Monitoring] Enable out of the box alerts modal #101565

Conversation

estermv commented Jun 8, 2021 • edited by simianhacker Loading

Summary

Test cases

Technical notes

Checklist

estermv commented Jun 22, 2021

kibanamachine commented Jun 22, 2021

ravikesarwani commented Jun 23, 2021

estermv commented Jun 23, 2021

ravikesarwani commented Jun 23, 2021 • edited Loading

estermv commented Jun 23, 2021

estermv commented Jun 28, 2021

estermv commented Jun 28, 2021

elasticmachine commented Jun 28, 2021

mgiota Jun 28, 2021

Choose a reason for hiding this comment

estermv Jun 28, 2021

Choose a reason for hiding this comment

simianhacker left a comment

Choose a reason for hiding this comment

katefarrar left a comment • edited Loading

Choose a reason for hiding this comment

estermv commented Jun 29, 2021

kibanamachine commented Jun 29, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kibanamachine commented Jun 29, 2021

💚 Backport successful

estermv commented Jun 8, 2021 •

edited by simianhacker

Loading

ravikesarwani commented Jun 23, 2021 •

edited

Loading

katefarrar left a comment •

edited

Loading