Add Notification Service #19236

pickypg · 2018-05-18T22:42:24Z

This adds a notification service to Kibana that can be used to send
asynchronous notifications, such as sending email and Slack messages,
which are intended to be configured via a combination of the
kibana.yml and Kibana keystore.

Once configured, the actions are automatically added to the notification
service and can be invoked via the server using the notificationService
singleton or HTTP to send it directly. See the included README for more
details.

NOTE: This feature is currently experimental.

elasticmachine · 2018-05-20T05:10:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

pickypg · 2018-05-20T05:23:01Z

Jenkins test this

Advanced Setting UI test failure and map server test failure.

elasticmachine · 2018-05-21T18:43:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon

What beautiful code @pickypg! LGTM! Thanks for setting us off in the right direction.

stacey-gammon · 2018-05-21T19:47:34Z

x-pack/plugins/notifications/index.js

+ * Invokes plugin modules to instantiate the Monitoring plugin for Kibana
+ *
+ * @param kibana {Object} Kibana plugin instance
+ * @return {Object} Monitoring UI Kibana plugin object


Should Monitoring => Notifications ?

stacey-gammon · 2018-05-21T19:56:13Z

x-pack/plugins/notifications/server/email/email_action.js

+ */
+export class EmailAction extends Action {
+
+  constructor({ server, options = { }, defaults = { }, _nodemailer = nodemailer }) {


Options and defaults should be mutually exclusive, right? Defaults can be overridden when triggering a new email action, but options can't, and that is the difference?

Correct. I should have added docs for the parameters. :) Will fix.

I also removed the default for options since that means the server isn't defined.

stacey-gammon · 2018-05-21T20:14:46Z

x-pack/plugins/notifications/server/routes/api/v1/notifications/notify.js

+  return null;
+}
+
+export async function sendNotification(server, notificationService, actionId, data, reply, { _checkForErrors = checkForErrors } = { }) {


Is exposing _checkForErrors only because of tests? I think there is a way, perhaps if you put it into it's own file, that you can mock it so you don't have to expose it here just for testing purposes.

Yeah, I am not used to Jest yet, so I did not honestly consider mocking the import. I'll swap that around.

I ended up not changing this and instead documented the parameter's purpose (for tests). It's handy to control that function instead of having to mock all of the behaviors.

Which ... now that I typed it and reread it, I could do by mocking the import.

stacey-gammon · 2018-05-21T20:15:33Z

x-pack/plugins/notifications/server/routes/api/v1/notifications/notify.js

+  if (error === null) {
+    return action.performAction(data)
+      .then(result => reply(result.toJson()))
+      .catch(err => reply(wrap(err))); // by API definition, this should never hapepn as performAction isn't allow to throw errrors


hapepn = happen

stacey-gammon · 2018-05-21T20:43:24Z

x-pack/plugins/notifications/server/service/action_result.js

+   *
+   * @returns {Object|undefined} The error response, or {@code undefined} if no error.
+   */
+  getError() {


This class looks so clean! I like that you expose the inner variables as functions rather than leaving them to be directly accessed. That was something I was going back and forth on with my panel actions/embeddable stuff. Any particular reason you like this approach best? I don't think we are very consistent about it. Just curious.

Thanks!

I have been working with Typescript recently and it feels more appropriate to have a defined object rather than a bunch of arbitrary properties for a contract like this where some things are inferred. It also enables any future Actions to extend ActionResult rather than trying to figure out how to make a custom object that matches all of the same fields and their behavior (e.g., overriding isOk).

In terms of this code, even I am somewhat inconsistent. The missing fields response is a plain Javascript Object with a predictable structure. I thought about making a class out of it, but it doesn't need one because nothing is inferred or even dynamic about it -- it just needs a strict interface definition, which Typescript should bring us in pretty short order. Hopefully we'll develop better collective habits once we are able to embrace Typescript within the codebase.

You can still override properties without making getters (this.isOk = mycustomFunc in the super class), and I think you can still use a typescript interface to define variables as well as functions expected.

But last night I thought of a reason to prefer using getter functions - to give a place where you could put deprecation warnings if you ever wanted to get rid of some variables! At least that works well for a class that acts an a pluggable API.

this.isOk = mycustomFunc that's assuming isOk wasn't just ok as a field though. It's the whole concept of encapsulation coming from standard Object Oriented development. But I also like your explanation even more.

stacey-gammon · 2018-05-21T20:45:51Z

x-pack/plugins/xpack_main/server/lib/xpack_info_license.js

+   * Returns mode of the license (basic, gold etc.). This is the "effective" type of the license.
+   * @returns {string|undefined}
+   */
+  getMode() {


How is this different from type, above?

TL;DR It's generally the same thing as type, but it's the one that you should check (same as isOneOf does above in this class).

mode is the operational mode that ES has enabled (the license types we're used to). Technically it can be different from type, but in general they're the exact same value. This comes back from GET /_xpack, which xpack_main uses as opposed to GET /_xpack/license, which reports the mode / type as the same value.

elasticmachine · 2018-05-21T23:56:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisdavies

LGTM. Have a look at the markdown -> html comment. Not sure about that, or what effect it has on email clients to send html emails that aren't actually html...

chrisdavies · 2018-05-22T17:57:44Z

x-pack/plugins/notifications/server/email/email_action.js

+      bcc: notification.bcc,
+      // email content
+      subject: notification.subject,
+      html: notification.markdown,


Setting html to a markdown seems incorrect. Is this intentional? Seems like you'd want to convert markdown -> html here.

My plan was that we would evolve the Markdown support to be rendered as HTML, but I did not want to focus on that for now since it meant more complexity that we don't need yet.

Enabling it does provide a lot of potential benefits, but it also creates some interesting problems that I wanted to push back on. Namely, if you include image attachments I suspect that many people will want to include them in the body of the email and I didn't want to box us out of something by overstepping here.

chrisdavies · 2018-05-22T18:06:03Z

x-pack/plugins/notifications/server/slack/slack_action.js

+    const attachments = [];
+
+    if (markdown) {
+      attachments.push({ text: markdown });


I wonder if we should send raw markdown here or convert to HTML. Similar to my comment in the email action. I honestly prefer raw markdown, so I'm OK with this. Just wanted to point out that we may want to support prettifying it here.

Slack interprets Markdown on its own, so in this case I would not touch it.

This adds a notification service to Kibana that can be used to send asynchronous notifications, such as sending email and Slack messages, which are intended to be configured via a combination of the `kibana.yml` and Kibana keystore. Once configured, the actions are automatically added to the notification service and can be invoked via the server using the `notificationService` singleton or HTTP to send it directly. See the included README for more details.

elasticmachine · 2018-05-22T22:36:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

This adds a notification service to Kibana that can be used to send asynchronous notifications, such as sending email and Slack messages, which are intended to be configured via a combination of the `kibana.yml` and Kibana keystore. Once configured, the actions are automatically added to the notification service and can be invoked via the server using the `notificationService` singleton or HTTP to send it directly. See the included README for more details.

pickypg · 2018-05-22T23:10:08Z

6.x/6.4: e6a88e0

Originally added in elastic#19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

Originally added in #19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

Originally added in elastic#19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

Originally added in #19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

pickypg added WIP Work in progress :Sharing v7.0.0 enhancement New value added to drive a business result v6.4.0 labels May 18, 2018

pickypg requested review from chrisdavies and stacey-gammon May 18, 2018 22:42

pickypg force-pushed the feature/add-notification-service branch 2 times, most recently from 8f8e063 to 2d62a71 Compare May 20, 2018 04:03

pickypg added review and removed WIP Work in progress labels May 20, 2018

elastic deleted a comment from elasticmachine May 21, 2018

stacey-gammon approved these changes May 21, 2018

View reviewed changes

chrisdavies approved these changes May 22, 2018

View reviewed changes

pickypg added 5 commits May 22, 2018 17:32

update tests

986ff2d

fix copy/paste typo in readme

88c445a

adjust comment

cf16dcc

updating based on comments

d76ba5e

pickypg force-pushed the feature/add-notification-service branch from 844eff1 to d76ba5e Compare May 22, 2018 21:32

pickypg merged commit f2ccf90 into elastic:master May 22, 2018

pickypg deleted the feature/add-notification-service branch May 22, 2018 23:02

pickypg mentioned this pull request May 22, 2018

Slack integration #18276

Open

cjcenizal mentioned this pull request Aug 8, 2018

[toasts] migrate toastNotifications to the new platform #21772

Merged

tylersmalley mentioned this pull request Oct 25, 2019

[npm] Removes @slack/client #49273

Merged

tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Oct 25, 2019

[npm] Removes @slack/client

3e31660

Originally added in elastic#19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

tylersmalley pushed a commit that referenced this pull request Oct 25, 2019

[npm] Removes @slack/client (#49273)

a56d2d8

Originally added in #19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

tylersmalley pushed a commit that referenced this pull request Oct 25, 2019

[npm] Removes @slack/client (#49273) (#49343)

f96ca41

Originally added in #19236 as part of the notification service, however it's no longer used. Signed-off-by: Tyler Smalley <[email protected]>

pmuellr mentioned this pull request Apr 9, 2020

[Alerting] upgrade nodemailer to latest #63153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Notification Service #19236

Add Notification Service #19236

pickypg commented May 18, 2018

elasticmachine commented May 20, 2018

pickypg commented May 20, 2018

elasticmachine commented May 21, 2018

stacey-gammon left a comment

stacey-gammon May 21, 2018

stacey-gammon May 21, 2018

pickypg May 21, 2018

pickypg May 21, 2018

stacey-gammon May 21, 2018

pickypg May 21, 2018

pickypg May 21, 2018

pickypg May 21, 2018

stacey-gammon May 21, 2018

stacey-gammon May 21, 2018

pickypg May 21, 2018

stacey-gammon May 23, 2018

pickypg May 23, 2018

stacey-gammon May 21, 2018

pickypg May 21, 2018

elasticmachine commented May 21, 2018

chrisdavies left a comment

chrisdavies May 22, 2018

pickypg May 22, 2018

chrisdavies May 22, 2018

pickypg May 22, 2018

elasticmachine commented May 22, 2018

pickypg commented May 22, 2018

Add Notification Service #19236

Add Notification Service #19236

Conversation

pickypg commented May 18, 2018

elasticmachine commented May 20, 2018

💔 Build Failed

pickypg commented May 20, 2018

elasticmachine commented May 21, 2018

💚 Build Succeeded

stacey-gammon 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

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 21, 2018

💚 Build Succeeded

chrisdavies 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

elasticmachine commented May 22, 2018

💚 Build Succeeded

pickypg commented May 22, 2018