Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Personalisation] Notifications API MVP #140743

Closed
gsoldevila opened this issue Sep 14, 2022 · 23 comments
Closed

[Personalisation] Notifications API MVP #140743

gsoldevila opened this issue Sep 14, 2022 · 23 comments
Labels
impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@gsoldevila
Copy link
Contributor

gsoldevila commented Sep 14, 2022

Overview

In the scope of personalisation & collaboration, we need a way to notify users when they are linked to a certain entity / event:

  • One or more users have been assigned to a case.
  • A case assigned to a user receives an update.
  • A user has been @mentioned somewhere in the Kibana UI.
  • ...

The personalisation & collaboration initiative is likely going to have multiple use cases impacting different solutions and plugins. The goal of this issue is to describe an API that will act as a centralised place for all these use cases to send notifications.

image

The Notifications API will rely on 2 basic building blocks:

  • User profile service (security plugin). The Notifications API will query this service to fetch users' preferences regarding notifications. Users must be able to opt-out from / opt-in to different notifications types and channels. E.g. a user might decide they want to receive only Slack notifications for Case mentions, and Slack + Email notifications for Case assignments.
  • Email, Slack connectors (actions plugin). These components exist in Kibana and can be configured to send notifications through different channels. The idea is for the Notifications API to capitalise on this existing logic.

Notification flow (draft)

Here's the sequence diagram describing the Email notification flow (MVP):

image

Q & A

  • QUESTION: Do we want to support multi-person notifications? e.g: do we want to send emails to multiple recipients at once? do we want to send MPIMs (Multi Person Instant Messages) through slack?

    • ANSWER: From a functional standpoint, there are scenarios in which it could be interesting to have multi person notifications, e.g. sending the same Case assignment email to multiple users at once, fostering communication between them. From a technical standpoint, this feature is currently supported by certain connectors (e.g. Email) but not yet supported (there's an implementation gap) by others (e.g. Slack). Since the feature might be interesting in some use cases, we could expose a groupIfPossible?: boolean parameter in the future to drive this behaviour (with a default of false).
    • ANSWER 2: Whilst it is relatively easy to implement such feature in terms of effort, we prefer to have users interact within Kibana rather than externally, e.g. on an email thread.
  • QUESTION: Will the Notifications API be a part of the Core services?

    • ANSWER: Since it depends on a couple of building blocks that are shaped as plugins, the Notifications API itself must be a plugin (Core services cannot depend on plugins).
  • QUESTION: Will the Notifications API be a REST endpoint or a programatic service?

    • ANSWER: For the time being, most of the use cases require interaction with server side. Thus, we can start exposing a programatic service, e.g. NotificationsService, which will provide convenient methods for plugins and solutions to send notifications. Nothing prevents us from exposing HTTP endpoints for that service at a later phase if the need arises.
  • QUESTION: Case assignment might be the first use case, but the roadmap foresees multiple notification types in the future. Where do we store the list of notification types?

    • ANSWER: It would seem that this knowledge should be stored somewhere within the Notifications API plugin. The NotificationsService will need that knowledge in its logic.
      • The User preferences UI that allows opting in and out will also need that list. In that sense, the NotificationsService could expose a couple of methods to read and update the notification preferences for the current user.
      • ⚠️ This notification type will need to be part of the API sooner rather than later, as we want users to be able to opt in / out of each of the types. So even if there's only one type for now (Case assignment), I believe it's worth taking it into account already.
  • QUESTION: Rich text formats. What formatting options will the notifications API support? Is it acceptable to send notifications in plain text?

    • ANSWER: The requirement for MVP is to be able to send plain text with a backlink to Kibana. We can expose a method to send plain text messages on a first phase, and later on enrich the API to provide other formatting options.
      • Note that different connectors offer different formatting options:
        • Email connector supports HTML and Markdown syntax in the body of the messages.
        • Slack connector supports plain text, but also BlockKit elements (structured JSON), along with mrkdwn syntax (a subset of Markdown language).
  • QUESTION: Should the API support localised notification messages?

    • ANSWER: Desired feature, not mandatory for MVP phase 1.
    • ⚠️ Could we delegate this responsibility to plugin owners for now?
  • QUESTION: Can new notification types be dynamically added by other plugins?

    • ANSWER: That would make plugin development not dependant on notification API owner, kind of how it works for EBT. Notification types can be registered at startup through the setup contract, and they can decide a default behaviour for each type. (opt-in vs opt-out).
  • QUESTION: Should the API support dynamic messages or only prebuilt ones? If we have to send the exact same messages over and over, perhaps it could be interesting to have some sort of NotificationPayloadBuilder which, given a MessageId, could build the payload to send to each connector. This would require an interface to manage predefined messages though.

    • ANSWER: Having prebuilt messages registered for each notification type would open the door to RTF and i18n, but it adds a bunch of complexity, thus, we propose to leave it out for MVP.
  • QUESTION: How do we determine the connector to be used for each notification channel?

    • ANSWER: Cloud deployments have a preconfigured email connector, so we could hardcode its ID for MVP. However, this is not very flexible, and it does not cover self-managed deployments. We should probably:
      • add new settings in kibana.yaml
      • or use SavedObjects (along with a UI to manage them?)
      • or some combination of the above
    • ANSWER 2: To keep it as simple as possible, we can rely on the existing email connector for cloud deployments, and make sure administrators create a preconfigured one for on-prem. For the later, we will have to rely on some sort of convention to select the appropriate connector (if there's more than one): We could use a naming convention, or we could simply select the first one.
  • QUESTION: Do we want to support using different connectors for each notification type? E.g. emailConnectorA for @mentions, emailConnectorB for Case assignment?

    • ANSWER: We can have a single connector for each channel for now, and then enhance the configuration in a non-breaking way at a later phase, in order to admit a list of connectors.
  • QUESTION: How do we let administrators (or users) know that notifications will NOT work because they have not been properly configured? Note that the appropriate connectors need to be configured in Kibana, and the NotificationsService must know which connector(s) to use for each type.

    • ANSWER:

Notifications API specification

Only after answering all of the above questions can we propose a convenient contract for the NotificationsService.
Here I'll try to reflect the current state of proposed API (subject to change as the technical design evolves):

interface NotificationsServiceSetup {
  registerNotificationType(plugin: string, type: string, autoOptIn: boolean);
}

interface NotificationsServiceStart {
  sendPlainTextNotification(payload: PlainTextNotification): Promise<boolean>;
}

interface AbstractNotification {
  recipient: string; // userID
  plugin: string;
  type: string; 
}

interface PlainTextNotification extends AbstractNotification {
  subject?: string;
  message: string;
}
@gsoldevila gsoldevila added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:AppServicesSv labels Sep 14, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@lukeelmers
Copy link
Member

Thanks for getting this started! Overall I'm particularly interested in how we can prevent connector-specific concepts from leaking into the API, since the idea is that we will eventually have a longer term platform-provided concept of connectors.

With that in mind, a few initial thoughts:

Do we want to support multi-person notifications?

I'm unsure if the benefits we get from a groupIfPossible option are worth the added complexity of providing it up front. Is it strictly needed at this point? If not, I'd vote we wait until we have a better understanding of the long-term architecture of the notifications/connectors system, and layer it in as a future enhancement. Always easier to add something later than to take it away...

the NotificationsService could expose a couple of methods to read and update the notification preferences for the current user.

I'd imagine that eventually we'd want that dependency to be inverted, so that there's a user-settings-service (perhaps similar to uiSettings), that the Notifications plugin would register these settings into and read from. Plus we might run into some circular dependency issues -- if Notifications need to read user profile to get settings from Security, Security won't be able to update preferences if it lives in Notifications.

How do we let administrators (or users) know that notifications will NOT work because they have not been properly configured?

Maybe we could start by just returning an error from the Notifications service so that the consuming application (Cases) could decide what to do with it? E.g. maybe they want to throw a toast notification or render an inline message mentioning that users have not been notified? idk...

Can new notification types be dynamically added by other plugins?
Should the API support dynamic messages or only prebuilt ones?
Do we want to support using different connectors for each notification type?

I'd vote 'no' on all of these items for MVP/Phase1 if we don't have a strong need for them 🙂 That way we keep it as simple as possible so we aren't locking ourselves into too many decisions before @elastic/kibana-app-services starts getting more involved.

@cnasikas
Copy link
Member

cnasikas commented Sep 16, 2022

Maybe we could start by just returning an error from the Notifications service so that the consuming application (Cases) could decide what to do with it? E.g. maybe they want to throw a toast notification or render an inline message mentioning that users have not been notified? idk...

I think that the process of notifying should not block HTTP requests. A case should be created/updated even if the user is not notified (error while sending an email) or notifications are not configured. I think the error should surface somewhere else. Ideally, in the admin notification UI page if there is one. Cases could also show a callout on the all cases page informing the user that notifications are not properly configured and cases cannot notify assignees. This would require the notification plugin (or the plugin responsible for the notification configuration) to expose from the start method of the plugin side of the plugin a boolean where cases can check and act accordingly.

@exalate-issue-sync exalate-issue-sync bot added the impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. label Sep 19, 2022
@pgayvallet
Copy link
Contributor

my initial remarks and comments (note: I did not look at @lukeelmers's reply to not be influenced)

QUESTION: Do we want to support multi-person notifications?

For the initial MVP of the revamped NC, idk. For the old/initial purpose of the notification service, this was a mandatory requirement.

Now, it was not about using, per example, slack's MPIM (which is somehow a tight coupling between a notification and its associated action, but I'll get back on that), just to have an API to send a given notification to multiple users to have the grouping/batching logic handled internally (which can be mandatory for some use case, e.g sending a mail to a large group of users)

QUESTION: Will the Notifications API be a part of the Core services?

Given the suggested architecture, I agree that it can only be a plugin.

Now I have to ask: are we sure we'll never need to use this API from Core services at some point? (and if we do, do we know how we will technically be able to do so? IOC with registration API as we do for telemetry collectors?)

QUESTION: Case assignment might be the first use case, but the roadmap foresees multiple notification types in the future. Where do we store the list of notification types?
ANSWER: It would seem that this knowledge should be stored somewhere within the Notifications API plugin. The NotificationsService will need that knowledge in its logic.

Why not allow consumers to register their notification types, as it's done for EBT for example? It seems like the proper isolation of concerns, and would avoid problem in code ownership where other contributors need to update the notification plugin to add their new types?

QUESTION: Rich text formats. What formatting options will the notifications API support? Is it acceptable to send notifications in plain text?

The problem with any enhanced text format is that is introduce a tight coupling between the notification and the connector. E.g depending if a given notification will be sent via mail or to slack, the actual formatting capabilities aren't the same.

QUESTION: Should the API support localised notification messages?
ANSWER:[...] we’d like the API future-proof, keeping localisation in mind.

Meaning what in term of impact on the design/API surface, exactly?

Note that this highly depends on the next point,

QUESTION: Should the API support dynamic messages or only prebuilt ones?

Excellent question.

Supporting only prebuild messages would allow to somehow solve the RTF/i18n problematic, as it would be handled internally by the notification API (you can have a formatter per connector for instance). Now, I suspect that in practice, it may not be sufficient (or would force to introduce hundreds of notification types very quickly), but I lack the proper vision to understand exactly what we're planning to do long term.

QUESTION: How do we determine the connector to be used for each notification channel?

Also something that should absolutely be decided in the initial design. More globally, what's the relations between a notification (the data) and a notification action (sending it to a channel / though a connector)? Is that static / predetermined? configurable? per user? overridable per notification instance?

QUESTION: Do we want to support using different connectors for each notification type?

Maybe I don't understand, but if we don't, we're bound to have a single connector for all and every notification types, don't we?

@gsoldevila
Copy link
Contributor Author

There are multiple opinions regarding multi-person notifications. Initially, I added it cause I saw it as a feature that could be interesting from a functional standpoint, and also because we have it "for free" with the current Email connector.
However, it would increase the complexity to add a Slack connector in the future, if we are to honour the flag (meaning we would have to enhance the connector to support MPIM notifications).

Also, seeing as though this parameter will likely not change for each "notification action", I believe it's probably better to add it at a later phase and to have it as a configuration param. I'll update the API accordingly.


Plus we might run into some circular dependency issues -- if Notifications need to read user profile to get settings from Security, Security won't be able to update preferences if it lives in Notifications.

@luke I was imagining it more as a layer on top of Security:

User Profile UI => Notifications Service => Security (notif. user prefs)
// and also
User Profile UI => Security (other user prefs)

The question I ask myself is: imagine the case where a user has not yet customised their notification preferences. How will the User Profile UI know which notification types (and channels) are there, to present the different checkboxes?
So even if we don't provide the specific user preferences that come from profile info, I believe we must at least provide a list of the existing notification types and channels.

We could hardcode it somewhere in an Enumeration, or as @pgayvallet suggests, I suppose we can allow consumers to register new notification types and store them ?in memory? (how does it work for EBT?). The later seems a lot more flexible, but it's probably harder to implement. Don't know what's the best strategy from an MVP standpoint.

That is unless the user preferences itself is backed by some sort of saved object that allows storing meta information about the preferences that exist. @arisonl ?


@pgayvallet thanks for the insight in RTF/i18n problematic. The API surface depends on it:

  • A) If we use free text, we simply accept the input text and pass it on as payload.
  • B) If the API performs the translation, what we have to accept is a translation key, rather than a message.
  • C) If we implement the concept of Notification entity (as opposed to notification action) we can store RTF templates and i18n information linked to that entity, but then from API standpoint we need to accept some sort of NotificationId instead.

Anyway, these approaches are not mutually exclusive, we could start with a service method for A) for the MVP, and then add new methods to the service for B) and C) at a later phase.


QUESTION: Do we want to support using different connectors for each notification type?

Maybe I don't understand, but if we don't, we're bound to have a single connector for all and every notification types, don't we?

Sorry, the question is not very clear. I was imagining a scenario where, for instance, an organisation had a couple different Slack workspaces, and they wanted to send notifications to users that are in either of these spaces. In that line, my question was whether it would be interesting to support configurations such as:

caseAssignmentNotificationConnectors: ['emailConnector', 'slackConnector1', 'slackConnector2']

So in fact, the question I was asking myself was more: Configuration-wise, for a given notification type (e.g. case assignment), should we support more than one connector of the same "type" (e.g. slack)? I imagine it wouldn't be too hard, and this way we are covered if presented with such scenario.

@pgayvallet
Copy link
Contributor

There are multiple opinions regarding multi-person notifications. Initially, I added it cause I saw it as a feature that could be interesting from a functional standpoint
it would increase the complexity to add a Slack connector in the future, if we are to honour the flag

I think we're not speaking of the same thing. As I said previously, I wasn't referring to notifications 'shared' between users (e.g a multi-users message in slack), but the capability to send notifications to a list / group of different users at the same time and using a single notification API call.

Now, if that's not required for the MVP, great.

User Profile UI => Notifications Service => Security (notif. user prefs)

I think the problem remains here: what if the security plugin wants to use the notification service? It adds a cycle here.

Note that the problem may not be in the current issue's proposed design, but in the fact that the security plugin is currently a single block doing too many things (similar to the cloud plugin for instance).

So in fact, the question I was asking myself was more: Configuration-wise, for a given notification type (e.g. case assignment), should we support more than one connector of the same "type" (e.g. slack)? I imagine it wouldn't be too hard, and this way we are covered if presented with such scenario.

Thanks, it's clearer now.

Then I would say it looks fairly easy to add later in a non-breaking way (change config structure to allow both a string or an array of string), in which case, if not required for the MVP, we can probably ignore it for now?

@exalate-issue-sync exalate-issue-sync bot added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Sep 26, 2022
@Dosant
Copy link
Contributor

Dosant commented Sep 29, 2022

the requirement for MVP is to be able to send plain text with a backlink to Kibana.

Who will generate the backlink to Kibana? Will this be done automatically inside of the service or does it have to be done by consumers who will have to append the link to the message string?

@cnasikas
Copy link
Member

I think it should be the responsibility of the consumers because they want to link to different pages in Kibana. For example in Cases, we would like to add a backlink to the case the user got assigned.

@gsoldevila
Copy link
Contributor Author

The idea was to keep the API as simple as possible, so the accepted message will be plain text. Thus, the API will not check whether text contains a link.
Then, this also means that we will rely on the fact that most email clients (Gmail, iOS, ...) perform some processing and make the links clickable.

@vadimkibana
Copy link
Contributor

vadimkibana commented Oct 4, 2022

As discussed offline today, and I've also spoken with few people about Alerting Connectors today, would like to propose the following:

We would create a notifications plugin in X-pack (which will later host the Notifications Center), with an email service, which has a .send() method.

plugins.notifications.email.send({
  to: 'abc@gmail.com',
  textBody: 'You were tagged in a case ...',
  // more fields ...
});

From the current technical limitations, it is best for now to use Alerting email Connectors, especially the pre-configured cloud email connector, and Alerting Action, which is executed immediately (without putting it into the Task Manager queue).

Would be great if we could just provide the email.send() method to Cases, without adding concepts of "notification" and "connector", to the "notifications" plugin for now.

Like above caseAssignmentNotificationConnectors, registerNotificationType, etc...

As it is not clear if the Notifications Center will have a concept of "connectors", and "notifications" in the Notifications Center will be way more complex persistent entities.

IMO, would be great for now if Cases could just use the plugins.notifications.email.send() method directly, which means they would need to get hold of the user's profile (I assume that already should be the case), from which email and locale (if we are storing it) could be used to localize and send the email.

// Once case was assigned
const {email, fullName, locale} = await plugins.users.loadProfile(assigneeId);
await plugins.notifications.email.send({
  to: email,
  textBody: localizeNotificationMessage(locale, fullName, caseId),
});

In the future, we could provide the email sending as some fundamental service from Core, then notifications email service could just be thought of as a temporary proxy for that:

plugins.notifications.email.send = (params) => core.email.send(params);

@lukeelmers
Copy link
Member

would be great for now if Cases could just use the plugins.notifications.email.send() method directly, which means they would need to get hold of the user's profile (I assume that already should be the case), from which email and locale (if we are storing it) could be used to localize and send the email.

No strong feelings from me one way or the other, but this approach makes sense to me. I'm +1 on anything we can do to avoid tying this API too closely to concepts that may change later. Connectors is the obvious one, but I suppose you could say the same for Notifications. Also, IIRC we are not considering localization part of the MVP, so that wouldn't be necessary for now.

@cnasikas what do you think of this proposal?

In the future, we could provide the email sending as some fundamental service from Core,

TBH I'm not sold on the idea of email being a core service, especially if we imagine a SaaS future where we could potentially have a standalone service for something like this. But that's outside the scope of this discussion, and not something we need to reach alignment on now.

@cnasikas
Copy link
Member

cnasikas commented Oct 5, 2022

I also favor not tying closely concepts that may change later. Cases still need to bulk get the user profiles to get the email assignees which is ok.

which is executed immediately (without putting it into the Task Manager queue).

Regarding executing immediately, I believe we should not go with this approach and use the task manager. Cases' endpoints should not be blocked to send an email. If the notification service does not use the task manager, consumers (Cases) will have to use the task manager leading to multiple implementations of the same task. Lastly, If in the future the notification service decides to use the task manager, Cases need to stop using the task manager to avoid creating two jobs: one for Cases and one for the notification service. In the long run, it will be better if the notification service uses the task manager and have control of how notifications are being sent.

We would create a notifications plugin in X-pack (which will later host the Notifications Center), with an email service, which has a .send() method.

A case can have multiple assignees. I think we should take that into consideration and let the send method accept an array of emails and send one email to each assignee (or bulk send emails or similar).

@kobelb
Copy link
Contributor

kobelb commented Oct 5, 2022

Regarding executing immediately, I believe we should not go with this approach and use the task manager. Cases' endpoints should not be blocked to send an email. If the notification service does not use the task manager, consumers (Cases) will have to use the task manager leading to multiple implementations of the same task. Lastly, If in the future the notification service decides to use the task manager, Cases need to stop using the task manager to avoid creating two jobs: one for Cases and one for the notification service. In the long run, it will be better if the notification service uses the task manager and have control of how notifications are being sent.

Agreed. The other benefit we get from this approach is that we will transparently retry sending these emails when there's a transient error once we start supporting at-least-once action runs.

@gsoldevila
Copy link
Contributor Author

gsoldevila commented Oct 14, 2022

I also favour not tying closely concepts that may change later. Cases still need to bulk get the user profiles to get the email assignees which is ok.

That's a good point. For the time being the name of the new plugin is pretty much the only mention we have about 'notifications'. It exposes an EmailService that allows sending mails, plain and simple, the bare minimum to fulfil MVP requirements.

Regarding executing immediately, I believe we should not go with this approach and use the task manager.

Sorry if my description was confusing, I was talking about a send() method, but I didn't want to imply the email would be sent straight away. The idea is that we're going to call a bulkEnqueueExecution() method, containing a list of all the executions that need to be run by task manager. If I understood correctly, this will create a single SO, that will then be read by task manager, and deleted once the corresponding emails have been sent.

A case can have multiple assignees. I think we should take that into consideration and let the send method accept an array of emails and send one email to each assignee (or bulk send emails or similar).

Indeed, the API is going to accept a list of recipients, and it will schedule a single bulk action with the list of all the executions.

@cnasikas
Copy link
Member

Thank you, @gsoldevila, and sorry for the confusion!

@vadimkibana
Copy link
Contributor

vadimkibana commented Oct 17, 2022

@cnasikas @kobelb @gsoldevila I was hoping we can execute an action immediately, using the execute() method. (I assume that execute() does not create a saved object.) So we can avoid creating an unnecessary saved object and adding it to the Task Manager queue.

Or am I missing something? Do we gain something by scheduling a task in the Task Manager? Or does it have to do with UI Kibana and Background Task Kibana split, where UI Kibana does not know how to execute an Action?

[..] Cases' endpoints should not be blocked to send an email. [..]

@cnasikas What do you mean here exactly? The execution should still be async, hence can run in the background:

async sendNotifications() {
  plugins.notifications.email.sendPlainTextEmail(opts).catch(() => {}); // No "await" - executes in background.
}

[..] If the notification service does not use the task manager, consumers (Cases) will have to use the task manager leading to multiple implementations of the same task. Lastly, If in the future the notification service decides to use the task manager, Cases need to stop using the task manager to avoid creating two jobs: one for Cases and one for the notification service. [..]

Curious, why should any of them use the Task Manager? Ideally, the current abstraction should "just send an email" and there should be no assumptions from Cases of how it is implemented under-the-hood.

[..] In the long run, it will be better if the notification service uses the task manager and have control of how notifications are being sent.

In the long run, it is not even clear if the Notifications Service/Center will be part of Kibana, maybe it will be a standalone cloud service. Hence, I'm curious, why do you think it should use the Task Manager?

Agreed. The other benefit we get from this approach is that we will transparently retry sending these emails when there's a transient error once we start supporting at-least-once action runs.

Currently, as far as I understand, we are not re-sending. And we should not optimize for that, as Notifications Center might not even be implemented in Kibana, but as a standalone service. And Email Service might not use Connectors in the future.

@kobelb
Copy link
Contributor

kobelb commented Oct 17, 2022

@cnasikas @kobelb @gsoldevila I was hoping we can execute an action immediately, using the execute() method. (I assume that execute() does not create a saved object.) So we can avoid creating an unnecessary saved object and adding it to the Task Manager queue.

Or am I missing something? Do we gain something by scheduling a task in the Task Manager? Or does it have to do with UI Kibana and Background Task Kibana split, where UI Kibana does not know how to execute an Action?

Currently, we get a few concrete benefits from scheduling a task in task-manager:

  1. It's asynchronously ran - this allow us to make the UI more responsive because it doesn't need to wait for the email to complete sending.
  2. Transparent retries - if there's a transient error sending the email, it will transparently retry sending the email. This will all be done asynchronously, and the user won't want to wait.

It's also aligned with our future long-term vision and allows us to make the email sending be performed by the Kibana process that is only responsible for running background-tasks.

@kobelb
Copy link
Contributor

kobelb commented Oct 17, 2022

@cnasikas What do you mean here exactly? The execution should still be async, hence can run in the background:

async sendNotifications() {
plugins.notifications.email.sendPlainTextEmail(opts).catch(() => {}); // No "await" - executes in background.
}

This will result in emails not being sent if the Kibana node crashes at the wrong time. We should NOT do this.

@vadimkibana
Copy link
Contributor

vadimkibana commented Oct 17, 2022

@kobelb

It's asynchronously ran - this allow us to make the UI more responsive because it doesn't need to wait for the email to complete sending.

It will need to wait for saved object to be stored. My example above shows how to make it truly async.

In any case, it is up the Cases team how much async they want it. Whether they want to block the user until the saved object is stored or not.

For the notifications plugin, the important part to understand is if we want to create the saved objects at all.

Transparent retries - if there's a transient error sending the email, it will transparently retry sending the email.

What kind of errors will it try to overcome? I assume those would be connector specific errors, like if it cannot execute an action at all for some reason? Is there a way to specify on which kind of errors it will re-try, so we don't end up sending multiple emails for the same notification?

I assume it will not handle email specific semantics, like debounced emails?

It's also aligned with our future long-term vision and allows us to make the email sending be performed by the Kibana process that is only responsible for running background-tasks.

This a good point. Essentially what I was asking above:

Or does it have to do with UI Kibana and Background Task Kibana split, where UI Kibana does not know how to execute an Action?

I guess the immediate execute() method will be deprecated on UI Kibanas in the fututre?

Which means, we have to create saved objects, otherwise it will be refactored to that in the future anyways.

@kobelb
Copy link
Contributor

kobelb commented Oct 17, 2022

It will need to wait for saved object to be stored. My example above shows how to make it truly async.

I feel like I'm missing something. Are we really so concerned about the additional time that it will take to persist a single saved object that we're willing to risk these emails not being sent?

This approach also means that any errors that are thrown when sending the email will be swallowed. The user will never know if their email didn't send.

In any case, it is up the Cases team how much async they want it. Whether they want to block the user until the saved object is stored or not.

For the notifications plugin, the important part to understand is if we want to create the saved objects at all.

In all other situations where we send emails, we've decided to persist a saved-object to increase the likelihood that the emails are sent. Is there something special about this situation that makes you think we should change our approach?

What kind of errors will it try to overcome? I assume those would be connector specific errors, like if it cannot execute an action at all for some reason? Is there a way to specify on which kind of errors it will re-try, so we don't end up sending multiple emails for the same notification?
I assume it will not handle email specific semantics, like debounced emails?

At the moment, the email connector is rather naive about retries, and it will retry on any error that is thrown when sending the email. We can definitely improve on this in the future. It is possible that we will end up sending multiple emails for the same notification.

I guess the immediate execute() method will be deprecated on UI Kibanas in the fututre?

We aren't certain about the future of execute(). There are some valid usages at the moment, for example we allow people to manually synchronize a case to a third-party service and we want to display the result in the UI. At a minimum, we do know that in the future we will be minimizing the usage to a bare minimum.

@vadimkibana
Copy link
Contributor

@kobelb

I feel like I'm missing something. Are we really so concerned about the additional time that it will take to persist a single saved object that we're willing to risk these emails not being sent?

This approach also means that any errors that are thrown when sending the email will be swallowed. The user will never know if their email didn't send.

I don't have an opinion here, if Cases team wants to block while waiting on saved object save, fine with me. (I guess people mean different things by async.)

From what I understand, we need a saved object in any case, as that is the mechanism how Task Manager will communicate from UI Kibana to Background Task Kibana.

Regarding email retries:

  1. Is it possible to configure it such that it retries to resolve only transient errors, which will not lead to more than one email for the same notification?
  2. Will it handle email protocol errors in the future (maybe already now), like debounced emails?

In all other situations where we send emails ...

Curious, what are the other places in Kibana where we send emails?

@kobelb
Copy link
Contributor

kobelb commented Oct 18, 2022

Is it possible to configure it such that it retries to resolve only transient errors, which will not lead to more than one email for the same notification?

Anything is possible with enough time and developer effort.

Will it handle email protocol errors in the future (maybe already now), like debounced emails?

It definitely can.

Curious, what are the other places in Kibana where we send emails?

Alerting rules send emails when various conditions are hit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

8 participants