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

[Alerting UI] Replaced AlertsContextProvider with KibanaContextProvider and exposed components in API #84604

Merged

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Dec 1, 2020

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

@YulNaumenko YulNaumenko self-assigned this Dec 1, 2020
@YulNaumenko YulNaumenko added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 2, 2020
@YulNaumenko YulNaumenko marked this pull request as ready for review December 2, 2020 06:51
@YulNaumenko YulNaumenko requested review from a team as code owners December 2, 2020 06:51
@YulNaumenko YulNaumenko requested a review from a team December 2, 2020 06:51
@YulNaumenko YulNaumenko requested a review from a team as a code owner December 2, 2020 06:51
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Dec 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM from uptime

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.

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.

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.

Looks good!

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 mikecote self-requested a review December 4, 2020 12:11
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.

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

Comment on lines 171 to 172
charts?: ChartsPluginSetup;
data?: DataPublicPluginStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

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

Choose a reason for hiding this comment

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

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?

dataFieldsFormats?: Pick<FieldFormatsRegistry, 'register'>;
}
```

|Property|Description|
Copy link
Contributor

Choose a reason for hiding this comment

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

This table below should also be removed?

metadata?: MetaData;
}
```

|Property|Description|
Copy link
Contributor

Choose a reason for hiding this comment

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

This table below should also be removed?

Comment on lines 50 to 55
getAddAlertFlyout: (
props: Omit<AlertAddProps, 'actionTypeRegistry' | 'alertTypeRegistry'>
) => ReactElement<AlertAddProps> | null;
getEditAlertFlyout: (
props: Omit<AlertEditProps, 'actionTypeRegistry' | 'alertTypeRegistry'>
) => ReactElement<AlertEditProps> | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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>;

Comment on lines 97 to 98
charts?: ChartsPluginSetup;
dataFieldsFormats?: FieldFormatsStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

@@ -123,7 +123,7 @@ export const IndexThresholdAlertTypeExpression: React.FunctionComponent<
});

if (index && index.length > 0) {
const currentEsFields = await getFields(http, index);
const currentEsFields = await getFields(http!, index);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines 107 to 109
if (!data) {
throw new Error('KibanaContext has not been initalized correctly.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 26 to 39
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}</>;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

So the memoization is not needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +127 to +130
<ApmAppRoot
apmPluginContextValue={apmPluginContextValue}
startDeps={startDeps}
/>,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@YulNaumenko YulNaumenko Dec 7, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing.

Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

Geo alerts changes lgtm!

  • code review

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 26 to 39
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}</>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@YulNaumenko YulNaumenko requested a review from smith December 7, 2020 22:52
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

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

@YulNaumenko YulNaumenko merged commit 6757b95 into elastic:master Dec 8, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 8, 2020
…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
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: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.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.