Adds index action as built-in action #41592

pmuellr · 2019-07-19T17:21:10Z

Intended to be functionally equivalent to the watcher index action.

pmuellr · 2019-07-19T17:37:13Z

Currently there is no config for this action, and the params look like this:

const ParamsSchema = schema.object({
  index: schema.string(),
  doc_id: schema.maybe(schema.string()),
  execution_time_field: schema.maybe(schema.string()),
  refresh: schema.maybe(schema.boolean()),
  body: IndexBodySchema,
});

body can be a single object or an array of objects. I think the ultimate "result" from the action would be an array of results for each doc indexed; so if you pass a single doc, you'd get an array of length one as part of the result.

It seems like having index be a config parm would be nice. That way you could create an action and either have it default or force the index value, preventing various sorts of errors when executing the action where the index isn't what you're expecting (eg, typo). OTOH, perhaps this would be a pain-in-the-rear for folks creating actions programmatically, writing to multiple indices, as they would need to create an action per index. And ... guessing this particular action is going to be more of a programmatic use case, compared to user-facing actions like slack and email.

So, thinking we could make index an optional config property; if set to a value, that value would be used by all executions of the action. If not set, the index would have to be set in the params. If both are used, the params value would be ignored - perhaps logging this would be appropriate?

This would be the first case of having an execution property being set by config | params, which is something we will likely see again.

spalger · 2019-07-19T17:44:51Z

retest

elasticmachine · 2019-07-19T18:43:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-19T23:59:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-20T01:51:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-20T06:04:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-22T20:48:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-22T20:55:47Z

Pinging @elastic/kibana-stack-services

elasticmachine · 2019-07-22T21:51:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pmuellr · 2019-07-23T03:04:23Z

fixes #40027

mikecote

Just one small comment but everything else looks good.

x-pack/legacy/plugins/actions/README.md

bmcconaghy

Looking good, just a couple of things need to get addressed (localization most importantly).

x-pack/legacy/plugins/actions/README.md

x-pack/legacy/plugins/actions/server/builtin_action_types/es_index.ts

bmcconaghy · 2019-07-23T14:14:20Z

x-pack/legacy/plugins/actions/server/builtin_action_types/es_index.ts

+type IndexArrayBodySchemaType = TypeOf<typeof IndexArrayBodySchema>;
+
+// see: https://www.elastic.co/guide/en/elastic-stack-overview/current/actions-index.html
+// - timeout not added here, as this seems to be a generic thing we want to do


wondering if we do want to add timeout here, as you might want to have that value be different from whatever would be set globally.

I have a card open for this - https://github.com/elastic/kibana/projects/26#card-24087404

Yeah, I think we will probably want a global timeout, and then allow an action type to override. Then the next question is, how can a user change these? env var or kibana yaml config or ...

We could get more elaborate, allow timeout per action, but I don't think we need to go there just yet ...

bmcconaghy · 2019-07-23T14:18:43Z

x-pack/legacy/plugins/actions/server/builtin_action_types/es_index.ts

+  doc_id: schema.maybe(schema.string()),
+  execution_time_field: schema.maybe(schema.string()),
+  refresh: schema.maybe(schema.boolean()),
+  body: IndexBodySchema,


looks like we are also missing refresh and transform from the options that watcher index action supports. Thinking that body isn't the best name for this param, maybe document instead? Also thinking that we should make this always an array, to make it more obvious that you can pass multiples here.

documents sounds good, as does always making it an array; that means doc_id would go away, as that was only applicable to the single document case.

refresh is already there. transform isn't, because I figured transform was a general watcher thing, perhaps not appropriate for Kibana. Guessing that if we want it here, we want it as a general capability for all actions. And to some extent, this may intersect with the thought of using templates to generate action parameters. And maybe what we want in the end is to use "expressions" to handle all this jazz. Expressions being the kind of "painless" of Kibana (in terms of a general transformer thing, and it's ubiquity) ...

@bmcconaghy so ... whatcha thinking on transform? Even though I think it's possible it could be a general thing for actions, perhaps it's really most appropriate for the index action, since that one deal in fairly arbitrary JSON-able objects, vs the other actions which have less arbitrary params and are more "notification"-y vs "update the db"-y.

Perhaps some research is required to see how people are using it today with watcher, come up with a use case for using it today?

Yeah don't really know how it is used today. I suppose that the alert that was going to trigger this action could do its own transformation, so maybe it's not necessary. Just was looking to see if we were at watcher parity and noticed this.

bmcconaghy · 2019-07-23T14:21:44Z

x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts

@@ -49,7 +50,7 @@ async function executor(execOptions: ActionTypeExecutorOptions): Promise<ActionT
  } catch (err) {
    return {
      status: 'error',
-      message: `error logging message: ${err.message}`,
+      message: `error in action ${id} logging message: ${err.message}`,


This and all strings returned from API calls should be localized.

Probably localization should be a separate PR - created a card for that - https://github.com/elastic/kibana/projects/26#card-24316443

- moved `nullableType` to a module so it can be reused across action types - changed index action param `body` to `documents`, and it's no longer an object or array, it's only an array - rename index action param `execution_time_field` to `executionTimeField` - remove index action param `doc_id` as it's no longer relevant (only useful in the single document story previously)

mikecote

LGTM for my comments 👍

bmcconaghy

Code LGTM.

mikecote · 2019-07-24T12:19:29Z

retest

elasticmachine · 2019-07-24T13:17:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

pmuellr · 2019-07-24T14:27:13Z

The X-Pack firefox smoke test failure is fixed in master (from some other PR, not this one), so will merge master ...

elasticmachine · 2019-07-24T15:33:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

pmuellr · 2019-07-24T16:01:29Z

retest

elasticmachine · 2019-07-24T16:57:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Adds index action as built-in action * resolve PR comments - moved `nullableType` to a module so it can be reused across action types - changed index action param `body` to `documents`, and it's no longer an object or array, it's only an array - rename index action param `execution_time_field` to `executionTimeField` - remove index action param `doc_id` as it's no longer relevant (only useful in the single document story previously)

pmuellr force-pushed the actions/add-index-action branch from aca712d to 685bc6b Compare July 20, 2019 00:56

Adds index action as built-in action

c34650b

pmuellr force-pushed the actions/add-index-action branch from e47a543 to c34650b Compare July 22, 2019 20:54

pmuellr added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services v7.4.0 v8.0.0 labels Jul 22, 2019

pmuellr requested a review from mikecote July 22, 2019 20:55

pmuellr marked this pull request as ready for review July 23, 2019 03:01

pmuellr requested a review from a team July 23, 2019 03:01

mikecote reviewed Jul 23, 2019

View reviewed changes

x-pack/legacy/plugins/actions/README.md Show resolved Hide resolved

bmcconaghy suggested changes Jul 23, 2019

View reviewed changes

mikecote approved these changes Jul 23, 2019

View reviewed changes

bmcconaghy approved these changes Jul 23, 2019

View reviewed changes

Merge branch 'master' into actions/add-index-action

fb97577

pmuellr merged commit ab6aca2 into elastic:master Jul 24, 2019

pmuellr mentioned this pull request Jul 24, 2019

[7.x] Adds index action as built-in action (#41592) #41932

Merged

pmuellr added the backported label Jul 24, 2019

pmuellr deleted the actions/add-index-action branch July 30, 2019 18:38

njd5475 mentioned this pull request Sep 13, 2019

Alerting and action services #24214

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds index action as built-in action #41592

Adds index action as built-in action #41592

pmuellr commented Jul 19, 2019

pmuellr commented Jul 19, 2019

spalger commented Jul 19, 2019

elasticmachine commented Jul 19, 2019

elasticmachine commented Jul 19, 2019

elasticmachine commented Jul 20, 2019

elasticmachine commented Jul 20, 2019

elasticmachine commented Jul 22, 2019

elasticmachine commented Jul 22, 2019

elasticmachine commented Jul 22, 2019

pmuellr commented Jul 23, 2019

mikecote left a comment

bmcconaghy left a comment

bmcconaghy Jul 23, 2019

pmuellr Jul 23, 2019

bmcconaghy Jul 23, 2019

pmuellr Jul 23, 2019

pmuellr Jul 23, 2019

bmcconaghy Jul 23, 2019

bmcconaghy Jul 23, 2019

pmuellr Jul 23, 2019

mikecote left a comment

bmcconaghy left a comment

mikecote commented Jul 24, 2019

elasticmachine commented Jul 24, 2019

pmuellr commented Jul 24, 2019

elasticmachine commented Jul 24, 2019

pmuellr commented Jul 24, 2019

elasticmachine commented Jul 24, 2019

Adds index action as built-in action #41592

Adds index action as built-in action #41592

Conversation

pmuellr commented Jul 19, 2019

pmuellr commented Jul 19, 2019

spalger commented Jul 19, 2019

elasticmachine commented Jul 19, 2019

💔 Build Failed

elasticmachine commented Jul 19, 2019

💔 Build Failed

elasticmachine commented Jul 20, 2019

💔 Build Failed

elasticmachine commented Jul 20, 2019

💚 Build Succeeded

elasticmachine commented Jul 22, 2019

💚 Build Succeeded

elasticmachine commented Jul 22, 2019

elasticmachine commented Jul 22, 2019

💚 Build Succeeded

pmuellr commented Jul 23, 2019

mikecote left a comment

Choose a reason for hiding this comment

bmcconaghy 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

mikecote left a comment

Choose a reason for hiding this comment

bmcconaghy left a comment

Choose a reason for hiding this comment

mikecote commented Jul 24, 2019

elasticmachine commented Jul 24, 2019

💔 Build Failed

pmuellr commented Jul 24, 2019

elasticmachine commented Jul 24, 2019

💔 Build Failed

pmuellr commented Jul 24, 2019

elasticmachine commented Jul 24, 2019

💚 Build Succeeded