Skip to content

Commit

Permalink
[Entity Analytics] Prevent multiple duplicated calls to index_status …
Browse files Browse the repository at this point in the history
…API on alert details page (elastic#176573)

## Summary

When checking if we are using the new or old risk engine, we use
`useIsNewRiskScoreModuleInstalled`, this uses the risk engine status
API. We were previously defaulting to returning false while the request
was loading, this would then cause us to make API calls for the old
indices index status, then the new ones once the API request comes back.

The fix is to add a loading state to the hook and wait for loading to
finish before proceeding, this seems to also prevent a few re-renders
which was triggering the hook multiple times, making it so we only call
the APIs once for each index (hosts and users)

**Steps to reproduce:** 

Open an alert, and expand details with network tab open, note how many
calls there are to the index_status API and to which indices.

### **Before, 14 API calls, most of which to legacy indices 😢** 
<img width="968" alt="Screenshot 2024-02-09 at 09 39 20"
src="https://github.com/elastic/kibana/assets/3315046/e3943ce2-5cf2-4034-a9fa-189015eccae8">

### **After, 2 API calls to the new indices 😍** 
<img width="716" alt="Screenshot 2024-02-09 at 09 39 53"
src="https://github.com/elastic/kibana/assets/3315046/596ff183-395b-434c-8cb0-a97a49fe067d">

---------

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
hop-dev and kibanamachine authored Feb 14, 2024
1 parent 7e3a9f8 commit c7c7bc2
Show file tree
Hide file tree
Showing 12 changed files with 102 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ import { useSearchStrategy } from '../../use_search_strategy';
jest.mock('../../use_search_strategy', () => ({
useSearchStrategy: jest.fn(),
}));

jest.mock('../../../../entity_analytics/api/hooks/use_risk_engine_status', () => ({
useIsNewRiskScoreModuleInstalled: jest
.fn()
.mockReturnValue({ isLoading: false, installed: true }),
}));
const mockUseSearchStrategy = useSearchStrategy as jest.Mock;
const mockSearch = jest.fn();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export const useUserRelatedHosts = ({
abort: skip,
});

const isNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled();
const { installed: isNewRiskScoreModuleInstalled, isLoading: riskScoreStatusLoading } =
useIsNewRiskScoreModuleInstalled();

const userRelatedHostsResponse = useMemo(
() => ({
Expand All @@ -76,10 +77,10 @@ export const useUserRelatedHosts = ({
);

useEffect(() => {
if (!skip) {
if (!skip && !riskScoreStatusLoading) {
search(userRelatedHostsRequest);
}
}, [userRelatedHostsRequest, search, skip]);
}, [userRelatedHostsRequest, search, skip, riskScoreStatusLoading]);

return userRelatedHostsResponse;
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ import { useSearchStrategy } from '../../use_search_strategy';
jest.mock('../../use_search_strategy', () => ({
useSearchStrategy: jest.fn(),
}));

jest.mock('../../../../entity_analytics/api/hooks/use_risk_engine_status', () => ({
useIsNewRiskScoreModuleInstalled: jest
.fn()
.mockReturnValue({ isLoading: false, installed: true }),
}));

const mockUseSearchStrategy = useSearchStrategy as jest.Mock;
const mockSearch = jest.fn();

Expand All @@ -30,7 +37,7 @@ const mockResult = {
loading: false,
};

describe('useUsersRelatedHosts', () => {
describe('useHostRelatedUsers', () => {
beforeEach(() => {
jest.clearAllMocks();
mockUseSearchStrategy.mockReturnValue({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ export const useHostRelatedUsers = ({
from,
skip = false,
}: UseHostRelatedUsersParam): UseHostRelatedUsersResult => {
const isNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled();
const { installed: isNewRiskScoreModuleInstalled, isLoading: riskScoreStatusLoading } =
useIsNewRiskScoreModuleInstalled();
const {
loading,
result: response,
Expand Down Expand Up @@ -75,10 +76,10 @@ export const useHostRelatedUsers = ({
);

useEffect(() => {
if (!skip) {
if (!skip && !riskScoreStatusLoading) {
search(hostRelatedUsersRequest);
}
}, [hostRelatedUsersRequest, search, skip]);
}, [hostRelatedUsersRequest, riskScoreStatusLoading, search, skip]);

return hostRelatedUsersResponse;
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,19 @@ export const useInvalidateRiskEngineStatusQuery = () => {
}, [queryClient]);
};

export const useIsNewRiskScoreModuleInstalled = () => {
const { data: riskEngineStatus } = useRiskEngineStatus();
interface RiskScoreModuleStatus {
isLoading: boolean;
installed?: boolean;
}

return riskEngineStatus?.isNewRiskScoreModuleInstalled ?? false;
export const useIsNewRiskScoreModuleInstalled = (): RiskScoreModuleStatus => {
const { data: riskEngineStatus, isLoading } = useRiskEngineStatus();

if (isLoading) {
return { isLoading: true };
}

return { isLoading: false, installed: !!riskEngineStatus?.isNewRiskScoreModuleInstalled };
};

export const useRiskEngineStatus = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import { useSearchStrategy } from '../../../common/containers/use_search_strateg
import { useAppToasts } from '../../../common/hooks/use_app_toasts';
import { useAppToastsMock } from '../../../common/hooks/use_app_toasts.mock';
import { useRiskScoreFeatureStatus } from './use_risk_score_feature_status';
import { useIsNewRiskScoreModuleInstalled } from './use_risk_engine_status';
import { RiskScoreEntity } from '../../../../common/search_strategy';

jest.mock('../../../common/containers/use_search_strategy', () => ({
useSearchStrategy: jest.fn(),
}));
Expand All @@ -25,12 +25,20 @@ jest.mock('../../../common/hooks/use_space_id', () => ({
jest.mock('../../../common/hooks/use_app_toasts');
jest.mock('./use_risk_score_feature_status');

jest.mock('./use_risk_engine_status');

const mockUseIsNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled as jest.Mock;
const mockUseRiskScoreFeatureStatus = useRiskScoreFeatureStatus as jest.Mock;
const mockUseSearchStrategy = useSearchStrategy as jest.Mock;
const mockSearch = jest.fn();

let appToastsMock: jest.Mocked<ReturnType<typeof useAppToastsMock.create>>;

const defaultRiskScoreModuleStatus = {
isLoading: false,
installed: false,
};

const defaultFeatureStatus = {
isLoading: false,
isDeprecated: false,
Expand Down Expand Up @@ -67,6 +75,7 @@ describe.each([RiskScoreEntity.host, RiskScoreEntity.user])(
(useAppToasts as jest.Mock).mockReturnValue(appToastsMock);
mockUseRiskScoreFeatureStatus.mockReturnValue(defaultFeatureStatus);
mockUseSearchStrategy.mockReturnValue(defaultSearchResponse);
mockUseIsNewRiskScoreModuleInstalled.mockReturnValue(defaultRiskScoreModuleStatus);
});

test('does not search if license is not valid', () => {
Expand Down Expand Up @@ -172,6 +181,8 @@ describe.each([RiskScoreEntity.host, RiskScoreEntity.user])(
renderHook(() => useRiskScore({ riskEntity }), {
wrapper: TestProviders,
});

expect(mockSearch).toHaveBeenCalledTimes(1);
expect(mockSearch).toHaveBeenCalledWith({
defaultIndex: [`ml_${riskEntity}_risk_score_latest_default`],
factoryQueryType: `${riskEntity}sRiskScore`,
Expand All @@ -180,6 +191,25 @@ describe.each([RiskScoreEntity.host, RiskScoreEntity.user])(
});
});

test('runs search with new index if feature is enabled and not deprecated and new module installed', () => {
mockUseIsNewRiskScoreModuleInstalled.mockReturnValue({
...defaultRiskScoreModuleStatus,
installed: true,
});

renderHook(() => useRiskScore({ riskEntity }), {
wrapper: TestProviders,
});

expect(mockSearch).toHaveBeenCalledTimes(1);
expect(mockSearch).toHaveBeenCalledWith({
defaultIndex: ['risk-score.risk-score-latest-default'],
factoryQueryType: `${riskEntity}sRiskScore`,
riskScoreEntity: riskEntity,
includeAlertsCount: false,
});
});

test('return result', async () => {
mockUseSearchStrategy.mockReturnValue({
...defaultSearchResponse,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,14 @@ export const useRiskScore = <T extends RiskScoreEntity.host | RiskScoreEntity.us
includeAlertsCount = false,
}: UseRiskScore<T>): RiskScoreState<T> => {
const spaceId = useSpaceId();
const isNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled();
const defaultIndex = spaceId
? riskEntity === RiskScoreEntity.host
? getHostRiskIndex(spaceId, onlyLatest, isNewRiskScoreModuleInstalled)
: getUserRiskIndex(spaceId, onlyLatest, isNewRiskScoreModuleInstalled)
: undefined;
const { installed: isNewRiskScoreModuleInstalled, isLoading: riskScoreStatusLoading } =
useIsNewRiskScoreModuleInstalled();
const defaultIndex =
spaceId && !riskScoreStatusLoading && isNewRiskScoreModuleInstalled !== undefined
? riskEntity === RiskScoreEntity.host
? getHostRiskIndex(spaceId, onlyLatest, isNewRiskScoreModuleInstalled)
: getUserRiskIndex(spaceId, onlyLatest, isNewRiskScoreModuleInstalled)
: undefined;
const factoryQueryType =
riskEntity === RiskScoreEntity.host ? RiskQueries.hostsRiskScore : RiskQueries.usersRiskScore;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,14 @@ export const useRiskScoreKpi = ({
}: UseRiskScoreKpiProps): RiskScoreKpi => {
const { addError } = useAppToasts();
const spaceId = useSpaceId();
const isNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled();
const defaultIndex = spaceId
? riskEntity === RiskScoreEntity.host
? getHostRiskIndex(spaceId, true, isNewRiskScoreModuleInstalled)
: getUserRiskIndex(spaceId, true, isNewRiskScoreModuleInstalled)
: undefined;
const { installed: isNewRiskScoreModuleInstalled, isLoading: riskScoreStatusLoading } =
useIsNewRiskScoreModuleInstalled();
const defaultIndex =
spaceId && !riskScoreStatusLoading && isNewRiskScoreModuleInstalled !== undefined
? riskEntity === RiskScoreEntity.host
? getHostRiskIndex(spaceId, true, isNewRiskScoreModuleInstalled)
: getUserRiskIndex(spaceId, true, isNewRiskScoreModuleInstalled)
: undefined;

const {
isDeprecated,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ import { useSearchStrategy } from '../../../../common/containers/use_search_stra
jest.mock('../../../../common/containers/use_search_strategy', () => ({
useSearchStrategy: jest.fn(),
}));

jest.mock('../../../../entity_analytics/api/hooks/use_risk_engine_status', () => ({
useIsNewRiskScoreModuleInstalled: jest
.fn()
.mockReturnValue({ isLoading: false, installed: true }),
}));
const mockUseSearchStrategy = useSearchStrategy as jest.Mock;
const mockSearch = jest.fn();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ export const useAllHost = ({
getHostsSelector(state, type)
);

const isNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled();
const { installed: isNewRiskScoreModuleInstalled, isLoading: riskScoreStatusLoading } =
useIsNewRiskScoreModuleInstalled();

const [hostsRequest, setHostRequest] = useState<HostsRequestOptionsInput | null>(null);

Expand Down Expand Up @@ -129,6 +130,9 @@ export const useAllHost = ({
);

useEffect(() => {
if (riskScoreStatusLoading) {
return;
}
setHostRequest((prevRequest) => {
const myRequest: HostsRequestOptionsInput = {
...(prevRequest ?? {}),
Expand Down Expand Up @@ -162,6 +166,7 @@ export const useAllHost = ({
startDate,
sortField,
isNewRiskScoreModuleInstalled,
riskScoreStatusLoading,
]);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import { UsersType } from '../../store/model';

jest.mock('../../../../common/containers/query_toggle');
jest.mock('../../../../common/lib/kibana');
jest.mock('../../../../entity_analytics/api/hooks/use_risk_engine_status', () => ({
useIsNewRiskScoreModuleInstalled: jest
.fn()
.mockReturnValue({ isLoading: false, installed: true }),
}));

const mockSearch = jest.fn();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ export const AllUsersQueryTabBody = ({

const getUsersSelector = useMemo(() => usersSelectors.allUsersSelector(), []);
const { activePage, limit, sort } = useDeepEqualSelector((state) => getUsersSelector(state));
const isNewRiskScoreModuleInstalled = useIsNewRiskScoreModuleInstalled();
const { installed: isNewRiskScoreModuleInstalled, isLoading: riskScoreStatusLoading } =
useIsNewRiskScoreModuleInstalled();

const {
loading,
Expand All @@ -67,7 +68,7 @@ export const AllUsersQueryTabBody = ({
});

useEffect(() => {
if (!querySkip) {
if (!querySkip && !riskScoreStatusLoading) {
search({
filterQuery,
defaultIndex: indexNames,
Expand All @@ -92,6 +93,7 @@ export const AllUsersQueryTabBody = ({
limit,
sort,
isNewRiskScoreModuleInstalled,
riskScoreStatusLoading,
]);

return (
Expand Down

0 comments on commit c7c7bc2

Please sign in to comment.