Skip to content

Commit

Permalink
[Security Solutions][Detection Engine] Removes duplicate API calls (#…
Browse files Browse the repository at this point in the history
…88420)

## Summary

This removes some duplicate API calls to reduce pressure on the backend and speed up querying times within the application for the front end. This fixes some of the issues of #82327, but there are several performance improvements that are going to be needed to help reduce the slowness when you have a system under a lot of pressure.

So far this removes duplication for these API calls when you are on the manage detection rules page:

```ts
api/detection_engine/rules/_find
api/detection_engine/rules/_find_statuses
api/detection_engine/tags
```

<img width="2465" alt="Screen Shot 2021-01-14 at 3 53 21 PM" src="https://user-images.githubusercontent.com/1151048/104662295-c031e080-5687-11eb-92d7-18b9ad355646.png">

* This hides the tags and searches while the page is loading to avoid duplicate calls when the pre-packaged rules counts come back
* This untangles the refetchRules from the refetchPrePackagedRulesStatus as two separate calls to avoid issues we have with re-rendering and re-calling the backend.
 
### 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
FrankHassanabad authored Jan 17, 2021
1 parent 7a87c33 commit 2abbd80
Show file tree
Hide file tree
Showing 16 changed files with 105 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import React, { SetStateAction, useEffect, useState } from 'react';
import { fetchQueryAlerts } from './api';
import { AlertSearchResponse } from './types';

type Func = () => void;
type Func = () => Promise<void>;

export interface ReturnQueryAlerts<Hit, Aggs> {
loading: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { createSignalIndex, getSignalIndex } from './api';
import * as i18n from './translations';
import { isSecurityAppError } from '../../../../common/utils/api';

type Func = () => void;
type Func = () => Promise<void>;

export interface ReturnSignalIndex {
loading: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@ export const fetchRules = async ({
...showElasticRuleFilter,
].join(' AND ');

const tags = [
...(filterOptions.tags?.map((t) => `alert.attributes.tags: "${t.replace(/"/g, '\\"')}"`) ?? []),
].join(' AND ');
const tags = filterOptions.tags
.map((t) => `alert.attributes.tags: "${t.replace(/"/g, '\\"')}"`)
.join(' AND ');

const filterString =
filtersWithoutTags !== '' && tags !== ''
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ export interface FilterOptions {
filter: string;
sortField: RulesSortingFields;
sortOrder: SortOrder;
showCustomRules?: boolean;
showElasticRules?: boolean;
tags?: string[];
showCustomRules: boolean;
showElasticRules: boolean;
tags: string[];
}

export interface FetchRulesResponse {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
getPrePackagedTimelineStatus,
} from '../../../pages/detection_engine/rules/helpers';

type Func = () => void;
type Func = () => Promise<void>;
export type CreatePreBuiltRules = () => Promise<boolean>;

interface ReturnPrePackagedTimelines {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,11 @@ export const useRulesStatuses = (rules: Rules): ReturnRulesStatuses => {
setLoading(false);
}
};
if (rules != null && rules.length > 0) {

if (rules.length > 0) {
fetchData(rules.map((r) => r.id));
}

return () => {
isSubscribed = false;
abortCtrl.abort();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ describe('useRules', () => {
filter: '',
sortField: 'created_at',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
},
})
);
Expand All @@ -48,6 +51,9 @@ describe('useRules', () => {
filter: '',
sortField: 'created_at',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
},
})
);
Expand Down Expand Up @@ -153,6 +159,9 @@ describe('useRules', () => {
filter: '',
sortField: 'created_at',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
},
})
);
Expand Down Expand Up @@ -182,6 +191,9 @@ describe('useRules', () => {
filter: '',
sortField: 'created_at',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
},
},
}
Expand All @@ -198,6 +210,9 @@ describe('useRules', () => {
filter: 'hello world',
sortField: 'created_at',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
},
});
await waitForNextUpdate();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,18 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { noop } from 'lodash/fp';
import { useEffect, useState, useRef } from 'react';

import { FetchRulesResponse, FilterOptions, PaginationOptions, Rule } from './types';
import { errorToToaster, useStateToaster } from '../../../../common/components/toasters';
import { fetchRules } from './api';
import * as i18n from './translations';

export type ReturnRules = [
boolean,
FetchRulesResponse | null,
(refreshPrePackagedRule?: boolean) => void
];
export type ReturnRules = [boolean, FetchRulesResponse | null, () => Promise<void>];

export interface UseRules {
pagination: PaginationOptions;
filterOptions: FilterOptions;
refetchPrePackagedRulesStatus?: () => void;
dispatchRulesInReducer?: (rules: Rule[], pagination: Partial<PaginationOptions>) => void;
}

Expand All @@ -34,20 +28,19 @@ export interface UseRules {
export const useRules = ({
pagination,
filterOptions,
refetchPrePackagedRulesStatus,
dispatchRulesInReducer,
}: UseRules): ReturnRules => {
const [rules, setRules] = useState<FetchRulesResponse | null>(null);
const reFetchRules = useRef<(refreshPrePackagedRule?: boolean) => void>(noop);
const reFetchRules = useRef<() => Promise<void>>(() => Promise.resolve());
const [loading, setLoading] = useState(true);
const [, dispatchToaster] = useStateToaster();

const filterTags = filterOptions.tags?.sort().join();
const filterTags = filterOptions.tags.sort().join();
useEffect(() => {
let isSubscribed = true;
const abortCtrl = new AbortController();

async function fetchData() {
const fetchData = async () => {
try {
setLoading(true);
const fetchRulesResult = await fetchRules({
Expand Down Expand Up @@ -77,15 +70,10 @@ export const useRules = ({
if (isSubscribed) {
setLoading(false);
}
}
};

fetchData();
reFetchRules.current = (refreshPrePackagedRule: boolean = false) => {
fetchData();
if (refreshPrePackagedRule && refetchPrePackagedRulesStatus != null) {
refetchPrePackagedRulesStatus();
}
};
reFetchRules.current = (): Promise<void> => fetchData();
return () => {
isSubscribed = false;
abortCtrl.abort();
Expand All @@ -100,7 +88,6 @@ export const useRules = ({
filterTags,
filterOptions.showCustomRules,
filterOptions.showElasticRules,
refetchPrePackagedRulesStatus,
]);

return [loading, rules, reFetchRules.current];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ interface GetBatchItems {
hasMlPermissions: boolean;
hasActionsPrivileges: boolean;
loadingRuleIds: string[];
reFetchRules: (refreshPrePackagedRule?: boolean) => void;
reFetchRules: () => Promise<void>;
refetchPrePackagedRulesStatus: () => Promise<void>;
rules: Rule[];
selectedRuleIds: string[];
}
Expand All @@ -39,17 +40,18 @@ export const getBatchItems = ({
hasMlPermissions,
loadingRuleIds,
reFetchRules,
refetchPrePackagedRulesStatus,
rules,
selectedRuleIds,
hasActionsPrivileges,
}: GetBatchItems) => {
const selectedRules = selectedRuleIds.reduce((acc, id) => {
const selectedRules = selectedRuleIds.reduce<Record<string, Rule>>((acc, id) => {
const found = rules.find((r) => r.id === id);
if (found != null) {
return { [id]: found, ...acc };
}
return acc;
}, {} as Record<string, Rule>);
}, {});

const containsEnabled = selectedRuleIds.some((id) => selectedRules[id]?.enabled ?? false);
const containsDisabled = selectedRuleIds.some((id) => !selectedRules[id]?.enabled ?? false);
Expand Down Expand Up @@ -139,7 +141,8 @@ export const getBatchItems = ({
dispatch,
dispatchToaster
);
reFetchRules(true);
await reFetchRules();
await refetchPrePackagedRulesStatus();
}}
>
<EuiToolTip
Expand All @@ -158,7 +161,8 @@ export const getBatchItems = ({
onClick={async () => {
closePopover();
await deleteRulesAction(selectedRuleIds, dispatch, dispatchToaster);
reFetchRules(true);
await reFetchRules();
await refetchPrePackagedRulesStatus();
}}
>
{i18n.BATCH_ACTION_DELETE_SELECTED}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('AllRulesTable Columns', () => {
const dispatch = jest.fn();
const dispatchToaster = jest.fn();
const reFetchRules = jest.fn();
const refetchPrePackagedRulesStatus = jest.fn();

beforeEach(() => {
results = [];
Expand All @@ -53,6 +54,7 @@ describe('AllRulesTable Columns', () => {
dispatchToaster,
history,
reFetchRules,
refetchPrePackagedRulesStatus,
true
)[1];
await duplicateRulesActionObject.onClick(rule);
Expand All @@ -75,6 +77,7 @@ describe('AllRulesTable Columns', () => {
dispatchToaster,
history,
reFetchRules,
refetchPrePackagedRulesStatus,
true
)[3];
await deleteRulesActionObject.onClick(rule);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ export const getActions = (
dispatch: React.Dispatch<Action>,
dispatchToaster: Dispatch<ActionToaster>,
history: H.History,
reFetchRules: (refreshPrePackagedRule?: boolean) => void,
reFetchRules: () => Promise<void>,
refetchPrePackagedRulesStatus: () => Promise<void>,
actionsPrivileges:
| boolean
| Readonly<{
Expand Down Expand Up @@ -77,7 +78,8 @@ export const getActions = (
enabled: (rowItem: Rule) => canEditRuleWithActions(rowItem, actionsPrivileges),
onClick: async (rowItem: Rule) => {
await duplicateRulesAction([rowItem], [rowItem.id], dispatch, dispatchToaster);
await reFetchRules(true);
await reFetchRules();
await refetchPrePackagedRulesStatus();
},
},
{
Expand All @@ -95,7 +97,8 @@ export const getActions = (
name: i18n.DELETE_RULE,
onClick: async (rowItem: Rule) => {
await deleteRulesAction([rowItem.id], dispatch, dispatchToaster);
await reFetchRules(true);
await reFetchRules();
await refetchPrePackagedRulesStatus();
},
},
];
Expand All @@ -115,7 +118,8 @@ interface GetColumns {
hasMlPermissions: boolean;
hasNoPermissions: boolean;
loadingRuleIds: string[];
reFetchRules: (refreshPrePackagedRule?: boolean) => void;
reFetchRules: () => Promise<void>;
refetchPrePackagedRulesStatus: () => Promise<void>;
hasReadActionsPrivileges:
| boolean
| Readonly<{
Expand All @@ -132,6 +136,7 @@ export const getColumns = ({
hasNoPermissions,
loadingRuleIds,
reFetchRules,
refetchPrePackagedRulesStatus,
hasReadActionsPrivileges,
}: GetColumns): RulesColumns[] => {
const cols: RulesColumns[] = [
Expand Down Expand Up @@ -279,6 +284,7 @@ export const getColumns = ({
dispatchToaster,
history,
reFetchRules,
refetchPrePackagedRulesStatus,
hasReadActionsPrivileges
),
width: '40px',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { patchRule } from '../../../../../containers/detection_engine/rules/api'
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const MyEuiBasicTable = styled(EuiBasicTable as any)`` as any;

export type Func = () => void;
export type Func = () => Promise<void>;
export interface ExceptionListFilter {
name?: string | null;
list_id?: string | null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ interface AllRulesProps {
hasNoPermissions: boolean;
loading: boolean;
loadingCreatePrePackagedRules: boolean;
refetchPrePackagedRulesStatus: () => void;
refetchPrePackagedRulesStatus: () => Promise<void>;
rulesCustomInstalled: number | null;
rulesInstalled: number | null;
rulesNotInstalled: number | null;
rulesNotUpdated: number | null;
setRefreshRulesData: (refreshRule: (refreshPrePackagedRule?: boolean) => void) => void;
setRefreshRulesData: (refreshRule: () => Promise<void>) => void;
}

export enum AllRulesTabs {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ const initialState: State = {
filter: '',
sortField: 'enabled',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
},
loadingRuleIds: [],
loadingRulesAction: null,
Expand Down Expand Up @@ -193,6 +196,9 @@ describe('allRulesReducer', () => {
filter: 'host.name:*',
sortField: 'enabled',
sortOrder: 'desc',
tags: [],
showCustomRules: false,
showElasticRules: false,
};
const { filterOptions, pagination } = reducer(initialState, {
type: 'updateFilterOptions',
Expand Down
Loading

0 comments on commit 2abbd80

Please sign in to comment.