Skip to content

Commit

Permalink
[Security Solution] Improve rules exception flyout opening for the in…
Browse files Browse the repository at this point in the history
…dices with huge amount of fields (elastic#159216)

## Summary

Original ticket:
[elastic#158751](elastic#158751)

These changes improve the rule's exceptions flyout opening experience.
We had a few complaints that it is very slow to open it and sometimes it
throws an exception about the limited response size.

To fix this, we decided to load extended field's data (conflicts and
unmapped info) only when user selects some field instead of fetching
this data for all fields on flyout opening.

## NOTES:

After these changes we gonna do next steps related to fields loading
when user creates/edits rule exceptions:
1. We will call `_fields_for_wildcard` **WITHOUT**
`include_unmapped=true` parameter to fetch all fields specs on exception
flyout loading
2. We will call `_fields_for_wildcard` **WITH** `include_unmapped=true`
for only one field when user selects it from the dropdown menu

With these changes we will improve slow exception flyout opening when
user has lots of fields which are unmapped in different indices. If for
some reason user has a lot of (thousands) conflicting fields around
indices then the loading is still might be slow as the
`_fields_for_wildcard` call will return conflicts information even
without `include_unmapped=true` parameter.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 31b3477)
  • Loading branch information
e40pud committed Jun 15, 2023
1 parent c65f896 commit 9edb58f
Show file tree
Hide file tree
Showing 13 changed files with 107 additions and 66 deletions.
Expand Up @@ -197,14 +197,15 @@ describe('BuilderEntryItem', () => {
);
});

test('it render mapping issues warning text when field has mapping conflicts', () => {
test('it render mapping issues warning text when field has mapping conflicts', async () => {
const field = getField('mapping issues');
wrapper = mount(
<BuilderEntryItem
autocompleteService={autocompleteStartMock}
entry={{
correspondingKeywordField: undefined,
entryIndex: 0,
field: getField('mapping issues'),
field,
id: '123',
nested: undefined,
operator: isOperator,
Expand All @@ -223,12 +224,16 @@ describe('BuilderEntryItem', () => {
setWarningsExist={jest.fn()}
showLabel
allowCustomOptions
getExtendedFields={(): Promise<FieldSpec[]> => Promise.resolve([field])}
/>
);

expect(wrapper.find('.euiFormHelpText.euiFormRow__text').text()).toMatch(
/This field is defined as different types across the following indices or is unmapped. This can cause unexpected query results./
);
await waitFor(() => {
wrapper.update();
expect(wrapper.find('.euiFormHelpText.euiFormRow__text').text()).toMatch(
/This field is defined as different types across the following indices or is unmapped. This can cause unexpected query results./
);
});
});

test('it renders field values correctly when operator is "isOperator"', () => {
Expand Down
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { useCallback, useMemo } from 'react';
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { FormattedMessage } from '@kbn/i18n-react';
import {
EuiAccordion,
Expand All @@ -26,6 +26,7 @@ import {
} from '@kbn/securitysolution-io-ts-list-types';
import {
BuilderEntry,
DataViewField,
EXCEPTION_OPERATORS_ONLY_LISTS,
FormattedBuilderEntry,
OperatorOption,
Expand Down Expand Up @@ -87,6 +88,7 @@ export interface EntryItemProps {
isDisabled?: boolean;
operatorsList?: OperatorOption[];
allowCustomOptions?: boolean;
getExtendedFields?: (fields: string[]) => Promise<DataViewField[]>;
}

export const BuilderEntryItem: React.FC<EntryItemProps> = ({
Expand All @@ -106,6 +108,7 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
isDisabled = false,
operatorsList,
allowCustomOptions = false,
getExtendedFields,
}): JSX.Element => {
const sPaddingSize = useEuiPaddingSize('s');

Expand Down Expand Up @@ -175,6 +178,22 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
[onChange, entry]
);

const [extendedField, setExtendedField] = useState<DataViewField | null>(null);
useEffect(() => {
if (!entry.field?.name) {
setExtendedField(null);
}
const fetchExtendedField = async (): Promise<void> => {
const fieldName = entry.field?.name;
if (getExtendedFields && fieldName) {
const extendedFields = await getExtendedFields([fieldName]);
const field = extendedFields.find((f) => f.name === fieldName) ?? null;
setExtendedField(field);
}
};
fetchExtendedField();
}, [entry.field?.name, getExtendedFields]);

const isFieldComponentDisabled = useMemo(
(): boolean =>
isDisabled ||
Expand Down Expand Up @@ -212,8 +231,11 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
);

const warningIconCss = { marginRight: `${sPaddingSize}` };
const getMappingConflictsWarning = (field: DataViewFieldBase): React.ReactNode | null => {
const conflictsInfo = getMappingConflictsInfo(field);
const getMappingConflictsWarning = (): React.ReactNode | null => {
if (!extendedField) {
return null;
}
const conflictsInfo = getMappingConflictsInfo(extendedField);
if (!conflictsInfo) {
return null;
}
Expand All @@ -238,13 +260,13 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
data-test-subj="mappingConflictsAccordion"
>
<div data-test-subj="mappingConflictsDescription">
{conflictsInfo.map((info) => {
{conflictsInfo.map((info, idx) => {
const groupDetails = info.groupedIndices.map(
({ name, count }) =>
`${count > 1 ? i18n.CONFLICT_MULTIPLE_INDEX_DESCRIPTION(name, count) : name}`
);
return (
<>
<EuiFlexItem key={`${idx}`}>
<EuiSpacer size="s" />
{`${
info.totalIndexCount > 1
Expand All @@ -254,7 +276,7 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
)
: info.type
}: ${groupDetails.join(', ')}`}
</>
</EuiFlexItem>
);
})}
<EuiSpacer size="s" />
Expand All @@ -268,12 +290,12 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
entry.nested == null && allowCustomOptions ? i18n.CUSTOM_COMBOBOX_OPTION_TEXT : undefined;

const helpText =
entry.field?.conflictDescriptions == null ? (
extendedField?.conflictDescriptions == null ? (
customOptionText
) : (
<>
{customOptionText}
{getMappingConflictsWarning(entry.field)}
{getMappingConflictsWarning()}
</>
);
return (
Expand All @@ -295,8 +317,9 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
osTypes,
isDisabled,
handleFieldChange,
sPaddingSize,
allowCustomOptions,
sPaddingSize,
extendedField,
]
);

Expand Down
Expand Up @@ -13,6 +13,7 @@ import { HttpStart } from '@kbn/core/public';
import { ExceptionListType, OsTypeArray } from '@kbn/securitysolution-io-ts-list-types';
import {
BuilderEntry,
DataViewField,
ExceptionsBuilderExceptionItem,
FormattedBuilderEntry,
OperatorOption,
Expand Down Expand Up @@ -65,6 +66,7 @@ interface BuilderExceptionListItemProps {
isDisabled?: boolean;
operatorsList?: OperatorOption[];
allowCustomOptions?: boolean;
getExtendedFields?: (fields: string[]) => Promise<DataViewField[]>;
}

export const BuilderExceptionListItemComponent = React.memo<BuilderExceptionListItemProps>(
Expand All @@ -88,6 +90,7 @@ export const BuilderExceptionListItemComponent = React.memo<BuilderExceptionList
isDisabled = false,
operatorsList,
allowCustomOptions = false,
getExtendedFields,
}) => {
const handleEntryChange = useCallback(
(entry: BuilderEntry, entryIndex: number): void => {
Expand Down Expand Up @@ -161,6 +164,7 @@ export const BuilderExceptionListItemComponent = React.memo<BuilderExceptionList
}
operatorsList={operatorsList}
allowCustomOptions={allowCustomOptions}
getExtendedFields={getExtendedFields}
/>
</MyOverflowContainer>
<BuilderEntryDeleteButtonComponent
Expand Down
Expand Up @@ -22,6 +22,7 @@ import {
} from '@kbn/securitysolution-io-ts-list-types';
import {
CreateExceptionListItemBuilderSchema,
DataViewField,
ExceptionsBuilderExceptionItem,
ExceptionsBuilderReturnExceptionItem,
OperatorOption,
Expand Down Expand Up @@ -98,6 +99,7 @@ export interface ExceptionBuilderProps {
operatorsList?: OperatorOption[];
exceptionItemName?: string;
allowCustomFieldOptions?: boolean;
getExtendedFields?: (fields: string[]) => Promise<DataViewField[]>;
}

export const ExceptionBuilderComponent = ({
Expand All @@ -121,6 +123,7 @@ export const ExceptionBuilderComponent = ({
osTypes,
operatorsList,
allowCustomFieldOptions = false,
getExtendedFields,
}: ExceptionBuilderProps): JSX.Element => {
const [state, dispatch] = useReducer(exceptionsBuilderReducer(), {
...initialState,
Expand Down Expand Up @@ -442,6 +445,7 @@ export const ExceptionBuilderComponent = ({
isDisabled={isDisabled}
operatorsList={operatorsList}
allowCustomOptions={allowCustomFieldOptions}
getExtendedFields={getExtendedFields}
/>
</EuiFlexItem>
</EuiFlexGroup>
Expand Down
Expand Up @@ -42,11 +42,7 @@ export const getAllFieldsByName = (
keyBy('name', getAllBrowserFields(browserFields));

export const getIndexFields = memoizeOne(
(
title: string,
fields: IIndexPatternFieldList,
_includeUnmapped: boolean = false
): DataViewBase =>
(title: string, fields: IIndexPatternFieldList): DataViewBase =>
fields && fields.length > 0
? {
fields: fields.map((field) =>
Expand All @@ -66,10 +62,7 @@ export const getIndexFields = memoizeOne(
title,
}
: { fields: [], title },
(newArgs, lastArgs) =>
newArgs[0] === lastArgs[0] &&
newArgs[1].length === lastArgs[1].length &&
newArgs[2] === lastArgs[2]
(newArgs, lastArgs) => newArgs[0] === lastArgs[0] && newArgs[1].length === lastArgs[1].length
);

const DEFAULT_BROWSER_FIELDS = {};
Expand All @@ -96,8 +89,7 @@ interface FetchIndexReturn {
export const useFetchIndex = (
indexNames: string[],
onlyCheckIfIndicesExist: boolean = false,
strategy: 'indexFields' | 'dataView' | typeof ENDPOINT_FIELDS_SEARCH_STRATEGY = 'indexFields',
includeUnmapped: boolean = false
strategy: 'indexFields' | 'dataView' | typeof ENDPOINT_FIELDS_SEARCH_STRATEGY = 'indexFields'
): [boolean, FetchIndexReturn] => {
const { data } = useKibana().services;
const abortCtrl = useRef(new AbortController());
Expand All @@ -121,11 +113,7 @@ export const useFetchIndex = (
abortCtrl.current = new AbortController();
const dv = await data.dataViews.create({ title: iNames.join(','), allowNoIndex: true });
const dataView = dv.toSpec();
const { browserFields } = getDataViewStateFromIndexFields(
iNames,
dataView.fields,
includeUnmapped
);
const { browserFields } = getDataViewStateFromIndexFields(iNames, dataView.fields);

previousIndexesName.current = dv.getIndexPattern().split(',');

Expand All @@ -135,7 +123,7 @@ export const useFetchIndex = (
browserFields,
indexes: dv.getIndexPattern().split(','),
indexExists: dv.getIndexPattern().split(',').length > 0,
indexPatterns: getIndexFields(dv.getIndexPattern(), dv.fields, includeUnmapped),
indexPatterns: getIndexFields(dv.getIndexPattern(), dv.fields),
});
} catch (exc) {
setState({
Expand All @@ -152,7 +140,7 @@ export const useFetchIndex = (

asyncSearch();
},
[addError, data.dataViews, includeUnmapped, indexNames, state]
[addError, data.dataViews, indexNames, state]
);

useEffect(() => {
Expand Down
Expand Up @@ -48,11 +48,7 @@ interface DataViewInfo {
* VERY mutatious on purpose to improve the performance of the transform.
*/
export const getDataViewStateFromIndexFields = memoizeOne(
(
_title: string,
fields: DataViewSpec['fields'],
_includeUnmapped: boolean = false
): DataViewInfo => {
(_title: string, fields: DataViewSpec['fields']): DataViewInfo => {
// Adds two dangerous casts to allow for mutations within this function
type DangerCastForMutation = Record<string, {}>;
if (fields == null) {
Expand All @@ -72,10 +68,7 @@ export const getDataViewStateFromIndexFields = memoizeOne(
return { browserFields: browserFields as DangerCastForBrowserFieldsMutation };
}
},
(newArgs, lastArgs) =>
newArgs[0] === lastArgs[0] &&
newArgs[1]?.length === lastArgs[1]?.length &&
newArgs[2] === lastArgs[2]
(newArgs, lastArgs) => newArgs[0] === lastArgs[0] && newArgs[1]?.length === lastArgs[1]?.length
);

export const useDataView = (): {
Expand Down
Expand Up @@ -438,8 +438,7 @@ export const useSourcererDataView = (
const browserFields = useCallback(() => {
const { browserFields: dataViewBrowserFields } = getDataViewStateFromIndexFields(
sourcererDataView.patternList.join(','),
sourcererDataView.fields,
false
sourcererDataView.fields
);
return dataViewBrowserFields;
}, [sourcererDataView.fields, sourcererDataView.patternList]);
Expand Down
Expand Up @@ -85,6 +85,7 @@ describe('When the add exception modal is opened', () => {
mockFetchIndexPatterns.mockImplementation(() => ({
isLoading: false,
indexPatterns: stubIndexPattern,
getExtendedFields: () => Promise.resolve([]),
}));

mockUseSignalIndex.mockImplementation(() => ({
Expand Down Expand Up @@ -153,6 +154,7 @@ describe('When the add exception modal is opened', () => {
mockFetchIndexPatterns.mockImplementation(() => ({
isLoading: true,
indexPatterns: { fields: [], title: 'foo' },
getExtendedFields: () => Promise.resolve([]),
}));

wrapper = mount(
Expand Down
Expand Up @@ -114,7 +114,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
onCancel,
onConfirm,
}: AddExceptionFlyoutProps) {
const { isLoading, indexPatterns } = useFetchIndexPatterns(rules);
const { isLoading, indexPatterns, getExtendedFields } = useFetchIndexPatterns(rules);
const [isSubmitting, submitNewExceptionItems] = useAddNewExceptionItems();
const [isClosingAlerts, closeAlerts] = useCloseAlertsFromExceptions();
const invalidateFetchRuleByIdQuery = useInvalidateFetchRuleByIdQuery();
Expand Down Expand Up @@ -502,6 +502,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
onExceptionItemAdd={setExceptionItemsToAdd}
onSetErrorExists={setConditionsValidationError}
onFilterIndexPatterns={filterIndexPatterns}
getExtendedFields={getExtendedFields}
/>

{listType !== ExceptionListTypeEnum.ENDPOINT && !sharedListToAddTo?.length && (
Expand Down
Expand Up @@ -124,6 +124,7 @@ describe('When the edit exception modal is opened', () => {
mockFetchIndexPatterns.mockImplementation(() => ({
isLoading: false,
indexPatterns: stubIndexPattern,
getExtendedFields: () => Promise.resolve([]),
}));
mockUseFindExceptionListReferences.mockImplementation(() => [
false,
Expand Down Expand Up @@ -168,6 +169,7 @@ describe('When the edit exception modal is opened', () => {
mockFetchIndexPatterns.mockImplementation(() => ({
isLoading: true,
indexPatterns: { fields: [], title: 'foo' },
getExtendedFields: () => Promise.resolve([]),
}));

const wrapper = mount(
Expand Down
Expand Up @@ -109,7 +109,7 @@ const EditExceptionFlyoutComponent: React.FC<EditExceptionFlyoutProps> = ({
const rules = useMemo(() => (rule != null ? [rule] : null), [rule]);
const listType = useMemo((): ExceptionListTypeEnum => list.type as ExceptionListTypeEnum, [list]);

const { isLoading, indexPatterns } = useFetchIndexPatterns(rules);
const { isLoading, indexPatterns, getExtendedFields } = useFetchIndexPatterns(rules);
const [isSubmitting, submitEditExceptionItems] = useEditExceptionItems();
const [isClosingAlerts, closeAlerts] = useCloseAlertsFromExceptions();

Expand Down Expand Up @@ -370,6 +370,7 @@ const EditExceptionFlyoutComponent: React.FC<EditExceptionFlyoutProps> = ({
onExceptionItemAdd={setExceptionItemsToAdd}
onSetErrorExists={setConditionsValidationError}
onFilterIndexPatterns={filterIndexPatterns}
getExtendedFields={getExtendedFields}
/>
{!openedFromListDetailPage && listType === ExceptionListTypeEnum.DETECTION && (
<>
Expand Down

0 comments on commit 9edb58f

Please sign in to comment.