From dc07f4d00aee2c11f7bd3c7a87f9507acd586a0c Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Thu, 26 Aug 2021 11:30:01 +0200 Subject: [PATCH] [Discover] Fix performance regression in sidebar (#109999) --- .../sidebar/discover_field.test.tsx | 14 +++++ .../components/sidebar/discover_field.tsx | 59 ++++++++++--------- .../discover_sidebar_responsive.test.tsx | 32 ++++++++++ .../sidebar/discover_sidebar_responsive.tsx | 13 ++-- 4 files changed, 86 insertions(+), 32 deletions(-) diff --git a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.test.tsx b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.test.tsx index d0f343a641717..1dfc14d6c20b9 100644 --- a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.test.tsx +++ b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.test.tsx @@ -15,6 +15,11 @@ import { IndexPatternField } from '../../../../../../../data/public'; import { stubIndexPattern } from '../../../../../../../data/common/stubs'; jest.mock('../../../../../kibana_services', () => ({ + getUiActions: jest.fn(() => { + return { + getTriggerCompatibleActions: jest.fn(() => []), + }; + }), getServices: () => ({ history: () => ({ location: { @@ -120,4 +125,13 @@ describe('discover sidebar field', function () { const dscField = findTestSubject(comp, 'field-troubled_field-showDetails'); expect(dscField.find('.kbnFieldButton__infoIcon').length).toEqual(1); }); + it('should not execute getDetails when rendered, since it can be expensive', function () { + const { props } = getComponent({}); + expect(props.getDetails.mock.calls.length).toEqual(0); + }); + it('should execute getDetails when show details is requested', function () { + const { props, comp } = getComponent({}); + findTestSubject(comp, 'field-bytes-showDetails').simulate('click'); + expect(props.getDetails.mock.calls.length).toEqual(1); + }); }); diff --git a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.tsx b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.tsx index 301866c762fbd..707514073e23e 100644 --- a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.tsx +++ b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.tsx @@ -358,7 +358,36 @@ function DiscoverFieldComponent({ ); - const details = getDetails(field); + const renderPopover = () => { + const details = getDetails(field); + return ( + <> + + {multiFields && ( + <> + + + + )} + + + ); + }; return ( - {infoIsOpen && ( - <> - - {multiFields && ( - <> - - - - )} - - - )} + {infoIsOpen && renderPopover()} ); } diff --git a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx index c7395c42bb2f1..fc1c09ec8c829 100644 --- a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx +++ b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx @@ -12,6 +12,7 @@ import { ReactWrapper } from 'enzyme'; import { findTestSubject } from '@elastic/eui/lib/test'; // @ts-expect-error import realHits from '../../../../../__fixtures__/real_hits.js'; +import { act } from 'react-dom/test-utils'; import { mountWithIntl } from '@kbn/test/jest'; import React from 'react'; import { IndexPatternAttributes } from '../../../../../../../data/common'; @@ -49,10 +50,24 @@ const mockServices = ({ }, } as unknown) as DiscoverServices; +const mockfieldCounts: Record = {}; +const mockCalcFieldCounts = jest.fn(() => { + return mockfieldCounts; +}); + jest.mock('../../../../../kibana_services', () => ({ + getUiActions: jest.fn(() => { + return { + getTriggerCompatibleActions: jest.fn(() => []), + }; + }), getServices: () => mockServices, })); +jest.mock('../../utils/calc_field_counts', () => ({ + calcFieldCounts: () => mockCalcFieldCounts(), +})); + function getCompProps(): DiscoverSidebarResponsiveProps { const indexPattern = stubLogstashIndexPattern; @@ -67,6 +82,11 @@ function getCompProps(): DiscoverSidebarResponsiveProps { { id: '2', attributes: { title: 'c' } } as SavedObject, ]; + for (const hit of hits) { + for (const key of Object.keys(indexPattern.flattenHit(hit))) { + mockfieldCounts[key] = (mockfieldCounts[key] || 0) + 1; + } + } return { columns: ['extension'], documents$: new BehaviorSubject({ @@ -102,6 +122,7 @@ describe('discover responsive sidebar', function () { expect(popular.children().length).toBe(1); expect(unpopular.children().length).toBe(7); expect(selected.children().length).toBe(1); + expect(mockCalcFieldCounts.mock.calls.length).toBe(1); }); it('should allow selecting fields', function () { findTestSubject(comp, 'fieldToggle-bytes').simulate('click'); @@ -116,4 +137,15 @@ describe('discover responsive sidebar', function () { findTestSubject(comp, 'plus-extension-gif').simulate('click'); expect(props.onAddFilter).toHaveBeenCalled(); }); + it('should allow filtering by string, and calcFieldCount should just be executed once', function () { + expect(findTestSubject(comp, 'fieldList-unpopular').children().length).toBe(7); + act(() => { + findTestSubject(comp, 'fieldFilterSearchInput').simulate('change', { + target: { value: 'abc' }, + }); + }); + comp.update(); + expect(findTestSubject(comp, 'fieldList-unpopular').children().length).toBe(4); + expect(mockCalcFieldCounts.mock.calls.length).toBe(1); + }); }); diff --git a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx index bbc2328e057d3..7533a54ade405 100644 --- a/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx +++ b/src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx @@ -120,9 +120,14 @@ export function DiscoverSidebarResponsive(props: DiscoverSidebarResponsiveProps) * needed for merging new with old field counts, high likely legacy, but kept this behavior * because not 100% sure in this case */ - const fieldCounts = useRef>( - calcFieldCounts({}, props.documents$.getValue().result, props.selectedIndexPattern) - ); + const fieldCounts = useRef | null>(null); + if (fieldCounts.current === null) { + fieldCounts.current = calcFieldCounts( + {}, + props.documents$.getValue().result, + props.selectedIndexPattern + ); + } const [documentState, setDocumentState] = useState(props.documents$.getValue()); useEffect(() => { @@ -130,7 +135,7 @@ export function DiscoverSidebarResponsive(props: DiscoverSidebarResponsiveProps) if (next.fetchStatus !== documentState.fetchStatus) { if (next.result) { fieldCounts.current = calcFieldCounts( - next.result.length ? fieldCounts.current : {}, + next.result.length && fieldCounts.current ? fieldCounts.current : {}, next.result, props.selectedIndexPattern! );