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

[Discover] Fix performance regression in sidebar #109999

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import { IndexPatternField } from '../../../../../../../data/public';
import { stubIndexPattern } from '../../../../../../../data/common/stubs';

jest.mock('../../../../../kibana_services', () => ({
getUiActions: jest.fn(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

less error messages when executing the test

return {
getTriggerCompatibleActions: jest.fn(() => []),
};
}),
getServices: () => ({
history: () => ({
location: {
Expand Down Expand Up @@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,36 @@ function DiscoverFieldComponent({
</EuiPopoverTitle>
);

const details = getDetails(field);
const renderPopover = () => {
const details = getDetails(field);
Copy link
Member Author

Choose a reason for hiding this comment

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

making sure getDetails is just executed when needed (popover is displayed)

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
Expand Down Expand Up @@ -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>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;

Expand All @@ -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({
Expand Down Expand Up @@ -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');
Expand All @@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}
Comment on lines +124 to +130
Copy link
Member Author

@kertal kertal Aug 26, 2021

Choose a reason for hiding this comment

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

make surecalcFieldCounts is just executed once

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 this can be moved to useEffect, before subscription, which should initialized after mounting


const [documentState, setDocumentState] = useState(props.documents$.getValue());
useEffect(() => {
const subscription = props.documents$.subscribe((next) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

documentState dependency change leads to initializing subscription two times. So the first listener will not be cleaned up, but I guess it was before.

if (next.fetchStatus !== documentState.fetchStatus) {
if (next.result) {
fieldCounts.current = calcFieldCounts(
next.result.length ? fieldCounts.current : {},
next.result.length && fieldCounts.current ? fieldCounts.current : {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still having trouble understanding this logic. we're calculating fieldCounts, passing in the value that we got as a result of passing in the previous result of calcFieldCounts?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it's like it behaved for a long time, didn't change, but I think it should be in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this logic needs some revisiting, but after testing I don't think it's broken. Let's see if we can do some cleanup here in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 wanna add an issue for that?

next.result,
props.selectedIndexPattern!
);
Expand Down