Add support for all event fields in the Pagerduty action plugin

[tobio]

Summary

Adds support for links, images, and customDetails to the built in Pagerduty action plugin. Fixes #63933.

Checklist

Delete any items that are not applicable to this PR.

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[mikecote]

LGTM, I'll also let @pmuellr have a look.

[mikecote]

We're working on this but we currently have duplication between this file and docs/user/alerting/action-types/pagerduty.asciidoc for now. Would you be able to copy these lines to the user docs as well?

I'll make sure we have an issue to cleanup the README.md file so it doesn't repeat what's in the docs.

[pmuellr]

code LGTM

Worrying about a couple of things tho:

• there's some interest in having better image support for actions that can support images - see issue Add image/chart support to actions #49908 . That issues goes beyond just specifying an image url, which is of course the simple case. But image url also seems like the least interesting case, as it assumes there's an image available somewhere on the web that will be visible to folks looking at the integration system (PagerDuty, in this case). I don't think we've heard a use case for this yet. Would the image url get filled in from the alert context? Or is it just a fixed image always included in the alert? Or is it a fixed url that displays a constantly changing image? I'm just curious on this one, but I think understanding the use case will ensure we structure this correctly.

• generally, wondering about the other two fields as well - links and customDetails - is there an expectation these would get filled in via the alert context? I'm not sure how this would actually work, with mustache templating and building a UI around it. Would you add a single link element in the UI, and the values you entered for href and text would have context-provided values? Or is there an expectation that the entire array would somehow be dynamic (the alert could end up with 0-n links being generated).

• no UI - we can do that in a separate issue/PR of course, but we'll also have to account time for it. Would we ship this new API without the UI for it, if we get in a crunch? Maybe someone just needs this programmatically, UI isn't really required in the alerting UIs? Mentioning this especially since the UI for this seems non-trivial - lists of objects, do we provide image previews, etc.

[tobio]

@pmuellr

Images

To be honest, I don't have a use case. I've included support for that field in the PR since it seemed strange to support every other API field except images. What I'd love to see from a Kibana perspective would be linking a visualisation into an action and getting a snapshot of that viz in the notification. But I don't expect Cloud will be using the images field unless there was something built into Kibana.

Links

For the most part Cloud will have a fixed list of URL's (some of which would be templated) associated with an action. Mostly this will be linking an alert into a runbook on our wiki, or linking to alerting components in the Cloud adminconsole.

Longer term, there are existing watches which build a list of links based on hits/aggs. We'll be evaluating these as we transition to Alerting. Some will make sense to keep as aggregate alerts with a full list. tl;dr we have some expectation for a dynamic list of links here.

As it stands, this PR isn't providing support for a templated list of links. From my perspective having configuration similar to Watchers dynamic_attachements is a simpler concept than generating a list with a mustache iterator. I wanted to check in with you all before approaching the dynamic list since it felt like there were some wider decisions to be made there. This is a longer term requirement though, and something we can load in as a plugin so I'd rather get it right.

UI

I think my opinion here is of limited use :) If it only makes sense to ship this change with the UI components in place I can look at working on it in my own time but I have no idea of when I will actually have any of that that. Alerting configuration via the UI is of limited use to Cloud at the moment. Any alerts/actions we have defined will be stored in git somewhere and so Alerting will be API driven. Whilst I expect we'll use the UI for defining new alerts or testing some ideas, anything used in a production context will be moved to git and kept in sync via the API.

[pmuellr]

I think I know why I didn't add these initially. They're hard! :-)

Images

But I don't expect Cloud will be using the images field ...

I feel like we should defer adding images then. I don't think I can come up with a use case where a fixed image would be useful (as a literal parameter), or a "dynamic" one that is partially filled in from data from an alert. (note it also has the static/dynamic issues same as links/custom details, as described below).

Links

Sounds like there are some "fixed" links you'd like to add - perhaps pointing to a particular rundeck page for a specific alert. And someday you'll have a dynamic list of links you'd like provided in the context. Mustache won't work for that dynamic list of links, since it only generates text, and we need an array of objects. So we would need some new mechanism like watcher's dynamic_attachments to handle that. I think we'll need something like this for ANY array-based action parameters that aren't "literals" (eg, webhook has headers, but it's not expected you would be setting them dynamically - the UI allows you to add explicit values).

I've opened an issue for this: #64247

Custom Details

I suspect - at least long term - this is similar to links in that there may be some "fixed" key/vals that could be explicitly set when creating the alert, and there are cases where you'd like some filled in dynamically from the alert.

[pmuellr]

I should mention I don't have an issue with adding these properties if we think they would be useful in a non-alerting context - someone invoking actions via the http endpoint or plugin's start execute() function. But guessing that's not the case. And I guess we'd be in a weird case where some action properties wouldn't be available to alerts, or only in a limited (static) form.

[pmuellr]

We'll want some tests for this - both jest tests (next to the actual code) as well as functional tests, probably in here: https://github.com/elastic/kibana/blob/master/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/pagerduty.ts

Might want to wait before diving into this, till we figure out what else we want to do here.

[pmuellr]

This will need to be a schema.nullable() vs schema.maybe(), to prevent "partial updates" of this field when it's updated in ES. Without it, it's impossible to delete fields in the record.

[pmuellr]

same is true for links and images as well, ^^^

[mikecote]

@tobio this PR has been opened for over a year and shows up in our new PR review reminder. Would it be ok to close this PR for now?

[kibanamachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 2c59108
• Pipeline Steps (look for red circles / failed steps)
• Interpreting CI Failures

Failed CI Steps

• Setup Build Environment and Dependencies
• Setup Build Environment and Dependencies
• Setup Build Environment and Dependencies
• Setup Build Environment and Dependencies

History

• 💚 Build #42284 succeeded 2c59108

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream