[Discover] Fix performance regression in sidebar (#109999)

elastic · Aug 26, 2021 · dc07f4d · dc07f4d
1 parent 652470b
commit dc07f4d
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 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
@@ -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
@@ -358,7 +358,36 @@ function DiscoverFieldComponent({
     </EuiPopoverTitle>
   );
 
-  const details = getDetails(field);
+  const renderPopover = () => {
+    const details = getDetails(field);
+    return (
+      <>
+        <DiscoverFieldDetails
+          indexPattern={indexPattern}
+          field={field}
+          details={details}
+          onAddFilter={onAddFilter}
+        />
+        {multiFields && (
+          <>
+            <EuiSpacer size="m" />
+            <MultiFields
+              multiFields={multiFields}
+              alwaysShowActionButton={alwaysShowActionButton}
+              toggleDisplay={toggleDisplay}
+            />
+          </>
+        )}
+        <DiscoverFieldVisualize
+          field={field}
+          indexPattern={indexPattern}
+          multiFields={rawMultiFields}
+          trackUiMetric={trackUiMetric}
+          details={details}
+        />
+      </>
+    );
+  };
 
   return (
     <EuiPopover
@@ -396,33 +425,7 @@ function DiscoverFieldComponent({
           })}
         </h5>
       </EuiTitle>
-      {infoIsOpen && (
-        <>
-          <DiscoverFieldDetails
-            indexPattern={indexPattern}
-            field={field}
-            details={details}
-            onAddFilter={onAddFilter}
-          />
-          {multiFields && (
-            <>
-              <EuiSpacer size="m" />
-              <MultiFields
-                multiFields={multiFields}
-                alwaysShowActionButton={alwaysShowActionButton}
-                toggleDisplay={toggleDisplay}
-              />
-            </>
-          )}
-          <DiscoverFieldVisualize
-            field={field}
-            indexPattern={indexPattern}
-            multiFields={rawMultiFields}
-            trackUiMetric={trackUiMetric}
-            details={details}
-          />
-        </>
-      )}
+      {infoIsOpen && renderPopover()}
     </EuiPopover>
   );
 }

diff --git a/...over/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx b/...over/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<string, number> = {};
+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<IndexPatternAttributes>,
   ];
 
+  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/.../discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx b/.../discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx
@@ -120,17 +120,22 @@ 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<Record<string, number>>(
-    calcFieldCounts({}, props.documents$.getValue().result, props.selectedIndexPattern)
-  );
+  const fieldCounts = useRef<Record<string, number> | null>(null);
+  if (fieldCounts.current === null) {
+    fieldCounts.current = calcFieldCounts(
+      {},
+      props.documents$.getValue().result,
+      props.selectedIndexPattern
+    );
+  }
 
   const [documentState, setDocumentState] = useState(props.documents$.getValue());
   useEffect(() => {
     const subscription = props.documents$.subscribe((next) => {
       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!
           );