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

Make the alerts plugin support generics #72716

Merged
merged 22 commits into from
Aug 14, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jul 21, 2020

Solves the alerts portion of #64147.

Resolves #64147 now that all portions are complete after this PR.

In this PR, I'm adding support for generics in the alerts plugin to the following types:

  • AlertServices<InstanceState, InstanceContext>
  • AlertExecutorOptions<Params, State, InstanceState, InstanceContext>
  • AlertType<Params, State, InstanceState, InstanceContext>

Example

export type AlertTypeParams = { threshold: number; index: string };

const thresholdAlertType: AlertType<AlertTypeParams, {}, {}, {}> = {
  ...
  async executor(execOptions) {
    // Below is considered a number with the type pulled from AlertType['threshold']
    const { threshold } = execOptions.params;
  }
}

@mikecote mikecote added Feature:Alerting v8.0.0 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.10.0 labels Jul 21, 2020
@mikecote mikecote self-assigned this Jul 21, 2020
this.taskManager.registerTaskDefinitions({
[`alerting:${alertType.id}`]: {
title: alertType.name,
type: `alerting:${alertType.id}`,
createTaskRunner: (context: RunContext) =>
this.taskRunnerFactory.create(alertType, context),
this.taskRunnerFactory.create({ ...alertType } as AlertType, context),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

{ ...alertType } is a "while I'm here" for shallow clones that was missed in #65309.

@mikecote mikecote marked this pull request as ready for review August 5, 2020 21:12
@mikecote mikecote requested review from a team as code owners August 5, 2020 21:12
@mikecote mikecote requested a review from a team August 5, 2020 21:12
@elasticmachine
Copy link
Contributor

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

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

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

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

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

import { parseDuration } from '../lib';

interface ScheduledExecutionOptions {
Copy link
Member

Choose a reason for hiding this comment

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

+1 on an interface removal - that's neat!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, for this one here I had to move it within the AlertInstance class in order to use the generics passed to the class.

return result;
}

function getPatternFiringAlertType() {
Copy link
Member

Choose a reason for hiding this comment

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

eh, we'll have a merge conflict here with https://github.com/elastic/kibana/pull/68437/files#diff-753fd9a204535630ddf989cebd2a4a76 - I had to make the patternFiring alert a little more interesting, to handle multiple instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for the heads up! If your PR merges soon, I'll wait until then as the changes I made aren't hard to redo.

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Great work Mike!

Comment on lines +9 to +13
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type AlertTypeState = Record<string, any>;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type AlertTypeParams = Record<string, any>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be a generic type with a default of unknown instead of any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converting these types to Record<string, unknown> caused over 130 type check failures. Most of these were outside of the alerting team's code and would cause us to change a lot of external code. I'm thinking we enforce this at some other time and have created #74897 to track this.

Copy link
Member

@spong spong 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 Security changes!

@mikecote mikecote requested a review from a team as a code owner August 14, 2020 14:39
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM !!

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Aug 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
alerts 89.3KB +155.0B 89.1KB

History

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

@mikecote mikecote merged commit d56b792 into elastic:master Aug 14, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Aug 14, 2020
* Initial work

* Expand generic support to alert instances

* Convert index threshold to use generics

* Make fixture alert types use generics

* Make alert instance related types use unknown

* Fix typecheck failures

* Cleanup + add instance generic support to registry.get API

* Shallow clone

* Rename some TS variables

* Fix failing api integration tests

* Change code for easier review and keep more history

* Fix

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 17, 2020
* master: (24 commits)
  [ML] Functional tests - skip regression and classification tests
  [Ingest Manager] fix removing ingest pipelines from elasticsearch (elastic#75092)
  move tests for placeholder indices to setup (elastic#75096)
  [jest] temporarily extend default test timeout (elastic#75118)
  [cli] remove reference to removed --optimize flag (elastic#75083)
  skip flaky suite (elastic#75044)
  Adding /etc/rc.d/init.d/functions to the init script when present to … (elastic#22985)
  [jenkins] add pipeline for hourly security solution cypress tests (elastic#75087)
  [Reporting/Flaky Test] Skip test for paging list of reports (elastic#75075)
  remove .kbn-optimizer-cache upload (elastic#75086)
  skip flaky suite (elastic#74814)
  Actions add proxy support (elastic#74289)
  [ILM] TS conversion of Edit policy components (elastic#74747)
  [Resolver] simulator tests select elements directly instead of using descendant selectors. (elastic#75058)
  [Enterprise Search] Add Workplace Search side navigation (elastic#74894)
  [Security solution] Sourcerer: Kibana index pattern selector for security views (elastic#74706)
  [Logs UI] Remove apollo deps from log link-to routes (elastic#74502)
  [Maps] add map configurations to docker list (elastic#75035)
  [functional test][saved objects] update tests for additional copy saved objects to space (elastic#74907)
  Make the alerts plugin support generics (elastic#72716)
  ...
mikecote added a commit that referenced this pull request Aug 17, 2020
* Initial work

* Expand generic support to alert instances

* Convert index threshold to use generics

* Make fixture alert types use generics

* Make alert instance related types use unknown

* Fix typecheck failures

* Cleanup + add instance generic support to registry.get API

* Shallow clone

* Rename some TS variables

* Fix failing api integration tests

* Change code for easier review and keep more history

* Fix

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] Add Generics support to Alerting
9 participants