[Alerting UI] Replaced AlertsContextProvider with KibanaContextProvider and exposed components in API by YulNaumenko · Pull Request #84604 · elastic/kibana

YulNaumenko · 2020-12-01T06:15:04Z

Removed AlertsContextProvider
Exposed AlertAdd and AlertEdit flyouts in triggers_actions_ui plugin start
Did the proper changes in other plugins to use api instead of components by itself.

…er and exposed components in API

…n start

elasticmachine · 2020-12-02T06:51:58Z

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

elasticmachine · 2020-12-02T23:40:42Z

Pinging @elastic/uptime (Team:uptime)

elasticmachine · 2020-12-02T23:40:42Z

Pinging @elastic/apm-ui (Team:apm)

Zacqary

Code looks good; Logs and Metrics UIs seem to work as expected

andrewvc

LGTM from uptime

ymao1

All the changes look great! Stack alerts section all seems to work as expected.

When I look in Metrics, in Metrics Explorer tab and create an alert. There are a few percentage expressions to fill in. When I click on any of the percentages to populate, the flyout closes and reopens.

x-pack/plugins/infra/public/alerting/metric_threshold/components/alert_flyout.tsx

x-pack/plugins/monitoring/kibana.json

ymao1

Looks good!

chrisronline

LGTM for Stack Monitoring

mikecote

Changes LGTM! I left a few comments regarding the typing and docs

mikecote · 2020-12-04T12:27:19Z

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

+  charts?: ChartsPluginSetup;
+  data?: DataPublicPluginStart;


Since these are required plugins for triggers actions ui to work, we will always have a value for them (similar to TriggersAndActionsUiServices). We should be able to remove the empty or data?. type checks in other files as well.

Suggested change

charts?: ChartsPluginSetup;

data?: DataPublicPluginStart;

charts: ChartsPluginSetup;

data: DataPublicPluginStart;

mikecote · 2020-12-04T12:33:39Z

x-pack/plugins/triggers_actions_ui/README.md

-    metadata: { test: 'some value', fields: ['test'] },
-  }}
->
  <AlertAdd consumer={'watcher'} addFlyoutVisible={alertFlyoutVisible}


Since this is the Embed the Create Alert flyout within any Kibana plugin section of the docs, should we explain how to do it using the plugin start contract?

mikecote · 2020-12-04T12:34:40Z

x-pack/plugins/triggers_actions_ui/README.md

-}
-```
-
 |Property|Description|


This table below should also be removed?

mikecote · 2020-12-04T12:34:43Z

x-pack/plugins/triggers_actions_ui/README.md

-}
-```
-
 |Property|Description|


This table below should also be removed?

mikecote · 2020-12-04T12:38:18Z

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

+  getAddAlertFlyout: (
+    props: Omit<AlertAddProps, 'actionTypeRegistry' | 'alertTypeRegistry'>
+  ) => ReactElement<AlertAddProps> | null;
+  getEditAlertFlyout: (
+    props: Omit<AlertEditProps, 'actionTypeRegistry' | 'alertTypeRegistry'>
+  ) => ReactElement<AlertEditProps> | null;


nit: it seems these functions don't have a scenario that returns null.

Suggested change

getAddAlertFlyout: (

props: Omit<AlertAddProps, 'actionTypeRegistry' | 'alertTypeRegistry'>

) => ReactElement<AlertAddProps> | null;

getEditAlertFlyout: (

props: Omit<AlertEditProps, 'actionTypeRegistry' | 'alertTypeRegistry'>

) => ReactElement<AlertEditProps> | null;

getAddAlertFlyout: (

props: Omit<AlertAddProps, 'actionTypeRegistry' | 'alertTypeRegistry'>

) => ReactElement<AlertAddProps>;

getEditAlertFlyout: (

props: Omit<AlertEditProps, 'actionTypeRegistry' | 'alertTypeRegistry'>

) => ReactElement<AlertEditProps>;

mikecote · 2020-12-04T12:51:33Z

x-pack/plugins/stack_alerts/public/alert_types/threshold/visualization.tsx

+  charts?: ChartsPluginSetup;
+  dataFieldsFormats?: FieldFormatsStart;


I think these will always have a value once the changes in x-pack/plugins/triggers_actions_ui/public/types.ts is done.

Suggested change

charts?: ChartsPluginSetup;

dataFieldsFormats?: FieldFormatsStart;

charts: ChartsPluginSetup;

dataFieldsFormats: FieldFormatsStart;

mikecote · 2020-12-04T12:53:54Z

x-pack/plugins/stack_alerts/public/alert_types/threshold/expression.tsx


    if (index && index.length > 0) {
-      const currentEsFields = await getFields(http, index);
+      const currentEsFields = await getFields(http!, index);


I wonder if there will be scenarios where http is undefined now?

mikecote · 2020-12-04T12:54:33Z

x-pack/plugins/stack_alerts/public/alert_types/geo_threshold/query_builder/index.tsx

+  if (!data) {
+    throw new Error('KibanaContext has not been initalized correctly.');
+  }


We should be able to remove this and some further checks below once the changes in x-pack/plugins/triggers_actions_ui/public/types.ts are done.

dgieselaar · 2020-12-06T10:41:22Z

x-pack/plugins/apm/public/components/alerting/AlertingFlyout/index.tsx

+  const AddAlertFlyout = useMemo(
+    () =>
+      alertType &&
+      triggersActionsUi.getAddAlertFlyout({
+        consumer: 'apm',
+        addFlyoutVisible,
+        setAddFlyoutVisibility,
+        alertTypeId: alertType,
+        canChangeTrigger: false,
+      }),
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [addFlyoutVisible, alertType]
  );
+  return <>{AddAlertFlyout}</>;


Is there a reason why this cannot be a component that the alerting UI exports?

From the code reducing prospective it sounds right to expose it as component, but our decision was to live the details of the flyout usage to the end plugin - as a result it can be without memoizations or with the own dependancies list.

So the memoization is not needed?

This looks ok, but since it's not a component could we make the variable lowercase? Also why are we disabling exhaustive-deps? If there's a good reason for that please add a comment explaining why.

dgieselaar · 2020-12-06T10:42:25Z

x-pack/plugins/apm/public/application/index.tsx

+    <ApmAppRoot
+      apmPluginContextValue={apmPluginContextValue}
+      startDeps={startDeps}
+    />,


Ideally startDeps is part of the plugin context value, similar to plugins. We've not named the properties here appropriately but we'll change that separately from this PR.

Sounds right for me, I had the similar thoughts, but decided not to modify your plugin context. Are you OK to have it for this PR as it is currently implemented?

Sure thing.

kindsun

Geo alerts changes lgtm!

code review

smith

See my comment on the add flyout component. Other than that everything looks ok. We can follow-up later with the dependency props.

smith · 2020-12-07T21:32:00Z

x-pack/plugins/apm/public/components/alerting/AlertingFlyout/index.tsx

+  const AddAlertFlyout = useMemo(
+    () =>
+      alertType &&
+      triggersActionsUi.getAddAlertFlyout({
+        consumer: 'apm',
+        addFlyoutVisible,
+        setAddFlyoutVisibility,
+        alertTypeId: alertType,
+        canChangeTrigger: false,
+      }),
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [addFlyoutVisible, alertType]
  );
+  return <>{AddAlertFlyout}</>;


This looks ok, but since it's not a component could we make the variable lowercase? Also why are we disabling exhaustive-deps? If there's a good reason for that please add a comment explaining why.

kibanamachine · 2020-12-08T00:31:20Z

💚 Build Succeeded

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

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	285	286	+1
`uptime`	541	540	-1
total			-0

Async chunks

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

id	before	after	diff
`apm`	3.1MB	3.1MB	-133.0B
`infra`	2.7MB	2.7MB	-1.1KB
`monitoring`	952.5KB	951.7KB	-798.0B
`stackAlerts`	114.0KB	114.2KB	+138.0B
`triggersActionsUi`	1.5MB	1.5MB	-1.1KB
`uptime`	1006.9KB	1006.1KB	-835.0B
total			-3.7KB

Distributable file count

id	before	after	diff
`default`	46930	47690	+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.8KB	48.8KB	+60.0B
`infra`	182.1KB	181.7KB	-435.0B
`monitoring`	35.5KB	35.3KB	-239.0B
`triggersActionsUi`	161.7KB	161.7KB	-31.0B
`uptime`	24.9KB	24.7KB	-244.0B
total			-889.0B

History

💚 Build #92503 succeeded 4ddc524
💔 Build #92492 failed 74d3b5d
💔 Build #92478 failed 9097aea
💚 Build #92130 succeeded c8e249e
💔 Build #92115 failed eb1e611

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

…or-disable-searchable-snapshot-fields * 'master' of github.com:elastic/kibana: (29 commits) HTTP CRUD+ API for Index Patterns (elastic#83576) Don't list packages in devDependencies already listed in dependencies (elastic#85120) Remove unused devDependencies (elastic#85121) [ILM] Reposition form toggles (elastic#85143) [APM] Make sure jest script can be run from anywhere (elastic#85111) Add EuiButtonEmptyTo components (elastic#85213) skip flaky suite (elastic#85216) skip flaky suite (elastic#83775) skip flaky suite (elastic#83774) skip flaky suite (elastic#83773) skip flaky suite (elastic#83793) skip flaky suite (elastic#85215) skip flaky suite (elastic#85217) [Usage Collection] Kibana Overview Page UI Metrics (elastic#81937) [Alerting UI] Replaced AlertsContextProvider with KibanaContextProvider and exposed components in API (elastic#84604) skip flaky suite (elastic#85208) [Security Solutions][Detection Engine] Fixes cypress errors by using latest signals mapping (elastic#84600) Small fixes to Kibana search bar (elastic#85079) Change link text to say Fleet (elastic#85083) [Metrics UI] Refactor Process List API to fetch only top processes (elastic#84716) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx

[Alerting UI] Replaced AlertsContextProvider with KibanaContextProvid…

fc8a25a

…er and exposed components in API

YulNaumenko self-assigned this Dec 1, 2020

YulNaumenko added 4 commits December 1, 2020 15:02

removed AlertContextProvider

e868dc5

exposed AlertAdd and EditAlert flyouts with triggers_actions_ui plugi…

31bfbf1

…n start

Merge remote-tracking branch upstream/master

1d23be2

fixed type check

019e223

YulNaumenko added Feature:Alerting Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 2, 2020

YulNaumenko marked this pull request as ready for review December 2, 2020 06:51

YulNaumenko requested a review from a team December 2, 2020 06:51

YulNaumenko requested review from a team as code owners December 2, 2020 06:51

YulNaumenko requested a review from a team December 2, 2020 06:51

YulNaumenko requested a review from a team as a code owner December 2, 2020 06:51

fixed tests

0702d63

botelastic bot added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Dec 2, 2020

YulNaumenko added 2 commits December 2, 2020 15:42

Merge remote-tracking branch upstream/master

98a0baf

fixed typechecks

a8f19c5

Zacqary approved these changes Dec 3, 2020

View reviewed changes

andrewvc approved these changes Dec 3, 2020

View reviewed changes

ymao1 reviewed Dec 3, 2020

View reviewed changes

x-pack/plugins/infra/public/alerting/metric_threshold/components/alert_flyout.tsx Outdated Show resolved Hide resolved

YulNaumenko added 2 commits December 3, 2020 12:54

fixed wrong consumer

3147ea8

Merge remote-tracking branch upstream/master

cc5f649

chrisronline reviewed Dec 3, 2020

View reviewed changes

x-pack/plugins/monitoring/kibana.json Show resolved Hide resolved

fixed monitoring flyout flickering

f3a86ff

YulNaumenko requested review from chrisronline and ymao1 December 3, 2020 21:12

ymao1 approved these changes Dec 3, 2020

View reviewed changes

chrisronline approved these changes Dec 4, 2020

View reviewed changes

mikecote self-requested a review December 4, 2020 12:11

mikecote approved these changes Dec 4, 2020

View reviewed changes

YulNaumenko added 4 commits December 4, 2020 10:39

Merge remote-tracking branch upstream/master

3c7e302

fixed due to comments

a8c0f57

fixed typechecks

eb1e611

fixed typechecks

c8e249e

dgieselaar reviewed Dec 6, 2020

View reviewed changes

kindsun approved these changes Dec 7, 2020

View reviewed changes

YulNaumenko added 3 commits December 7, 2020 10:54

Merge remote-tracking branch upstream/master

9097aea

fixed typechecks

74d3b5d

fixed typechecks

4ddc524

smith requested changes Dec 7, 2020

View reviewed changes

fixed due to comments

7972208

YulNaumenko requested a review from smith December 7, 2020 22:52

smith approved these changes Dec 7, 2020

View reviewed changes

YulNaumenko merged commit 6757b95 into elastic:master Dec 8, 2020

YulNaumenko mentioned this pull request Dec 8, 2020

[7.x] [Alerting UI] Replaced AlertsContextProvider with KibanaContextProvider and exposed components in API (#84604) #85218

Merged

This was referenced Dec 14, 2020

[Alerts] Remove Add Alerts flyout onClose #85462

Merged

[Alerting] fixes the examples plugin #85774

Merged

XavierM mentioned this pull request Feb 10, 2021

[SECURITY SOLUTIONS] Meta to move cases everywhere #90905

Closed

15 tasks

		charts?: ChartsPluginSetup;
		dataFieldsFormats?: FieldFormatsStart;

Conversation

YulNaumenko commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 2, 2020

Uh oh!

elasticmachine commented Dec 2, 2020

Uh oh!

elasticmachine commented Dec 2, 2020

Uh oh!

Zacqary left a comment

Choose a reason for hiding this comment

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

chrisronline left a comment

Choose a reason for hiding this comment

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YulNaumenko Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kindsun left a comment

Choose a reason for hiding this comment

Uh oh!

smith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 8, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

History

Uh oh!

Reviewers

YulNaumenko commented Dec 1, 2020 •

edited

Loading

YulNaumenko Dec 7, 2020 •

edited

Loading