Skip to content

Commit

Permalink
[Response Ops] [Alerts table] Cell context alert table performance (#…
Browse files Browse the repository at this point in the history
…180016)

## Summary

This pr makes use of a new prop (and some generic memoization fixes) in
2 eui prs merged recently (elastic/eui#7556 and
elastic/eui#7374) to improve the performance of
the alerts table. Mainly, the cellContext prop is now available to
consumers of the triggersActionsUi alerts table to pass in custom props
in a way that allows the renderCellValue/rowCellRender functions of the
various EuiDataGrid prop apis to remain referentially stable across
re-renders. There are also some various changes to various hooks and
props found throughout plugins that use the table to improve
performance. There should be no change in functionality, just a moderate
to substantial reduction in time the app spends rendering the alerts
table in various scenarios. Below will be some react dev tools
performance profiles, main compared to this branch, with the exact same
set of generated data.

Main, switching from 10-100 rows:

![main_alerts_table](https://github.com/elastic/kibana/assets/56408403/6b87093f-5b1b-4d22-8e23-ccc3406317f4)

This branch 10-100 rows:

![context_alerts_table](https://github.com/elastic/kibana/assets/56408403/75bf5d53-045d-42ae-979a-e52d6c0f8020)

Pretty modest gain here, 1 render is slower than any others on main, but
overall less time spent rendering by about a second.

Main, opening the cell popover on 2 cells

![main_open_cell_popover](https://github.com/elastic/kibana/assets/56408403/60c5d132-b526-4859-a29c-5d3157142d50)

This branch, opening cellpopover

![context_open_cell_popover](https://github.com/elastic/kibana/assets/56408403/2c60b250-6a9f-44b4-aec8-f9dcb8c87531)

Again nothing crazy here, modest improvement.

Main opening timeline and hitting refresh


![main_open_timeline](https://github.com/elastic/kibana/assets/56408403/7525200d-cf9b-4f43-9f24-43f314740db1)

This branch, opening timeline and hitting refresh


![context_open_timeline](https://github.com/elastic/kibana/assets/56408403/efd3cf95-a81a-4933-b310-fa61de24258f)

This is the case that brought this about in the first place, as security
solution shares a lot of code between tables, the alerts table
recreating all cells on every render was destroying performance of the
timeline rendering in a flyout/modal while users were on alerts page or
the rule detail page, which is probably the most common use case. 93ms
in this branch vs 2500+ ms in main. This type of performance hit happens
when almost any action is taken in timeline.

Main, selecting 3 alerts 1 at a time

![main_actions](https://github.com/elastic/kibana/assets/56408403/87487149-cf6d-4bcc-9192-b64411abe321)

This branch selecting 3 alerts 1 at a time


![context_actions](https://github.com/elastic/kibana/assets/56408403/8407953a-5af0-4cfd-9f2c-08c710c81fb3)

Pretty substantial improvement here as well, ~2000ms vs 67ms.

Apologies if some of the gifs are cut off, please try the branch vs main
on your own! This was done on a local kibana in dev mode, so things like
emotion are eating up more cpu than they would for a user, but there
still should be improvement, and I can't think of any circumstance where
things will be worse. Also this branch was a little longer lived than I
would have liked, so if you are reviewing and changed something around
these files recently please double check I didn't miss anything.



### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
kqualters-elastic authored Apr 16, 2024
1 parent 47c7174 commit 91c8270
Show file tree
Hide file tree
Showing 65 changed files with 2,235 additions and 1,765 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import type { FieldSpec } from '@kbn/data-views-plugin/common';
import { METRIC_TYPE, UiCounterMetricType } from '@kbn/analytics';
import React, { useCallback, useEffect } from 'react';
import React, { useCallback, useEffect, useMemo } from 'react';

import { groupActions, groupByIdSelector } from './state';
import type { GroupOption } from './types';
Expand Down Expand Up @@ -64,19 +64,21 @@ export const useGetGroupSelectorStateless = ({
[onGroupChange]
);

return (
<GroupSelector
{...{
groupingId,
groupsSelected: ['none'],
'data-test-subj': 'alerts-table-group-selector',
onGroupChange: onChange,
fields,
maxGroupingLevels,
options: defaultGroupingOptions,
}}
/>
);
return useMemo(() => {
return (
<GroupSelector
{...{
groupingId,
groupsSelected: ['none'],
'data-test-subj': 'alerts-table-group-selector',
onGroupChange: onChange,
fields,
maxGroupingLevels,
options: defaultGroupingOptions,
}}
/>
);
}, [groupingId, fields, maxGroupingLevels, defaultGroupingOptions, onChange]);
};

export const useGetGroupSelector = ({
Expand Down Expand Up @@ -198,18 +200,19 @@ export const useGetGroupSelector = ({
}
}, [defaultGroupingOptions, options, selectedGroups, setOptions]);

return (
<GroupSelector
{...{
groupingId,
groupsSelected: selectedGroups,
'data-test-subj': 'alerts-table-group-selector',
onGroupChange: onChange,
fields,
maxGroupingLevels,
options,
title,
}}
/>
);
return useMemo(() => {
return (
<GroupSelector
{...{
groupingId,
groupsSelected: selectedGroups,
'data-test-subj': 'alerts-table-group-selector',
onGroupChange: onChange,
fields,
maxGroupingLevels,
options,
}}
/>
);
}, [groupingId, fields, maxGroupingLevels, onChange, selectedGroups, options]);
};
7 changes: 5 additions & 2 deletions x-pack/examples/triggers_actions_ui_example/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,11 @@ export class TriggersActionsUiExamplePlugin
id: 'observabilityCases',
columns,
useInternalFlyout,
getRenderCellValue: () => (props) => {
const value = props.data.find((d) => d.field === props.columnId)?.value ?? [];
getRenderCellValue: (props: {
data?: Array<{ field: string; value: string }>;
columnId?: string;
}) => {
const value = props.data?.find((d) => d.field === props.columnId)?.value ?? [];

if (Array.isArray(value)) {
return <>{value.length ? value.join() : '--'}</>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { useCallback } from 'react';
import { useCallback, useMemo } from 'react';
import { useApplication } from '../../../common/lib/kibana/use_application';
import { CaseStatuses } from '../../../../common/types/domain';
import type { AllCasesSelectorModalProps } from '.';
Expand Down Expand Up @@ -158,9 +158,11 @@ export const useCasesAddToExistingCaseModal = ({
[closeModal, dispatch, handleOnRowClick, onClose, onCreateCaseClicked]
);

return {
open: openModal,
close: closeModal,
};
return useMemo(() => {
return {
open: openModal,
close: closeModal,
};
}, [openModal, closeModal]);
};
export type UseCasesAddToExistingCaseModal = typeof useCasesAddToExistingCaseModal;
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type React from 'react';
import { useCallback } from 'react';
import { useCallback, useMemo } from 'react';
import type { CaseAttachmentsWithoutOwner } from '../../../types';
import { useCasesToast } from '../../../common/use_cases_toast';
import type { CaseUI } from '../../../containers/types';
Expand Down Expand Up @@ -88,10 +88,12 @@ export const useCasesAddToNewCaseFlyout = ({
onClose,
]
);
return {
open: openFlyout,
close: closeFlyout,
};
return useMemo(() => {
return {
open: openFlyout,
close: closeFlyout,
};
}, [openFlyout, closeFlyout]);
};

export type UseCasesAddToNewCaseFlyout = typeof useCasesAddToNewCaseFlyout;
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export function registerAlertsTableConfiguration(
},
columns,
useInternalFlyout: getAlertFlyout(columns, getAlertFormatters(fieldFormats)),
getRenderCellValue: getRenderCellValue(fieldFormats),
getRenderCellValue,
sort,
useActionsColumn: () => ({
renderCustomActionsRow: (props: RenderCustomActionsRowArgs) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
*/

import { isEmpty } from 'lodash';
import React, { type ReactNode } from 'react';
import React from 'react';
import type { RenderCellValue } from '@elastic/eui';
import { isDefined } from '@kbn/ml-is-defined';
import { ALERT_DURATION, ALERT_END, ALERT_START } from '@kbn/rule-data-utils';
import type { GetRenderCellValue } from '@kbn/triggers-actions-ui-plugin/public';
import type { FieldFormatsRegistry } from '@kbn/field-formats-plugin/common';
import { FIELD_FORMAT_IDS } from '@kbn/field-formats-plugin/common';
import { getFormattedSeverityScore, getSeverityColor } from '@kbn/ml-anomaly-utils';
Expand All @@ -21,11 +21,6 @@ import {
} from '../../../common/constants/alerts';
import { getFieldFormatterProvider } from '../../application/contexts/kibana/use_field_formatter';

interface Props {
columnId: string;
data: any;
}

export const getMappedNonEcsValue = ({
data,
fieldName,
Expand Down Expand Up @@ -54,22 +49,17 @@ const getRenderValue = (mappedNonEcsValue: any) => {
return '—';
};

export const getRenderCellValue = (fieldFormats: FieldFormatsRegistry): GetRenderCellValue => {
export const getRenderCellValue: RenderCellValue = ({ columnId, data, fieldFormats }) => {
const alertValueFormatter = getAlertFormatters(fieldFormats);
if (!isDefined(data)) return;

return ({ setFlyoutAlert }) =>
(props): ReactNode => {
const { columnId, data } = props as Props;
if (!isDefined(data)) return;
const mappedNonEcsValue = getMappedNonEcsValue({
data,
fieldName: columnId,
});
const value = getRenderValue(mappedNonEcsValue);

const mappedNonEcsValue = getMappedNonEcsValue({
data,
fieldName: columnId,
});
const value = getRenderValue(mappedNonEcsValue);

return alertValueFormatter(columnId, value);
};
return alertValueFormatter(columnId, value);
};

export function getAlertFormatters(fieldFormats: FieldFormatsRegistry) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ export const AlertsPanel: FC = () => {

const [isOpen, setIsOpen] = useState(true);
const [toggleSelected, setToggleSelected] = useState(`alertsSummary`);

const {
services: { fieldFormats },
} = useMlKibana();
const { anomalyDetectionAlertsStateService } = useAnomalyExplorerContext();

const countByStatus = useObservable(anomalyDetectionAlertsStateService.countByStatus$);
Expand All @@ -48,6 +50,9 @@ export const AlertsPanel: FC = () => {
query: alertsQuery,
showExpandToDetails: true,
showAlertStatusWithFlapping: true,
cellContext: {
fieldFormats,
},
};
const alertsStateTable = triggersActionsUi!.getAlertsStateTable(alertStateProps);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,7 @@ export const getAlertsPageTableConfiguration = (
id: observabilityFeatureId,
cases: { featureId: casesFeatureId, owner: [observabilityFeatureId] },
columns: getColumns({ showRuleName: true }),
getRenderCellValue: ({ setFlyoutAlert }) =>
getRenderCellValue({
observabilityRuleTypeRegistry,
setFlyoutAlert,
}),
getRenderCellValue,
sort: [
{
[ALERT_START]: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import { ALERT_STATUS, ALERT_STATUS_ACTIVE, ALERT_STATUS_RECOVERED } from '@kbn/rule-data-utils';
import type { DeprecatedCellValueElementProps } from '@kbn/timelines-plugin/common';
import { createObservabilityRuleTypeRegistryMock } from '../../../rules/observability_rule_type_registry_mock';
import { render } from '../../../utils/test_helper';
import { getRenderCellValue } from './render_cell_value';

Expand All @@ -16,17 +15,10 @@ interface AlertsTableRow {
}

describe('getRenderCellValue', () => {
const observabilityRuleTypeRegistryMock = createObservabilityRuleTypeRegistryMock();

const renderCellValue = getRenderCellValue({
setFlyoutAlert: jest.fn(),
observabilityRuleTypeRegistry: observabilityRuleTypeRegistryMock,
});

describe('when column is alert status', () => {
it('should return an active indicator when alert status is active', async () => {
const cell = render(
renderCellValue({
getRenderCellValue({
...requiredProperties,
columnId: ALERT_STATUS,
data: makeAlertsTableRow({ alertStatus: ALERT_STATUS_ACTIVE }),
Expand All @@ -38,7 +30,7 @@ describe('getRenderCellValue', () => {

it('should return a recovered indicator when alert status is recovered', async () => {
const cell = render(
renderCellValue({
getRenderCellValue({
...requiredProperties,
columnId: ALERT_STATUS,
data: makeAlertsTableRow({ alertStatus: ALERT_STATUS_RECOVERED }),
Expand Down
Loading

0 comments on commit 91c8270

Please sign in to comment.