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

[Alerting][rfc][skip-ci] Actions service #31835

Closed
wants to merge 18 commits into from

Conversation

njd5475
Copy link
Contributor

@njd5475 njd5475 commented Feb 22, 2019

@njd5475 njd5475 added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Alerting RFC labels Feb 22, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@elasticmachine
Copy link
Contributor

💔 Build Failed

@njd5475 njd5475 changed the title [Alerting][rfc] Actions service [Alerting][rfc][skip-ci] Actions service Feb 25, 2019
@njd5475 njd5475 mentioned this pull request Mar 7, 2019
19 tasks
@njd5475 njd5475 marked this pull request as ready for review March 8, 2019 22:46
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantics

I'm finding some of the semantics proposed here both a bit confusing and more coupled to alerts than it needs to be. Here's how I'm thinking about this:

  • We need to have classes of actions, actionType makes sense. 👍
  • Each actionType needs to be able to have different handlers that can be registered by different plugins. For this concept, I think the term "connector" is too specific to alerts. For instance, an actionType could be "generate report" or "visualize query". In those cases, I don't think connector makes sense. Instead, I believe "actionTypeHandler" or just "actionHandler" is more accurate.
  • Each handler needs to be able to have saved configurations. I think "instance" isn't bad, but "actionHandlerConfig" would be more clear to differentiate between a specific instance of an action firing and a saved action handler configuration.

```JS
server.actions.registerActionType({
name: 'notification',
fire: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect this to be named params similar to how it's used on the fire API call. Also could we leverage TypeScript here to define types for each of these fields? We should be able to use keyof and a few tricks to get type information for each param field.

Something like this might work:

type Param = string | number | boolean;
type Params = { [name: string]: Param };

interface ActionTypeParam<T extends Param> {
  optional?: boolean;
}

type ActionTypeParams<P extends Params> = {
  [K in keyof P]: ActionTypeParam<P[K]>
}

That may allow you to enforce the types when actions.fire is called by using overloads of the fire function (each plugin could extend this interface to add the required param types for any actionTypes they register.)

// Base type info in the actions service code itself
interface ActionFire {
  (name: string, params: ActionTypeParams<any>): Promise<MyReturnType>;
}

// Interface extended / merged by plugins that add new actionTypes
// For example, the alerting plugin could do this:
interface ActionFire {
  (name: 'send to slack', params: SendToSlackParams): Promise<MyReturnType>;
  (name: 'send email', params: SendEmailParams): Promise<MyReturnType>;
}

This may not be exactly what it looks like, but I believe this should be possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree this should be named params, and was definitely thinking that we make the typescript interface enforce the param types in the fire function.

```JS
server.actions.registerConnector({
actionType: 'notification',
connectorType: 'slack',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should connectors/handlers define param types as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, the connectorParam channel's type should be defined here! And the instance method should enforce proper creation of the param structure and types.

This might be different when using typescript though as you could probably just specify the connectorParams type and infer the type from that.

connectorParams: {
  { name: 'channel', type: 'string' }
},

the code complexity of each alert and time to market for new integrations. And
increased risk of alerts for specific applications not having options to perform
the same actions that other applications support. Also differences in UI and
customizability for the action connectors may vary per application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as new action types are added to meet users needs are discovered alerts will require changes to support the new types.

Can you elaborate on this a bit? I assumed an alert could be configured by a user to use specific action configurations (like send notification to slack). If we add a new action handler for the "notification" actionType, I don't see why we'd need to change the alerting code to be able to use it. Of course, the user would still to configure them, but I assumed that by adding a new action handler, it would show up in a list of options for the user to configure.

Am I understanding this incorrectly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let me try to elaborate and may be it isn't as big of a concern.

One of the main methods this service will be consumed is by the alert tasks which will specifically invoke a method to use an action type something like the below.

task({ actions }) {
  elasticsearch.search(someQuery...)
  ...
  actions.sendNotification({
      destination: "slack/url",
      message: params.message,
      title: params.title,
  })
}

The alert could also be coded to support sending one notification for each hit from elasticsearch or each that met some condition. What is possible with a given alert is up the alert itself.

So given that the alert implementation must know the action type parameters. If we add other action types in the future all alert code would need to be updated to support those action types, we wouldn't automatically be able to use the new types everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I got it. So if new handlers are added, the alerting code doesn't need to be updated. A user would be able to configure an instance of the action type that uses the new handler. This works because the actionType has consistent params for all handlers of that type.

When new actionTypes are added, alerting code would need to be updated because that type may accept completely different params. Makes sense to me now.

@joshdover
Copy link
Contributor

The only feedback I have now is the "instance" name. I still think this may be confusing. Here's a concrete example:

// This creates an "instance". I presume it would return an instance object that could be called
// directly with `fire` as well.
actions.instance({
  name: 'send message to slack',
  actionType: 'notification',
  handler: 'slack',
  params: {
    destination: '<slack url>',
  },
  handlerParams: {
    channel: '#bot-playground',
  },
});

// This fires an instance. What is the return value of this function called? An instance instance?
actions.fire({
  // I'd expect the name of this parameter to match whatever a configured action is called.
  action: 'send message to slack',
  // This seems unnecessary, isn't this defined by the instance config?
  actionType: 'notification',
  params: {
    message: 'This is a test message from kibana actions service',
    title: 'custom title'
  }
})

I think calling a configured handler / actionType combo actionConfig is more internally consistent and less ambiguous. Example:

const sendToSlack = actions.createConfig({
  id: 'send message to slack',
  actionType: 'notification',
  handler: 'slack',
  params: {
    destination: '<slack url>',
  },
  handlerParams: {
    channel: '#bot-playground',
  },
});

// Can call on the service with the matching `id`.
// `instance` in this case would contain the results of processing the action
const instance = await actions.fire({
  actionConfigId: 'send message to slack',
  params: {
    message: 'This is a test message from kibana actions service',
    title: 'custom title'
  }
});

// Could also call directly on the `sendToSlack` returned from `createConfig`.
// `instance` is the same as above, the results of processing the action.
const instance = sendToSlack.fire({
  params: {
    message: 'This is a test message from kibana actions service',
    title: 'custom title'
  }
});

@mikecote mikecote closed this May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting RFC Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants