[Alerting] Encourage type safe usage of Alerting #86623

gmmorris · 2020-12-21T10:36:46Z

Summary

This PR encourages type safe usage of the Alerting framework by replacing the current default Params/State/InstanceState/InstanceContext types (which are AlertTypeParams/AlertTypeState/etc.) with never.
This means that code can continue to omit the specific types for these fields, as long as they aren't referenced.
Once an alert developer wishes to actually reference the parameters (or state/context), then they have to specify the type.

This PR also changed the typing of the AlertTypeParams and AlertTypeState from Record<string, any> to Record<string, unknown>, to ensure that where these catch-all types are used they will at least enforce unknown rather than any.
This change broke some usage in both @elastic/kibana-alerting-services plugins, but also other plugins in the Stack/Solutions. I tried to fix these where I could, but some of these require new types and refactoring in other teams' code, which I decided is best done by the team who own and maintain that code - I've added explicit TODO comments in all of these places, describing the required fix.

This PR also introduced a Generics based typing for the Alert type so that the params field can be typed as something other than AlertTypeParams.

Checklist

Delete any items that are not applicable to this PR.

~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~[ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~[ ] This was checked for cross-browser compatibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

* 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 · 2020-12-21T11:35:16Z

x-pack/plugins/alerts/server/alert_type_registry.ts

@@ -32,7 +32,7 @@ export interface ConstructorOptions {

 export interface RegistryAlertType
  extends Pick<
-    NormalizedAlertType,
+    UntypedNormalizedAlertType,


We can't have mixed types in the Map, so we have to string these when we set them, and rely on the user to provide the right type when they get them.

gmmorris · 2020-12-21T11:36:45Z

x-pack/plugins/alerts/server/alert_type_registry.ts

+export type UntypedNormalizedAlertType = NormalizedAlertType<
+  AlertTypeParams,
+  AlertTypeState,
+  AlertInstanceState,
+  AlertInstanceContext
+>;
+


By Untyped... what we mean is that we down-cast the types to their base type, but it sounded clearer to me than BaseTypeNormalizedAlertType or DownCastTypedNormalizedAlertType

gmmorris · 2020-12-21T11:37:54Z

x-pack/plugins/alerts/server/alert_type_registry.ts

+    Params extends AlertTypeParams,
+    State extends AlertTypeState,
+    InstanceState extends AlertInstanceState,
+    InstanceContext extends AlertInstanceContext


Removed defaults so we force the developers to be explicit about the types they expect.
This should encourage them to use real types rather than just Record<string, whatever>

gmmorris · 2020-12-21T11:41:21Z

x-pack/plugins/alerts/server/alerts_client/alerts_client.ts

  }

-  public async get({ id }: { id: string }): Promise<SanitizedAlert> {
+  public async get<Params extends AlertTypeParams = never>({


Using never here felt right - we don't force them to specify the type, unless they want to use the field, in which case they'll have to be explicit.

gmmorris · 2020-12-21T12:08:28Z

x-pack/plugins/triggers_actions_ui/public/types.ts

  alertInterval: string;
  alertThrottle: string;
  setAlertParams: (property: string, value: any) => void;
-  setAlertProperty: <Key extends keyof Alert>(key: Key, value: Alert[Key] | null) => void;
+  setAlertProperty: <Key extends keyof Alert>(key: Key, value: any) => void;


Turns out this type was wrong the whole time, and had an underlying any.
At least now it's explicit... but I'll still try to fix this before we merge.

elasticmachine · 2020-12-21T12:42:26Z

Pinging @elastic/uptime (Team:uptime)

chrisronline

LGTM for Stack Monitoring

ymao1

LGTM! Cleanup looks great.

kibanamachine · 2020-12-21T15:03:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1071113

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
`stackAlerts`	114.2KB	114.3KB	+100.0B

Distributable file count

id	before	after	diff
`default`	47142	47902	+760

Page load bundle

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

id	before	after	diff
`triggersActionsUi`	162.5KB	162.3KB	-126.0B

History

💔 Build #95696 failed 561ebb1
💔 Build #95705 failed 5ddcf7a
💔 Build #95693 failed bfcab2a

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

andrewvc

LGTM

YulNaumenko

LGTM

dhurley14

LGTM

This PR encourages type safe usage of the Alerting framework by replacing the current default Params/State/InstanceState/InstanceContext types (which are `AlertTypeParams`/`AlertTypeState`/etc.) with `never`. This means that code can continue to omit the specific types for these fields, as long as they aren't referenced. Once an alert developer wishes to actually reference the parameters (or state/context), then they have to specify the type. This PR also changed the typing of the `AlertTypeParams` and `AlertTypeState` from `Record<string, any>` to `Record<string, unknown>`, to ensure that where these catch-all types are used they will at least enforce `unknown` rather than `any`. This change broke some usage in both @elastic/kibana-alerting-services plugins, but also other plugins in the Stack/Solutions. I tried to fix these where I could, but some of these require new types and refactoring in other teams' code, which I decided is best done by the team who own and maintain that code - I've added explicit `TODO` comments in all of these places, describing the required fix. This PR also introduced a Generics based typing for the `Alert` type so that the `params` field can be typed as something other than `AlertTypeParams`.

* 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 added 2 commits December 18, 2020 10:49

removed defaults from AlertType

c4863a4

removed any usage from AlertTypeState and AlertTypeParams

bfcab2a

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 21, 2020

gmmorris added 2 commits December 21, 2020 10:54

fixed typing in tests

5ddcf7a

gmmorris commented Dec 21, 2020

View reviewed changes

removed dead code

93f10cb

gmmorris commented Dec 21, 2020

View reviewed changes

added TODOs

f4c543a

gmmorris marked this pull request as ready for review December 21, 2020 12:20

gmmorris requested review from a team as code owners December 21, 2020 12:20

gmmorris requested a review from a team December 21, 2020 12:20

gmmorris requested a review from a team as a code owner December 21, 2020 12:20

gmmorris added 2 commits December 21, 2020 12:23

mixed test

aa95e0e

corrected AlertTypeModel typing

dd833fc

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Dec 21, 2020

renamed generics

1071113

chrisronline approved these changes Dec 21, 2020

View reviewed changes

ymao1 approved these changes Dec 21, 2020

View reviewed changes

andrewvc approved these changes Dec 21, 2020

View reviewed changes

YulNaumenko approved these changes Dec 21, 2020

View reviewed changes

dhurley14 approved these changes Dec 21, 2020

View reviewed changes

gmmorris merged commit 2db7666 into elastic:master Dec 21, 2020

gmmorris mentioned this pull request Dec 21, 2020

[7.x] [Alerting] Encourage type safe usage of Alerting (#86623) #86686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] Encourage type safe usage of Alerting #86623

[Alerting] Encourage type safe usage of Alerting #86623

gmmorris commented Dec 21, 2020 •

edited

Loading

gmmorris Dec 21, 2020

gmmorris Dec 21, 2020

gmmorris Dec 21, 2020

gmmorris Dec 21, 2020

gmmorris Dec 21, 2020

elasticmachine commented Dec 21, 2020

chrisronline left a comment

ymao1 left a comment

kibanamachine commented Dec 21, 2020

andrewvc left a comment

YulNaumenko left a comment

dhurley14 left a comment

[Alerting] Encourage type safe usage of Alerting #86623

[Alerting] Encourage type safe usage of Alerting #86623

Conversation

gmmorris commented Dec 21, 2020 • edited Loading

Summary

Checklist

For maintainers

gmmorris Dec 21, 2020

Choose a reason for hiding this comment

gmmorris Dec 21, 2020

Choose a reason for hiding this comment

gmmorris Dec 21, 2020

Choose a reason for hiding this comment

gmmorris Dec 21, 2020

Choose a reason for hiding this comment

gmmorris Dec 21, 2020

Choose a reason for hiding this comment

elasticmachine commented Dec 21, 2020

chrisronline left a comment

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 21, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

andrewvc left a comment

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

dhurley14 left a comment

Choose a reason for hiding this comment

gmmorris commented Dec 21, 2020 •

edited

Loading