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.

Sign up for GitHub

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

Jump to bottom

[Alerting] add more alert properties to action parameter templating #59718

Merged
merged 3 commits into from
Mar 10, 2020
Merged

[Alerting] add more alert properties to action parameter templating #59718

merged 3 commits into from
Mar 10, 2020

Conversation

pmuellr
Copy link
Member

@pmuellr pmuellr commented Mar 10, 2020 •
edited
Loading

This is a pre-cursor to #58529

I realized a bit ago that we weren't making quite enough info available
in the action parameter templating that happens when alerts schedule
actions to execute. Missing were alert name, tags, and spaceId.

For the index threshold alert, I had added them to it's context, but
then every other action would have to do the same if they also
wanted those values.

So I added these as additional top-level variables that can be
used in templates, along with the alert id, alert instance id,
context, and state. The other bits in RawAlert didn't seem
that interesting, to be used as an action parameter.

@pmuellr
[Alerting] add more alert properties to action parameter templating
1ca63cc 
This is a pre-cursor to elastic#58529

I realized a bit ago that we weren't making quite enough info available
in the action parameter templating that happens when alerts schedule
actions to execute.  Missing were alert name, tags, and spaceId.

For the index threshold alert, I had added them to it's context, but
then every other action would have to do the same if they also
wanted those values.

So I added these as additional top-level variables that can be
used in templates, along with the alert id, alert instance id,
context, and state.  The other bits in RawAlert didn't seem
that interesting, to be used as an action parameter.
@pmuellr pmuellr added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Mar 10, 2020
@elasticmachine
Copy link
Contributor

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

@pmuellr pmuellr marked this pull request as ready for review March 10, 2020 13:57
@pmuellr pmuellr requested a review from a team as a code owner March 10, 2020 13:57
@mikecote mikecote self-requested a review March 10, 2020 14:53
mikecote
mikecote approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM, I wonder if we need to reach out to solution teams about potential breaking changes? (since I believe this removes some fields from context)

YulNaumenko
YulNaumenko approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM.

@pmuellr
Copy link
Member Author

pmuellr commented Mar 10, 2020

I wonder if we need to reach out to solution teams about potential breaking changes? (since I believe this removes some fields from context)

I think it's just a breaking change for the index threshold alertType - removal of some of the context variables, which are now available as top-level variables instead of under context. At this early stage of it's usage, seems like a breaking change like this is ok.

We could add them back to the indexThreshold alert type, in their description (in PR #59756 ) we could note them as "deprecated" or something.

Or maybe I missed something, in terms of a breaking change. My intention was to only have a breaking change to the indexThreshold alertType's context variables.

@mikecote
Copy link
Contributor

I think it's just a breaking change for the index threshold alertType - removal of some of the context variables, which are now available as top-level variables instead of under context. At this early stage of it's usage, seems like a breaking change like this is ok.

Doh! You are right, this is index threshold specific! I somehow thought x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/action_context.ts was not index threshold related (didn't read the path 🙈 ).

All good!

@pmuellr
Copy link
Member Author

pmuellr commented Mar 10, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@pmuellr pmuellr merged commit 7db4196 into elastic:master Mar 10, 2020
pmuellr added a commit to pmuellr/kibana that referenced this pull request Mar 10, 2020
@pmuellr
[Alerting] add more alert properties to action parameter templating (e…
cfa124e 
…lastic#59718)

This is a pre-cursor to elastic#58529

I realized a bit ago that we weren't making quite enough info available
in the action parameter templating that happens when alerts schedule
actions to execute.  Missing were alert name, tags, and spaceId.

For the index threshold alert, I had added them to it's context, but
then every other action would have to do the same if they also
wanted those values.

So I added these as additional top-level variables that can be
used in templates, along with the alert id, alert instance id,
context, and state.  The other bits in RawAlert didn't seem
that interesting, to be used as an action parameter.
@pmuellr pmuellr mentioned this pull request Mar 10, 2020
Merged
pmuellr added a commit that referenced this pull request Mar 10, 2020
@pmuellr
[Alerting] add more alert properties to action parameter templating (#…
f4fd128 
…59718) (#59829)

This is a pre-cursor to #58529

I realized a bit ago that we weren't making quite enough info available
in the action parameter templating that happens when alerts schedule
actions to execute.  Missing were alert name, tags, and spaceId.

For the index threshold alert, I had added them to it's context, but
then every other action would have to do the same if they also
wanted those values.

So I added these as additional top-level variables that can be
used in templates, along with the alert id, alert instance id,
context, and state.  The other bits in RawAlert didn't seem
that interesting, to be used as an action parameter.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 11, 2020
@gmmorris
Merge branch 'master' into alerting/view-in-app
ff30bf8 
* master:
  [ML] Transforms: Use EuiInMemoryTable instead of custom typed table. (elastic#59782)
  Alerting/fix flaky instance test (elastic#58994)
  ci: disable all Mocha rules for tape tests (elastic#59798)
  Fix UX in alerting UI forms when errors occur (elastic#59444)
  [DOCS] Updated and added jump tables (elastic#59774)
  [DOCS] Moved rolled up index content (elastic#59372)
  Regenerate core api docs (elastic#59814)
  [Lens] remove react warnings (elastic#59574)
  The scripts/backport.js file isn't an executable (elastic#59800)
  [Alerting] add more alert properties to action parameter templating (elastic#59718)
  [Design] Branding changes in Elastic to focus more towards the Elastic brand (elastic#58160)
  [SIEM] Adds 'Create new rule' Cypress test (elastic#59790)
  Updating svgo -> css-tree -> mdn-data, all so we get mdn-data > 2.0 (elastic#58913)
  Use EUI test environment build with Jest (elastic#55877)
  update typescript version in all packages to avoid warnings (elastic#59787)
  [SIEM] [Case] Insert timeline into case textarea (elastic#59586)
  [ML] Functional tests - stabilize saved search tests (elastic#59652)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@mikecote mikecote mikecote approved these changes

Assignees
No one assigned
Labels
backported Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pmuellr @elasticmachine @mikecote @kibanamachine @YulNaumenko