[Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin #82951

gmmorris · 2020-11-09T13:45:56Z

Summary

closes #80958

This PR includes the following refactors:

Moves the Index Pattern Api from Stack Alerts to the Server plugin of Trigger Actions UI. This fixes a potential bug where a user could disable the Stack Alerts plugin and inadvertently break the UI of the _ES Index _ action type.
Extracts the UI components for Index Threshold and Geo Threshold from the Trigger Actions UI plugin and moves them into Stack Alerts.

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: (39 commits) Fix ilm navigation (elastic#81664) [Lens] Distinct icons for XY and pie chart value labels toolbar (elastic#82927) [data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. (elastic#81509) Index patterns api - load field list on server (elastic#82629) New events resolver (elastic#82170) [App Search] Misc naming tech debt (elastic#82770) load empty_kibana in test to have clean starting point (elastic#82772) Remove data <--> expressions circular dependencies. (elastic#82685) Update 8.0 breaking change template to gather information on how to programmatically detect it. (elastic#82905) Add alerting as codeowners to related documentation folder (elastic#82777) Add captions to user and space grid pages (elastic#82713) add alternate path for x-pack/Cloud test for Lens (elastic#82634) Uses asCurrentUser in getClusterUuid (elastic#82908) [Alerting][Connectors] Add new executor subaction to get 3rd party case fields (elastic#82519) Fix test import objects (elastic#82767) [ML] Add option for anomaly charts for metric detector should plot min, mean or max as appropriate (elastic#81662) Update alert type selection layout to rows instead of grid (elastic#73665) Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (elastic#82817) Update grunt and related packages (elastic#79327) Allow the repository to search across all namespaces (elastic#82863) ...

gmmorris · 2020-11-10T10:42:26Z

x-pack/plugins/stack_alerts/public/alert_types/threshold/index_threshold_api.ts

+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+import { HttpSetup } from 'kibana/public';
+import { TimeSeriesResult } from '../../../../triggers_actions_ui/common';
+import { IndexThresholdAlertParams } from './types';
+
+const INDEX_THRESHOLD_DATA_API_ROOT = '/api/triggers_actions_ui/data';
+
+export interface GetThresholdAlertVisualizationDataParams {
+  model: IndexThresholdAlertParams;
+  visualizeOptions: {
+    rangeFrom: string;
+    rangeTo: string;
+    interval: string;
+  };
+  http: HttpSetup;
+}
+
+export async function getThresholdAlertVisualizationData({
+  model,
+  visualizeOptions,
+  http,
+}: GetThresholdAlertVisualizationDataParams): Promise<TimeSeriesResult> {
+  const timeSeriesQueryParams = {
+    index: model.index,
+    timeField: model.timeField,
+    aggType: model.aggType,
+    aggField: model.aggField,
+    groupBy: model.groupBy,
+    termField: model.termField,
+    termSize: model.termSize,
+    timeWindowSize: model.timeWindowSize,
+    timeWindowUnit: model.timeWindowUnit,
+    dateStart: new Date(visualizeOptions.rangeFrom).toISOString(),
+    dateEnd: new Date(visualizeOptions.rangeTo).toISOString(),
+    interval: visualizeOptions.interval,
+  };
+
+  return await http.post<TimeSeriesResult>(`${INDEX_THRESHOLD_DATA_API_ROOT}/_time_series_query`, {
+    body: JSON.stringify(timeSeriesQueryParams),
+  });
+}


Extracted from the original index_threshold_api.ts which lived in triggers_actions_ui, so this ins't new code, it's just been split into two files - one lives in stack_alerts and the other stays in triggers_actions_ui (which is now renamed data_apis).

gmmorris · 2020-11-10T10:44:19Z

x-pack/plugins/stack_alerts/server/alert_types/index_threshold/alert_type_params.test.ts

+
+export function runTests(schema: ObjectType, defaultTypeParams: Record<string, unknown>): void {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  let params: any;
+
+  const CoreDefaultParams: Writable<Partial<CoreQueryParams>> = {


This helper test function has been duplicated between the two plugins.
This is because both plugins have components using the same Api and the same schema.

I couldn't think of a clean way of sharing this test code in both plugins, so chose to duplicate instead, but it's worth noting as the schema itself is shared it means these tests are linked through the schema, so changing the schema will be caught by both components that rely on it - which is probably a good thing.

elasticmachine · 2020-11-10T11:38:42Z

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

ymao1

LGTM! I tried creating an Index Threshold & Geo Alert and everything worked as expected.

gmmorris · 2020-11-10T16:47:04Z

@elasticmachine merge upstream

…na into alerts/stack-alerts-public * 'alerts/stack-alerts-public' of github.com:gmmorris/kibana: [Security Solution] Fix DNS Network table query (elastic#82778) [Workplace Search] Consolidate groups routes (elastic#83015) Adds cloud links to user menu (elastic#82803) [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023) [App Search] Added the log retention panel to the Settings page (elastic#82982) [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006) [DOCS] Consolidates drilldown pages (elastic#82081) [Maps] add on-prem EMS config (elastic#82525) migrate i18n mixin to KP (elastic#81799) [bundle optimization] fix imports of react-use lib (elastic#82847) [Discover] Add metric on adding filter (elastic#82961) [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829) skip flaky suite (elastic#82804) Fix SO query for searching across spaces (elastic#83025) renaming built-in alerts to Stack Alerts (elastic#82873) [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278) [Visualizations] Remove kui usage (elastic#82810) [Visualizations] Make the icon buttons labels more descriptive (elastic#82585) [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694) :

spalger

Operations: new plugin, new limit, LGTM

pmuellr

LGTM, nice clean up, super nice to see such a minimal set of changes to the FT (as you would expect for a PR like this). Only one real meaningful comment, about whether we need the schema.maybe() on enableGeoTrackingThresholdAlert.

pmuellr · 2020-11-10T18:54:07Z

x-pack/plugins/stack_alerts/common/config.ts

@@ -8,6 +8,7 @@ import { schema, TypeOf } from '@kbn/config-schema';

 export const configSchema = schema.object({
  enabled: schema.boolean({ defaultValue: true }),
+  enableGeoTrackingThresholdAlert: schema.maybe(schema.boolean({ defaultValue: false })),


I think using defaultValue: false means you don't need a schema.maybe() wrapper. Or else I'm not sure what this could mean. When would the defaultValue be applied?

This is always a bit of a confusing aspect to me with config-schema, and other type builders like this. :-) I'd prefer we use the "fewer possibilities" of what the value can be, when we can, and removing the schema.maybe() means that property should always have a boolean value vs boolean | undefined, which seems like will be easier for us anyway.

That's a fair point - I copied it as is from the original plugin.
Think you're right... I'll sort that out.

pmuellr · 2020-11-10T18:57:10Z

x-pack/plugins/stack_alerts/kibana.json

@@ -3,7 +3,7 @@
  "server": true,
  "version": "8.0.0",
  "kibanaVersion": "kibana",
-  "requiredPlugins": ["alerts", "features"],
+  "requiredPlugins": ["alerts", "features", "triggersActionsUi", "kibanaReact"],


We don't have a case where someone would want to disable triggersActionsUi but have the stack alerts still usable, right? Probably anyway? :-)

I could imagine a situation where we might someday want to build a UI-free version of Kibana to be able to run the alerts/actions without the UI, but that's super-long term, and no need to account for that yet, I don't think.

Just wanted to make sure we didn't have any other use cases like that ...

For the time being we'll have to live with it as a dependency for actions or alerts 🤷
In the future, sure, we might want to offer alerting without UI, but for now I think this is ok.

pmuellr · 2020-11-10T19:13:07Z

x-pack/plugins/stack_alerts/server/alert_types/index_threshold/README.md

@@ -123,227 +120,6 @@ server    log   [17:32:10.060] [warning][actions][actions][plugins] \
 [now-iso]: https://github.com/pmuellr/now-iso


-## http endpoints
+## Data Apis via the TriggersActionsUi plugin and its http endpoints


ahhh ... now I see, the index action UIs are also using these APIs I guess. Confused me for a bit as to why the APIs moved there. I think this makes sense for now, but does seem a little weird - I guess it feels like maybe we need YAAP (yet another alerting plugin) where these back-end APIs might live, to separate it out a bit from the mainly UI plugin (I guess I kinda alluded to that in a previous comment re: the new plugin dependencies). A change to make later, if we ever need to ...

TBH I think these APIs belong in the data plugin... hence the name ;)
For now it makes sense in the Triggers Actions UI as it's a plugin we can expect to always be enabled if actions or alerts are in use

pmuellr · 2020-11-10T19:14:01Z

x-pack/plugins/stack_alerts/server/alert_types/index_threshold/alert_type.ts

@@ -152,7 +155,7 @@ export function getAlertType(service: Service): AlertType<Params, {}, {}, Action
      interval: undefined,
    };
    // console.log(`index_threshold: query: ${JSON.stringify(queryParams, null, 4)}`);
-    const result = await service.indexThreshold.timeSeriesQuery({
+    const result = await (await data).timeSeriesQuery({


await await (hehe)

Yeah I hate it 😆

…ts-public * upstream/master: (57 commits) Remove unused asciidoc file (elastic#83228) [Lens] Remove background from lens embeddable (elastic#83061) [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991) Removing unnecessary trailing slash in CODEOWNERS Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236) Trying to fix CODEOWERS, missing a starting slash (elastic#83233) skip flaky suite (elastic#83231) Add enzyme rerender test helper (elastic#83208) Move Elasticsearch type definitions out of APM (elastic#83081) [ts/checkTsProjects] produce a more useful error message (elastic#83209) [kbnClient] retry updating config if necessary (elastic#83205) I accidentally removed this line in a recent PR (elastic#83201) Don't make the caller do work the function can do (elastic#83180) [App Search] Update EngineRouter & EngineNav to use EngineLogic (elastic#83138) [Workplace Search] Add routes for Sources (elastic#83125) Update logstash pipeline management to use system index APIs (elastic#80405) [ML] Replace EuiBasicTable with EuiInMemoryTable (elastic#83057) [Metrics UI] Add basic interaction and shell for node details overlay (elastic#82013) [App Search] Added the log retention confirmation modal to the Settings page (elastic#83009) [docs] Fix create map title in import geospatial page (elastic#83172) ...

* master: [Advanced Settings] Introducing telemetry (elastic#82860) [alerts] add executionStatus to event log doc for action execute (elastic#82401) Add additional sources routes (elastic#83227) [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149) [Logs UI] Add pagination to the log stream shared component (elastic#81193) [Index Management] Add an index template link to data stream details (elastic#82592) Add maps_oss folder to code_owners (elastic#83204) fix truncation issue (elastic#83000) [Ingest Manger] Move asset getters out of registry (elastic#83214)

kibanamachine · 2020-11-12T16:38:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 84ecfdb

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`stackAlerts`	-	31	+31
`triggersActionsUi`	297	275	-22
total			+9

Async chunks

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

id	before	after	diff
`stackAlerts`	-	79.6KB	+79.6KB
`triggersActionsUi`	1.5MB	1.4MB	-95.3KB
total			-15.7KB

Distributable file count

id	before	after	diff
`default`	42775	42782	+7

Page load bundle

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

id	before	after	diff
`stackAlerts`	-	14.3KB	+14.3KB
`triggersActionsUi`	133.1KB	140.0KB	+6.9KB
total			+21.2KB

Unknown metric groups

async chunk count

id	before	after	diff
`stackAlerts`	-	2	+2
`triggersActionsUi`	33	31	-2
total			-0

History

💔 Build #87566 failed 90bcf6d
💔 Build #87181 failed 839463f
💔 Build #87168 failed 0104cb6
💔 Build #87147 failed a7f37a9
💔 Build #87123 failed 0e3a745

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

* master: [ML] Update apidoc config with the Trained models endpoints (elastic#83274) [Fleet] IngestManager Plugin interface for registering UI extensions (elastic#82783) [Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin (elastic#82951)

…Public Plugin (elastic#82951) This PR includes the following refactors: 1. Moves the Index Pattern Api from _Stack Alerts_ to the _Server_ plugin of _Trigger Actions UI_. This fixes a potential bug where a user could disable the _Stack Alerts_ plugin and inadvertently break the UI of the _ES Index _ action type. 2. Extracts the UI components for _Index Threshold_ and _Geo Threshold_ from the _Trigger Actions UI_ plugin and moves them into _Stack Alerts_. # Conflicts: # packages/kbn-optimizer/limits.yml

…Public Plugin (#82951) (#83316) This PR includes the following refactors: 1. Moves the Index Pattern Api from _Stack Alerts_ to the _Server_ plugin of _Trigger Actions UI_. This fixes a potential bug where a user could disable the _Stack Alerts_ plugin and inadvertently break the UI of the _ES Index _ action type. 2. Extracts the UI components for _Index Threshold_ and _Geo Threshold_ from the _Trigger Actions UI_ plugin and moves them into _Stack Alerts_. # Conflicts: # packages/kbn-optimizer/limits.yml

gmmorris added 8 commits November 9, 2020 13:45

extracted index-threshold apis into triggers actions ui

188ddd8

fixed i18n

d6459b1

moved Threshold UIs into Stack Alerts

e02056e

removed unused imports

c7ae42d

fixed i18n in geo

56b286d

renamed index apis

f3ca392

renamed index apis to data apis

b6df73b

gmmorris commented Nov 10, 2020

View reviewed changes

gmmorris changed the title ~~extracted index-threshold apis into triggers actions ui~~ [Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin Nov 10, 2020

gmmorris added 2 commits November 10, 2020 11:25

fixed i18n

72f0816

fixed import in test

b94d2c3

gmmorris marked this pull request as ready for review November 10, 2020 11:34

gmmorris requested review from a team as code owners November 10, 2020 11:34

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.11.0 v8.0.0 labels Nov 10, 2020

ymao1 approved these changes Nov 10, 2020

View reviewed changes

gmmorris added 2 commits November 10, 2020 14:42

updated i18n label

a8d1a8f

fixed i18n translations

0e3a745

kibanamachine and others added 3 commits November 10, 2020 11:47

Merge branch 'master' into alerts/stack-alerts-public

a7f37a9

added stackAlerts to limits

2f616f1

gmmorris requested a review from a team as a code owner November 10, 2020 17:58

fixed import

839463f

spalger approved these changes Nov 10, 2020

View reviewed changes

pmuellr approved these changes Nov 10, 2020

View reviewed changes

ryankeairns approved these changes Nov 10, 2020

View reviewed changes

gmmorris added 4 commits November 12, 2020 09:45

make defaulted field non maybe

845f359

removed import from plugin code as it causes FTR to fail

9e2beac

gmmorris merged commit ab72206 into elastic:master Nov 12, 2020

gmmorris mentioned this pull request Nov 12, 2020

[7.x] [Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin (#82951) #83316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin #82951

[Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin #82951

gmmorris commented Nov 9, 2020 •

edited

Loading

gmmorris Nov 10, 2020

gmmorris Nov 10, 2020 •

edited

Loading

elasticmachine commented Nov 10, 2020

ymao1 left a comment

gmmorris commented Nov 10, 2020

spalger left a comment

pmuellr left a comment

pmuellr Nov 10, 2020

gmmorris Nov 11, 2020

pmuellr Nov 10, 2020

gmmorris Nov 12, 2020

pmuellr Nov 10, 2020

gmmorris Nov 12, 2020

pmuellr Nov 10, 2020

gmmorris Nov 11, 2020

kibanamachine commented Nov 12, 2020

async chunk count

[Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin #82951

[Alerting] Moves the Index & Geo Threshold UIs into the Stack Alerts Public Plugin #82951

Conversation

gmmorris commented Nov 9, 2020 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

gmmorris Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 10, 2020

ymao1 left a comment

Choose a reason for hiding this comment

gmmorris commented Nov 10, 2020

spalger left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Nov 12, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

async chunk count

History

gmmorris commented Nov 9, 2020 •

edited

Loading

gmmorris Nov 10, 2020 •

edited

Loading