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] Enforces typing of Alert's ActionGroups #86761

Merged
merged 17 commits into from
Jan 5, 2021
Merged

[Alerting] Enforces typing of Alert's ActionGroups #86761

merged 17 commits into from
Jan 5, 2021

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Dec 22, 2020 •
edited
Loading

Summary

closes #83501

This PR tightens the typing on the Alerting framework's AlertType and its deeper typing around AlertServices and AlertExecutorOptions.

This ensures the following:

  1. It's now impossible✴ to schedule actions on any ActionGroup other than the groups specified on the AlertType (including the Recovery group)
  2. It's now impossible✴ to schedule actions with incorrect InstanceState or InstanceContext

✴ Unless they bypass the Typescript typing, which is an explicit choice to bypass type safety

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@gmmorris
removed defaults from AlertType
c4863a4
@gmmorris
removed any usage from AlertTypeState and AlertTypeParams
bfcab2a
@gmmorris
Merge branch 'master' into alerts/remove-generics-defaults-redux
561ebb1 
* master: (48 commits)
  Fix request with disabled aggregation (elastic#85696)
  [Security Solution][Detections][Threshold Rules] Threshold Rule Bug Fixes (elastic#84918)
  Removed a possibility to define two different names for Alert types on API and UI level. (elastic#86236)
  Bump Node.js from version 14.15.2 to 14.15.3 (elastic#86593)
  [index patterns] Fleep app - Keep saved object field list until field caps provides fields (elastic#85370)
  [Security Solutions] fix timeline tabs + layout (elastic#86581)
  Upgrade to hapi version 20 (elastic#85406)
  App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. (elastic#82791)
  Rename chartLibrary setting to legacyChartsLibrary (elastic#86529)
  [CI] TeamCity updates (elastic#85843)
  [Maps] Use Json for mvt-tests (elastic#86492)
  [Rollup Jobs] Added autofocus to cron editor (elastic#86324)
  [Monitoring][Alerting] CCR read exceptions alert (elastic#85908)
  [CI] Bump memory for main CI workers (elastic#86541)
  Explicitly set Elasticsearch heap size during CI and local development (elastic#86513)
  [App Search] Updates to results on the documents view (elastic#86181)
  [Discover] Change default sort handling  (elastic#85561)
  [App Search] Convert DocumentCreationModal to DocumentCreationFlyout (elastic#86508)
  [App Search] Sample Engines should have access to the Crawler (elastic#86502)
  Fixed duplication of create new modal (elastic#86489)
  ...
@gmmorris
fixed typing in tests
5ddcf7a
@gmmorris
removed dead code
93f10cb
@gmmorris
added TODOs
f4c543a
@gmmorris
mixed test
aa95e0e
@gmmorris
corrected AlertTypeModel typing
dd833fc
@gmmorris
renamed generics
1071113
@gmmorris
Merge branch 'master' into alerts/typed-action-groups
d1e98eb 
* master: (36 commits)
  update apm index pattern (elastic#86739)
  [Visualizations] Remove vis_default_editor - visualize plugins cyclic dependencies (elastic#85422)
  [ML] Fix alignment of values in data frame analytics results view badges (elastic#86621)
  [Visualizations] Remove charts - editor plugins cyclic dependencies (elastic#84887)
  fixing blank page (elastic#86640)
  Update dependency vega to ^5.17.1 (elastic#86715)
  [Monitoring] Convert Kibana-related server files that read from _source to typescript (elastic#86364)
  Uses @elastic/elasticsearch-canary (elastic#86398)
  [CI] Removes script previously used for Karma (elastic#86412)
  [build] Remove grunt checkPlugins task (elastic#85852)
  [build] Remove grunt docker:docs task (elastic#85848)
  [ML] Add doc link for classification AUC ROC evaluation (elastic#86660)
  [ML] Edits saved object synchronization message (elastic#86664)
  Uses the new es client in canvas usage collector's fetch methods (elastic#86668)
  [ML] Support legacy watcher URL (elastic#86661)
  [ML] Fix Single Metric Viewer y domain extending beyond the visible focus area (elastic#86655)
  Migrates search telemetry usage collector es client from legacy to new (elastic#86597)
  [Alerting] Encourage type safe usage of Alerting (elastic#86623)
  Migrates kql_telemetry usage collector es client (elastic#86585)
  [ML] Fix time range adjustment for the swim lane causing the infinite loop update (elastic#86461)
  ...
@gmmorris
typed action groups
b79eb96
@gmmorris
fixed typing in test and example plugins
0206690
@gmmorris gmmorris changed the title Alerts/typed action groups [Alerting] Enforces typing of Alert's ActionGroups Dec 30, 2020
gmmorris
gmmorris commented Dec 30, 2020
View reviewed changes
.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts
Comment on lines +123 to +127
/**
* TODO: We're lying to the compiler here as explicitly calling `scheduleActions` on
* the RecoveredActionGroup isn't allowed
*/
(actionGroupId as unknown) as InventoryMetricThresholdAllowedActionGroups,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The InventoryMetricThreshold AlertType relies on specifying the recovery ActionGroup explicitly in order to include a context that is accessible when an instance is recovered. This approach isn't in line with how the system was designed to be used, which is why we have encountered some unforeseen side effects (alert instances recovering twice, for example).

This PR makes this kind of usage impossible unless you lie to the compiler, which is why we do so here.

I have began a dialogue with @Zacqary to find a solution that will satisfy their need, but for now, I'd still like to proceed with this PR to discourage this type of usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikecote I couldn't find an existing issue to explore ways of enabling some kind of context when an instance recovers. Are you aware of one which I can link to here?
If not, I'll open a new one and we can try and find a solution for Metrics.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gmmorris none that I recall, just some conversations in another issue: #49405 (comment).

@gmmorris gmmorris added 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.12.0 v8.0.0 labels Dec 30, 2020
@gmmorris gmmorris marked this pull request as ready for review December 30, 2020 10:59
@gmmorris gmmorris requested review from a team as code owners December 30, 2020 10:59
@gmmorris gmmorris requested a review from a team December 30, 2020 10:59
@gmmorris gmmorris requested a review from a team as a code owner December 30, 2020 10:59
@elasticmachine
Copy link
Contributor

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

@gmmorris gmmorris mentioned this pull request Dec 30, 2020
Closed
3 tasks
mikecote
mikecote approved these changes Dec 30, 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.

Alerting team code changes LGTM!

yctercero
yctercero approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

LGTM detection engine side! Thanks for the type updates. Looks like the changes now specify the defaultActionGroup which for DE we only have one specified (default) here - x-pack/plugins/security_solution/server/lib/detection_engine/signals/siem_rule_action_groups.ts.

Checked out and just played around with basic DE functionality - rule creation, edit (versioning updating), enabling/disabling, and deletion. Looks great, thank you!

@gmmorris
Copy link
Contributor Author

Thanks for the 👍 @yctercero

Looks like the changes now specify the defaultActionGroup which for DE we only have one specified (default) here - x-pack/plugins/security_solution/server/lib/detection_engine/signals/siem_rule_action_groups.ts.

This PR doesn't actually change that, it was already defined, but it wasn't expressed in the type signature, so all this PR did is include that in the type so that it can't be misused.
Does that make sense?

@gmmorris
Copy link
Contributor Author

gmmorris commented Jan 4, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47264 48024 +760

Page load bundle

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

id before after diff
apm 48.6KB 48.7KB +80.0B

History

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

ymao1
ymao1 approved these changes Jan 4, 2021
View reviewed changes
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.

LGTM!

phillipb
phillipb approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Infra type changes LGTM

ogupte
ogupte approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

APM changes LGTM

chrisronline
chrisronline approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM for Stack Monitoring!

@gmmorris gmmorris merged commit 76b8c49 into elastic:master Jan 5, 2021
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 5, 2021
@gmmorris
[Alerting] Enforces typing of Alert's ActionGroups (elastic#86761)
f0df49f 
This PR tightens the typing on the Alerting framework's `AlertType` and its deeper typing around `AlertServices ` and `AlertExecutorOptions`.

This ensures the following:

1. It's now impossible<sup>✴</sup> to schedule actions on any ActionGroup other than the groups specified on the AlertType (including the Recovery group)
2. It's now impossible<sup>✴</sup> to schedule actions with incorrect `InstanceState` or `InstanceContext`

✴ Unless they bypass the Typescript typing, which is an explicit choice to bypass type safety
@gmmorris gmmorris mentioned this pull request Jan 5, 2021
Closed
jbudz added a commit that referenced this pull request Jan 5, 2021
@jbudz
Revert "[Alerting] Enforces typing of Alert's ActionGroups (#86761)"
051be28 
This reverts commit 76b8c49.
@jbudz
Copy link
Member

jbudz commented Jan 5, 2021

I temporarily reverted this due to a typecheck failure:
https://kibana-ci.elastic.co/job/elastic+kibana+master/10922/execution/node/357/log/

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 5, 2021
@gmmorris
Revert "Revert "[Alerting] Enforces typing of Alert's ActionGroups (e…
26e28ea 
…lastic#86761)""

This reverts commit 051be28.
@gmmorris gmmorris mentioned this pull request Jan 5, 2021
Merged
8 tasks
gmmorris added a commit that referenced this pull request Jan 6, 2021
@gmmorris
[Alerting] revert the revert of `Enforces typing of Alert's ActionGro…
b99ca96 
…ups` (#87382)

The #86761 PR was reverted due to a small typing issue.

This PR reverts that revert and adds a commit to address the issue: 9e4ab20.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 6, 2021
@gmmorris
[Alerting] revert the revert of `Enforces typing of Alert's ActionGro…
840e751 
…ups` (elastic#87382)

The elastic#86761 PR was reverted due to a small typing issue.

This PR reverts that revert and adds a commit to address the issue: elastic@9e4ab20.
gmmorris added a commit that referenced this pull request Jan 6, 2021
@gmmorris
[Alerting] revert the revert of `Enforces typing of Alert's ActionGro…
07e96ff 
…ups` (#87382) (#87454)

The #86761 PR was reverted due to a small typing issue.

This PR reverts that revert and adds a commit to address the issue: 9e4ab20.
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 7, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86761 or prevent reminders by adding the backport:skip label.

4 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86761 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86761 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86761 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86761 or prevent reminders by adding the backport:skip label.

@mikecote mikecote added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ymao1 ymao1 ymao1 approved these changes

@yctercero yctercero yctercero approved these changes

@mikecote mikecote mikecote approved these changes

@shahzad31 shahzad31 shahzad31 approved these changes

@ogupte ogupte ogupte approved these changes

@phillipb phillipb phillipb approved these changes

@chrisronline chrisronline chrisronline approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Alerts] Prevent alert types from calling scheduleActions with the resolved action group