[Watcher] Support for adding actions to threshold watch #35175

alisonelizabeth · 2019-04-16T17:58:39Z

This PR adds support for adding actions via threshold alert.

Other action items:

Field validation for each action
Remove hipchat support
Server-side support for missing actions (Jira, PagerDuty, Index & Webhook)

Instructions

Instructions for setting up accounts to test actions.

Index, webhook and logging actions can be tested without any configuration in ES.

All other actions need at least one configured account to work.

Slack

Follow documentation here.

I used my own account and created a dummy channel: #alison-watcher-test. I plan on archiving after testing is complete.

PagerDuty

Follow documentation here.

Jira

Follow documentation here.

Please DM me for details on a Jira account to use.

Email

Follow documentation here.

Screenshots

Actions panel with no actions selected:

Actions dropdown:

Email action:

Logging action:

Slack action:

Webhook action:

Index action:

PagerDuty action:

Jira action:

Action with field error:

Advanced watch with invalid action:

elasticmachine · 2019-04-16T18:41:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-17T19:38:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-18T20:39:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-19T15:15:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…atch-actions

elasticmachine · 2019-04-22T13:22:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-22T15:19:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-22T17:56:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-23T18:52:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-23T19:52:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-23T20:42:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…atch-actions

alisonelizabeth · 2019-04-25T20:21:04Z

@cjcenizal I pushed some changes based on your review and a couple other bugs I noticed while testing.

Re:

The action_defaults files were responsible for adding some default values to the action fields, e.g., for logging, the default logging text is: "Watch [{{ctx.metadata.name}}] has exceeded the threshold." I thought it was all hard-coded text, so I moved it to each action’s model. However, now that I’m looking at the files again, I need to go back and verify the email action. It looks like it is also setting the “to” email address if xPack:defaultAdminEmail is set.

I fixed this by calling chrome.getUiSettingsClient() in the email-action model to check if there is a default email address set.

I definitely agree on the folder structure being hard to follow. What do you think about this? Note: I also moved watch_context out of the components dir.

I went ahead and made changes to the folder structure as shown in my above comment. Let me know what you think.

FYI: I need to do more thorough testing for Jira and PagerDuty actions, but I think logging, index, email and Slack are in good shape now.

elasticmachine · 2019-04-25T20:47:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-04-26T16:34:52Z

@bmcconaghy / @cjcenizal Something else I came across while testing…

The current code was calling the cluster settings API to determine if an action was enabled or not for this dropdown menu.

However, I don’t think this really achieves what we want, especially now that we have added support for Jira and PagerDuty. Technically, for Jira and PagerDuty (and Slack), a user only has to set some values via the keystore to configure. In this scenario, there would not be any values under the xpack.notificaton property in elasticsearch.yml that we’re checking against. Therefore, it would be marked as “not enabled” even though it is.

It looks like this may have come up before with Slack, and as a workaround, Slack is “enabled” by default in the code. I supposed we could do the same for Jira and PagerDuty, but in that case, the only thing this code is really checking for is email. Thoughts?

Code in question: https://github.com/alisonelizabeth/kibana/blob/threshold-watch-actions/x-pack/plugins/watcher/server/models/settings/settings.js

yaronp68 · 2019-04-26T16:55:16Z

@alisonelizabeth Great work. I find the action combo too much to the right (compared to other fields)

alisonelizabeth · 2019-04-26T17:13:51Z

Thanks, @yaronp68! It's currently right-aligned. I tried to replicate what existing previously, but I'm not sold on it either. Do you have any suggestions?

I wonder if it would help if there was more separation between the different sections? Another idea might be to just show the actions dropdown initially, and only render the text if an action has been added rather than having Will perform 0 actions... as a default. Then, maybe the text could sit underneath the dropdown, or something to that effect. Thoughts?

alisonelizabeth · 2019-04-26T20:32:18Z

Had a conversation with @bmcconaghy about the actions dropdown. Updated the code to work as follows:

We will continue to check the yml file to attempt to determine if an action is enabled or not.
For actions that can technically be set up through the keystore only (Slack, Jira and PagerDuty), we will not disable it in the dropdown and instead render a warning callout indicating that the account may not be set up, with a link to the configuration docs.

elasticmachine · 2019-04-26T20:53:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…atch-actions

elasticmachine · 2019-04-26T22:16:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-04-29T15:01:53Z

Great job @alisonelizabeth ! For now, I only did a local test of the UI (haven't reviewed the code yet), but here are some comments on the UI.

I get a EUI ComboBoxOptionsList warning in the Chrome console when type the index name, has this bug been raised to them?

In other apps we've decided to only show validation after the user has submitted the form so we don't "bother" him too early while he sets the form correctly. Would it be something easy to add here?
I wonder if we shouldn't change the "Add an action" to a primary button with a "+" icon in it. When clicking on it, the dropdown that is currently in place would replace the button, and there would be "cancel" link button right next to it to revert. What do you think? (cc @cjcenizal )
For the Webhook action, do we need to separate the URL in 3 fields? Couldn't we let the user simply enter

http://www.mydomain.com:7000/some-path
// or
https://mydomain.com:7000/some-path

and extract the fields from there? If it's complex we could leave it for a second pass but, as a user, I would expect to be able to copy paste a URL in one field.

One last thing, could you add in the PR description how we can test the integration (email, Slack, Jira, Pager duty)? I'm sure I would find it in the docs somewhere but it would be much easier to copy paste your elasticsearch.yml file 😊

alisonelizabeth · 2019-04-29T17:31:25Z

Thanks, @sebelga!

I updated the PR with links to some setup instructions. You can reach out to me regarding Jira, and of course if you have any other questions!

I didn’t work on this particular part, but noticed that warning as well. Opened: EuiComboBox: React warning related to duplicate keys eui#1901
I noticed that too. Left a comment here: [Watcher] Support for adding actions to threshold watch #35175 (comment). This is how the validation is working in all of the new watcher code currently. Perhaps we can address in a separate PR to align with other apps.
I’m open to changing the design here. Do you know if there’s an example somewhere that I could take a look at where we are following the pattern you described?
That’s a good idea. I’ll look into it.

cjcenizal · 2019-04-30T02:58:43Z

I wonder if we shouldn't change the "Add an action" to a primary button with a "+" icon in it. When clicking on it, the dropdown that is currently in place would replace the button, and there would be "cancel" link button right next to it to revert.

I think the idea of using a button is a good one. I'd suggest using the same pattern Jen used for rollup index pattern creation.

cjcenizal

I went ahead and made changes to the folder structure as shown in my above comment. Let me know what you think.

Looks much better! Thank you for doing that.

Great work on this @alisonelizabeth. This code LGTM. At a later point I think we should create a test plan for this, based on the original Watcher behavior. For this test plan, I'd expect:

Explicit steps on how to get set up, e.g. a copy-pasteable elasticsearch.yml file as Seb suggested.
Step-by-step instructions for testing each action, with notes on what the tester should expect to happen in the UI in response to different input (e.g. callouts, validation states).
Notes on any special behaviors, e.g. setting the admin email will pre-populate the Email action's to field.

These are my just initial thoughts. @silne30 owns testing and QA so once we get to that point we'll look to him for guidance.

cjcenizal · 2019-04-30T03:16:05Z

Retest

elasticmachine · 2019-04-30T04:06:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga

LGTM. I added a few comments about code style, look if you can address them.

About the client side validation that you removed, can you double checked that you went in "create advanced watch" and made sure we don't need them any more there?

Make sure we don't reintroduced the bug from #23887. where a badly formatted watch could be created in the console, and then could not be editted in the Watcher UI.

Cheers!

sebelga · 2019-04-30T10:58:40Z

x-pack/plugins/watcher/public/models/action/email_action.js

+    if (!this.to || !this.to.length) {
+      errors.to.push(
+        i18n.translate('xpack.watcher.watchActions.email.emailRecipientIsRequiredValidationMessage', {
+          defaultMessage: 'To email address is required.',


I'm not native speaker, but shouldn't we quote the "to"? "To" email property is required.

Good point. Updated, but also plan on doing a copy review with gail.

sebelga · 2019-04-30T11:14:05Z

x-pack/plugins/watcher/public/models/action/slack_action.js

  }

  validate() {
-    const errors = [];
-
-    if (!this.to.length) {


So the validation that was added in #23887 was that we can't validate the "to" field as it is not mandatory in the UI if it has been set in elasticsearch.yml. Why do you think it is not necessary anymore to warn the user of a possible error?

see comment regarding this above. if you still have concerns, please let me know!

sebelga · 2019-04-30T11:20:18Z

x-pack/plugins/watcher/public/models/watch/base_watch.js

-   * Client validation of the Watch.
-   * Currently we are *only* validating the Watch "Actions"
-   */
-  validate() {


Could you remind me why we don't do any more validation client side? This is both used in Threshold alert and advanced watch.

There is still validation, but it was added as part of the specific watch (see: models/watch/threshold_watch.js, models/watch/json_watch.js). Each action model also has its own validation.

sebelga · 2019-04-30T11:26:43Z

...ic/sections/watch_edit/components/threshold_watch_edit/action_fields/index_action_fields.tsx

+}) => {
+  const { index } = action;
+  return (
+    <Fragment>


nit: no need to wrap a Fragment here

sebelga · 2019-04-30T11:28:39Z

.../sections/watch_edit/components/threshold_watch_edit/action_fields/logging_action_fields.tsx

+}) => {
+  const { text } = action;
+  return (
+    <Fragment>


nit: no need to wrap with Fragment here.

sebelga · 2019-04-30T11:40:27Z

...lic/sections/watch_edit/components/threshold_watch_edit/threshold_watch_action_accordion.tsx

+    name: string;
+    iconClass: 'loggingApp' | 'logoWebhook' | 'logoSlack' | 'apps' | 'email' | 'indexOpen';
+  }) => (
+    <Fragment>


nit: Fragment is not needed here. You're like @jen-huang , you love Fragments! 😄

haha. thanks for the catch - fixed. i originally had this set up a little differently where it was necessary and forgot to remove.

sebelga · 2019-04-30T11:43:29Z

...lic/sections/watch_edit/components/threshold_watch_edit/threshold_watch_action_accordion.tsx

+          buttonContentClassName="euiAccordionForm__button"
+          buttonContent={<ButtonContent name={action.typeName} iconClass={action.iconClass} />}
+          extraAction={
+            <DeleteIcon


Why not directly use the <EuiButtonIcon /> here?

good point, fixed.

sebelga · 2019-04-30T11:53:57Z

...lic/sections/watch_edit/components/threshold_watch_edit/threshold_watch_action_accordion.tsx

+  const { watch, setWatchProperty } = useContext(WatchContext);
+  const { actions } = watch;
+
+  const ButtonContent = ({


Can we declare the component outside the WatchActionsAccordion? It seems strange to me to declare a component from inside another one. It would then name it EuiAccordionButton probably.

good point. i decided to directly declare this for the buttonContent prop

sebelga · 2019-04-30T12:02:17Z

...blic/sections/watch_edit/components/threshold_watch_edit/threshold_watch_action_dropdown.tsx

+  const allActionTypes = Action.getActionTypes();
+
+  const actions = Object.keys(allActionTypes).map(actionKey => {
+    const { typeName, iconClass, selectMessage } = allActionTypes[actionKey];


nit: you could simplify with Object.entries

const allActionTypes = Action.getActionTypes() as Record<string, any>; const actions = Object.entries(allActionTypes).map(([type, { typeName, iconClass, selectMessage }]) => ({ type, typeName, iconClass, selectMessage, isEnabled: settings ? settings.actionTypes[type].enabled : true, }));

thanks - updated

sebelga · 2019-04-30T12:10:33Z

x-pack/plugins/watcher/public/sections/watch_edit/watch_edit_actions.ts

@@ -75,21 +75,30 @@ export async function saveWatch(watch: BaseWatch, licenseService: any) {
 }

 export async function validateActionsAndSaveWatch(watch: BaseWatch, licenseService: any) {
-  const { warning } = watch.validate();


I am not sure this is correct. From the comment

The way I setup my Slack Webhook for this watch, it has a default "to" on the slack side.

So a "to" is required. The thing is, the user can set it multiple ways. The warning that was put in place is: "we can't validate that you have set properly a "to" field, be aware that the watch might not work if you haven't specified it outside our UI".

This validation was useful if the user enters "Create advanced watch", add a JSON payload of the watch with a Slack action, but without a "to" field.

…atch-actions

elasticmachine · 2019-05-01T14:32:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-05-01T14:37:48Z

@sebelga, thanks for your review! I believe I addressed all of your comments. Also, fixed a couple bugs I came across while making changes. Can you take another look when you get a chance?

I’d like to address the design issues in a follow-up PR if that works for you (changing the actions dropdown to a button, and updating the webhook form to have one field for host/port/path)

I opened an issue against EUI regarding the ComboBox, but realized that it is working as intended. We might need to rethink how we’re using it. See: elastic/eui#1901.

I tested the scenario mentioned in #23887. It works as expected in edit mode. However, it looks like we are missing the error column and corresponding modal on the details pages (see screenshots of old UI below). This is unrelated to my changes, so I think this should be addressed in a separate PR. FYI @cjcenizal

elasticmachine · 2019-05-01T17:43:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-05-01T17:46:57Z

retest

elasticmachine · 2019-05-01T18:39:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…atch-actions

elasticmachine · 2019-05-03T19:23:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

initial support for adding actions via threshold watch

b107924

alisonelizabeth force-pushed the threshold-watch-actions branch from a4af6cc to b107924 Compare April 16, 2019 18:01

alisonelizabeth added the Feature:Watcher label Apr 16, 2019

added validation; support for email and slack action types

38e3d55

added support for index action

ab7f44e

added support for webhook action

ac0b569

alisonelizabeth force-pushed the threshold-watch-actions branch from d939599 to ac0b569 Compare April 19, 2019 14:53

alisonelizabeth added 2 commits April 22, 2019 08:59

added support for pagerduty action

f3ce9ca

Merge remote-tracking branch 'upstream/watcher-port' into threshold-w…

419c349

…atch-actions

added support for jira action

b25f6ee

alisonelizabeth changed the title ~~Threshold watch actions~~ [Watcher] Support for adding actions to threshold watch Apr 23, 2019

alisonelizabeth force-pushed the threshold-watch-actions branch from b4c6218 to feae801 Compare April 23, 2019 18:32

alisonelizabeth force-pushed the threshold-watch-actions branch from feae801 to 86974fd Compare April 23, 2019 19:13

alisonelizabeth force-pushed the threshold-watch-actions branch from 86974fd to 23cf1c3 Compare April 23, 2019 20:01

cleanup

7d85488

alisonelizabeth force-pushed the threshold-watch-actions branch from 23cf1c3 to 7d85488 Compare April 24, 2019 18:03

alisonelizabeth added the non-issue Indicates to automation that a pull request should not appear in the release notes label Apr 24, 2019

alisonelizabeth added 2 commits April 24, 2019 14:16

Merge remote-tracking branch 'upstream/watcher-port' into threshold-w…

7c4d032

…atch-actions

remove extra spacers

e858f66

alisonelizabeth added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Apr 24, 2019

adjust logic that determines if actions are enabled/disabled

85fc8df

alisonelizabeth force-pushed the threshold-watch-actions branch from 6aba211 to 85fc8df Compare April 26, 2019 20:58

Merge remote-tracking branch 'upstream/watcher-port' into threshold-w…

762eb44

…atch-actions

cjcenizal approved these changes Apr 30, 2019

View reviewed changes

sebelga reviewed Apr 30, 2019

View reviewed changes

alisonelizabeth added 2 commits May 1, 2019 09:51

addressing pr feedback

884335c

Merge remote-tracking branch 'upstream/watcher-port' into threshold-w…

514149b

…atch-actions

fix linting error

3aca320

Merge remote-tracking branch 'upstream/watcher-port' into threshold-w…

3896071

…atch-actions

alisonelizabeth merged commit 0835f38 into elastic:watcher-port May 3, 2019

alisonelizabeth deleted the threshold-watch-actions branch May 3, 2019 19:25

[Watcher] Support for adding actions to threshold watch #35175

[Watcher] Support for adding actions to threshold watch #35175

Conversation

alisonelizabeth commented Apr 16, 2019 • edited Loading

Instructions

Slack

PagerDuty

Jira

Email

Screenshots

elasticmachine commented Apr 16, 2019

💔 Build Failed

elasticmachine commented Apr 17, 2019

💔 Build Failed

elasticmachine commented Apr 18, 2019

💔 Build Failed

elasticmachine commented Apr 19, 2019

💔 Build Failed

elasticmachine commented Apr 22, 2019

💔 Build Failed

elasticmachine commented Apr 22, 2019

💔 Build Failed

elasticmachine commented Apr 22, 2019

💔 Build Failed

elasticmachine commented Apr 23, 2019

💔 Build Failed

elasticmachine commented Apr 23, 2019

💔 Build Failed

elasticmachine commented Apr 23, 2019

💚 Build Succeeded

alisonelizabeth commented Apr 25, 2019

elasticmachine commented Apr 25, 2019

💚 Build Succeeded

alisonelizabeth commented Apr 26, 2019

yaronp68 commented Apr 26, 2019

alisonelizabeth commented Apr 26, 2019

alisonelizabeth commented Apr 26, 2019

elasticmachine commented Apr 26, 2019

💔 Build Failed

elasticmachine commented Apr 26, 2019

💔 Build Failed

sebelga commented Apr 29, 2019

alisonelizabeth commented Apr 29, 2019 • edited Loading

cjcenizal commented Apr 30, 2019

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal commented Apr 30, 2019

elasticmachine commented Apr 30, 2019

💔 Build Failed

sebelga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisonelizabeth May 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 1, 2019

💔 Build Failed

alisonelizabeth commented May 1, 2019 • edited Loading

elasticmachine commented May 1, 2019

💔 Build Failed

alisonelizabeth commented May 1, 2019

elasticmachine commented May 1, 2019

💚 Build Succeeded

elasticmachine commented May 3, 2019

💚 Build Succeeded

alisonelizabeth commented Apr 16, 2019 •

edited

Loading

alisonelizabeth commented Apr 29, 2019 •

edited

Loading

alisonelizabeth May 1, 2019 •

edited

Loading

alisonelizabeth commented May 1, 2019 •

edited

Loading