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] Enables AlertTypes to define the custom recovery action groups #84408

Merged
merged 29 commits into from
Dec 4, 2020

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Nov 26, 2020

Summary

Closes #83981

In this PR we introduce a new recoveryActionGroup field on AlertTypes which allows an implementor to specify a custom action group which the framework will use when an alert instance goes from active to inactive.
By default all alert types will use the existing RecoveryActionGroup, but when recoveryActionGroup is specified, this group is used instead.

This is applied across the UI, event log and underlying object model, rather than just being a label change.
To support this we also introduced the alertActionGroupName message variable which is the human readable version of existing alertActionGroup variable.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

* master: (70 commits)
  [Uptime] Fix headers io-ts type (elastic#84089)
  [fleet] Add config options to accepted docker env vars (elastic#84338)
  [Fleet] Support URL query state in agent logs UI (elastic#84298)
  [basePathProxy] include query in redirect (elastic#84356)
  [Security Solution] Add Endpoint policy feature checks (elastic#83972)
  Fix issues with show_license_expiration (elastic#84361)
  [Security Solution][Resolver] Add support for predefined schemas for endpoint and winlogbeat (elastic#84103)
  [cli/dev] log a warning when --no-base-path is used with --dev (elastic#84354)
  [Fleet] Support input-level vars & templates (elastic#83878)
  [APM] Elastic chart issues (elastic#84238)
  [Time to Visualize] Fix Unlink Action via Rollback of ReplacePanel (elastic#83873)
  redirect to visualize listing page when by value visualization editor doesn't have a value input (elastic#84287)
  add live region for field search (elastic#84310)
  [ML] Persisted URL state for Anomalies table (elastic#84314)
  [dev/cli] detect worker type using env, not cluster module (elastic#83977)
  [Workplace Search] Migrate DisplaySettings tree (elastic#84283)
  Deprecate `xpack.task_manager.index` setting (elastic#84155)
  [Search] Search batching using bfetch (again) (elastic#84043)
  Use .kibana instead of .kibana_current to mark migration completion (elastic#83373)
  [Monitoring] Only look at ES for the missing data alert for now (elastic#83839)
  ...
* master: (119 commits)
  [Uptime] Fix headers io-ts type (elastic#84089)
  [fleet] Add config options to accepted docker env vars (elastic#84338)
  [Fleet] Support URL query state in agent logs UI (elastic#84298)
  [basePathProxy] include query in redirect (elastic#84356)
  [Security Solution] Add Endpoint policy feature checks (elastic#83972)
  Fix issues with show_license_expiration (elastic#84361)
  [Security Solution][Resolver] Add support for predefined schemas for endpoint and winlogbeat (elastic#84103)
  [cli/dev] log a warning when --no-base-path is used with --dev (elastic#84354)
  [Fleet] Support input-level vars & templates (elastic#83878)
  [APM] Elastic chart issues (elastic#84238)
  [Time to Visualize] Fix Unlink Action via Rollback of ReplacePanel (elastic#83873)
  redirect to visualize listing page when by value visualization editor doesn't have a value input (elastic#84287)
  add live region for field search (elastic#84310)
  [ML] Persisted URL state for Anomalies table (elastic#84314)
  [dev/cli] detect worker type using env, not cluster module (elastic#83977)
  [Workplace Search] Migrate DisplaySettings tree (elastic#84283)
  Deprecate `xpack.task_manager.index` setting (elastic#84155)
  [Search] Search batching using bfetch (again) (elastic#84043)
  Use .kibana instead of .kibana_current to mark migration completion (elastic#83373)
  [Monitoring] Only look at ES for the missing data alert for now (elastic#83839)
  ...
…' into alerts/custom-recovery-action-group

* origin/alerts/fix-buggy-default-message:
  fixed typing
  added missing test
  fixed buggy default message behaviour
* master:
  [Security Solution] Exceptions Cypress tests (elastic#81759)
  [ML] Fix spaces job ID check (elastic#84404)
  [Security Solution][Detections] Handle dupes when processing threshold rules (elastic#83062)
  skip flaky suite (elastic#84440)
  skip flaky suite (elastic#84445)
  [APM] Fix missing `service.node.name` (elastic#84269)
  Upgrade fp-ts to 2.8.6 (elastic#83866)
  Added data streams privileges to better control delete actions in UI (elastic#83573)
  Improve short-url redirect validation (elastic#84366)
  TSVB offsets (elastic#83051)
  [Discover] Fix navigating back when changing index pattern (elastic#84061)
  [Logs UI] Polish the UI for the log entry examples in the anomaly table (elastic#82139)
  [Logs UI] Limit the height of the "view in context" container (elastic#83178)
  [Application Usage] Update `schema` with new `fleet` rename (elastic#84327)
  fix identation in list (elastic#84301)
@gmmorris
Copy link
Contributor Author

gmmorris commented Dec 1, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

* master: (63 commits)
  Revert the Revert of "[Alerting] renames Resolved action group to Recovered (elastic#84123)"  (elastic#84662)
  declare kbn/monaco dependency on kbn/i18n explicitly (elastic#84660)
  Remove unscripted fields from sample data index-pattern saved objects (elastic#84659)
  [ML] Fix unnecessary trigger of wildcard field type search for ML plugin routes. (elastic#84605)
  Update create.asciidoc (elastic#84046)
  [Security Solution][Detections] Fix labels and issue with mandatory fields (elastic#84525)
  Fix flaky test suite (elastic#84602)
  [Security Solution] [Detections] Create a 'partial failure' status for rules (elastic#84293)
  Revert "[Alerting] renames Resolved action group to Recovered (elastic#84123)"
  Update code-comments describing babel plugins (elastic#84622)
  [Security Solution] [Cases] Cypress for case connector selector options (elastic#80745)
  [Discover] Unskip doc table tests (elastic#84564)
  [Lens] (Accessibility) Improve landmarks in Lens (elastic#84511)
  [Lens] (Accessibility) Focus mistakenly stops on righthand form (elastic#84519)
  Return early when parallel install process detected (elastic#84190)
  [Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides (elastic#83723)
  [Security Solution][Detections] Fix grammatical error in validation message for threshold field in "Create new rule" -> "Define rule" (elastic#84490)
  [Fleet] Update agent details page  (elastic#84434)
  adding documentation of use of NODE_EXTRA_CA_CERTS env var (elastic#84578)
  [Search] Integrate "Send to background" UI with session service (elastic#83073)
  ...
* master:
  [Lens] Show color in flyout instead of auto (elastic#84532)
  [Lens] Use index pattern through service instead of reading saved object (elastic#84432)
  Make it possible to use Kibana anonymous authentication provider with ES anonymous access. (elastic#84074)
  TelemetryCollectionManager: Use X-Pack strategy as an OSS overwrite (elastic#84477)
  migrate away from rest_total_hits_as_int (elastic#84508)
  [Input Control] Custom renderer (elastic#84423)
  Attempt to more granularly separate App Search vs Workplace Search vs shared GitHub notifications (elastic#84713)
  [Security Solutino][Case] Case connector alert UI (elastic#82405)
  [Maps] Support runtime fields in tooltips (elastic#84377)
  [CCR] Fix row actions in follower index and auto-follow pattern tables (elastic#84433)
  [Enterprise Search] Migrate shared Indexing Status component (elastic#84571)
  [maps] remove fields from index-pattern test artifacts (elastic#84379)
  Add routes for use in Sources Schema (elastic#84579)
  Changes UI links for drilldowns (elastic#83971)
  endpoint telemetry cloned endpoint tests (elastic#81498)
  [Fleet] Handler api key creation errors when Fleet Admin is invalid (elastic#84576)
@gmmorris gmmorris marked this pull request as ready for review December 2, 2020 10:42
@gmmorris gmmorris requested review from a team as code owners December 2, 2020 10:42
@gmmorris gmmorris added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 labels Dec 2, 2020
@elasticmachine
Copy link
Contributor

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

@gmmorris gmmorris changed the title Alerts/custom recovery action group [Alerting] Enables AlertTypes to define the custom recovery action groups Dec 2, 2020
@@ -32,7 +32,7 @@ export const AddMessageVariables: React.FunctionComponent<Props> = ({
messageVariables?.map((variable: ActionVariable, i: number) => (
<EuiContextMenuItem
key={variable.name}
data-test-subj={`variableMenuButton-${i}`}
data-test-subj={`variableMenuButton-${variable.name}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the underlying unit tests more reliable as i changes when you introduce new variables.

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected. Left a minor naming nit that I could go either way on.

I'm curious how many built in action groups we expect to have? And if we will want to provide overrides for each of them? I could see the alert type definition getting a little unwieldy if we have a lot and are providing top level fields to override each of them, but I'm assuming we don't plan on having many.

Finally, we be providing a way to override the default recovered action message? Is that another issue?

@@ -6,13 +6,13 @@
import { i18n } from '@kbn/i18n';
import { ActionGroup } from './alert_type';

export const RecoveredActionGroup: ActionGroup = {
export const RecoveredActionGroup: Readonly<ActionGroup> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

naming nit: It seems like we use recovered everywhere except here where it is recovery. What if we renamed the built-in action group DefaultRecoveredActionGroup and then named the field on the alert recoveredActionGroup

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize I put my comment on the wrong line where it doesn't actually say recovery 🤦‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha interesting, I just thought of recovery as the verb for becoming Recovered ...which is why the default AG for is Recovered. The term recoveredActionGroup could work.... but the past tense feels wrong to me 🤔

I don't know... Having two first languages makes these weird i my head at times.
Any thoughts from someone else in @elastic/kibana-alerting-services ?

x-pack/plugins/alerts/server/alert_type_registry.ts Outdated Show resolved Hide resolved
@gmmorris
Copy link
Contributor Author

gmmorris commented Dec 2, 2020

I'm curious how many built in action groups we expect to have? And if we will want to provide overrides for each of them? I could see the alert type definition getting a little unwieldy if we have a lot and are providing top level fields to override each of them, but I'm assuming we don't plan on having many.

We don't know... I agree, it could become unwieldy, but at the moment I'm only aware that we're considering NoData... that's about it. 🤔

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

SIEM/Endpoint LGTM

…overy-action-group

* upstream/master: (48 commits)
  [Lens] accessibility screen reader issues (elastic#84395)
  [Logs UI] Fetch single log entries via a search strategy (elastic#81710)
  fix: 🐛 don't add separator befor group on no main items (elastic#83166)
  [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323)
  [APM] Add APM agent config options (elastic#84678)
  Fixed a11y issue on rollup jobs table selection (elastic#84567)
  [Discover] Refactor getContextUrl to separate file (elastic#84503)
  [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654)
  [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749)
  [Lens] Migrate legacy es client and remove total hits as int (elastic#84340)
  Improve logging pipeline in @kbn/legacy-logging (elastic#84629)
  Catch @hapi/podium errors (elastic#84575)
  [Discover] Unskip date histogram test (elastic#84727)
  Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791)
  [Enterprise Search] Fix schema errors button (elastic#84842)
  [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589)
  [Maps] Always initialize routes on server-startup (elastic#84806)
  [Fleet] EPM support to handle uploaded file paths (elastic#84708)
  [Snapshot Restore] Fix initial policy form state (elastic#83928)
  Upgrade Node.js to version 14 (elastic#83425)
  ...
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.

Awesome Recovery improvement!. Code LGTM.

* master: (28 commits)
  [Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty (elastic#84891)
  Only attempt to rollover signals index if version < builtin version (elastic#84982)
  skip flaky suite (elastic#84978)
  skip lens rollup tests
  Add geo containment tracking alert type (elastic#84151)
  Changed rollup tests to use test user rather than elastic super user. (elastic#79567)
  skip 'should allow creation of lens xy chart' elastic#84957
  [APM] Add log_level/sanitize_field_names config options to Python Agent (elastic#84810)
  [Maps] geo line source (elastic#76572)
  [data.search] Move search method inside session service and add tests (elastic#84715)
  skip lens drag and drop test.  elastic#84941
  [Ingest Node Pipelines] Integrate painless autocomplete (elastic#84554)
  [Lens] allow drag and drop reorder on xyChart for y dimension (elastic#84640)
  [Lens] Fix error when selecting the current field again (elastic#84817)
  [Metrics UI] Add metadata tab to node details flyout (elastic#84454)
  [CI] Enables APM collection (elastic#81731)
  [Workplace Search] Migrate Sources Schema tree (elastic#84847)
  Disable checking for conflicts when copying saved objects (elastic#83575)
  [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (elastic#84368)
  y18n 4.0.0 -> 4.0.1 (elastic#84905)
  ...
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 8.0MB 8.0MB +8.0B
triggersActionsUi 1.5MB 1.5MB +6.0KB
total +6.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerts 66.4KB 66.5KB +111.0B
triggersActionsUi 161.5KB 161.7KB +189.0B
total +300.0B

History

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

@gmmorris gmmorris merged commit 249a1a4 into elastic:master Dec 4, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 4, 2020
…oups (elastic#84408)

In this PR we introduce a new `recoveryActionGroup` field on AlertTypes which allows an implementor to specify a custom action group which the framework will use when an alert instance goes from _active_ to _inactive_.
By default all alert types will use the existing `RecoveryActionGroup`, but when `recoveryActionGroup` is specified, this group is used instead.

This is applied across the UI, event log and underlying object model, rather than just being a label change.
To support this we also introduced the `alertActionGroupName` message variable which is the human readable version of existing `alertActionGroup` variable.
# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/action_type_form.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 4, 2020
…fields

* 'master' of github.com:elastic/kibana:
  [ILM] Fix delete phase serialization bug (elastic#84870)
  chore(NA): removes auto install of pre-commit hook (elastic#83566)
  chore(NA): upgrade node-sass into last v4.14.1 to stop shipping old node-gyp (elastic#84935)
  [Alerting] Enables AlertTypes to define the custom recovery action groups (elastic#84408)
  [ML] Functional tests - add missing test data cleanup (elastic#84998)
  Migrate privilege/role/user-related operations to a new Elasticsearch client. (elastic#84641)

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts
gmmorris added a commit that referenced this pull request Dec 4, 2020
…oups (#84408) (#85022)

In this PR we introduce a new `recoveryActionGroup` field on AlertTypes which allows an implementor to specify a custom action group which the framework will use when an alert instance goes from _active_ to _inactive_.
By default all alert types will use the existing `RecoveryActionGroup`, but when `recoveryActionGroup` is specified, this group is used instead.

This is applied across the UI, event log and underlying object model, rather than just being a label change.
To support this we also introduced the `alertActionGroupName` message variable which is the human readable version of existing `alertActionGroup` variable.
# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/action_type_form.tsx
@mikecote mikecote added needs_docs release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Dec 16, 2020
@gmmorris
Copy link
Contributor Author

Decided to remove the needs_docs label on this as it is documented in the Developer Docs and as an end user can't use this feature (it is used by Alert Type implementors), so it doesn't make sense to add them to user docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting UI] Ability for alert type to define the context specific name for Recovered action group
7 participants