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

[DataViews] Fix excessive resolve_index requests in create data view flyout #109500

Merged
merged 3 commits into from
Aug 23, 2021
Merged
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 @@ -9,6 +9,7 @@
import React, { useState, useEffect, useCallback, useRef } from 'react';
import { EuiTitle, EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiLoadingSpinner } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import memoizeOne from 'memoize-one';

import {
IndexPatternSpec,
Expand Down Expand Up @@ -105,12 +106,22 @@ const IndexPatternEditorFlyoutContentComponent = ({

const { getFields } = form;

const [{ title, allowHidden, type }] = useFormData<FormInternal>({ form });
// `useFormData` initially returns `undefined`,
// we override `undefined` with real default values from `schema`
// to get a stable reference to avoid hooks re-run and reduce number of excessive requests
const [
{
title = schema.title.defaultValue,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like internally useFormData has some before init state, where these all returns undefined even though default values are specified in the schema.

Initial switches between undefined and "" caused some redundant effects re-run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't work with our form lib before, so not sure if there is a better approach

Copy link
Contributor

Choose a reason for hiding this comment

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

I did notice this was happening. This seems like a bug with the form lib. What do you think @sebelga ?

👍 on finding a work around!

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't say it's a bug, it is more an unpleasant side effect on how the architecture and react hook lifecycle work. I looked into it while working on #109238 but decided to keep that PR focused. It is on my radar and will see what can be done.

For you information this is how the data flow works:

  1. You call the hook and ask for the value --> the field is not mounted yet so its value is undefined
  2. In a useEffect() inside <UseField /> the field mounts and say to the form: "I am here!" --> this updates the field value and the useFormData hook triggers an update that you receive.

You can pass any number of fields inside the schema (that might not actually exist in the form) but the source of truth of real fields rendered in the form is the DOM (JSX) --> so when a field mounts and say to the form it is there. This is why I don't rely on the schema to return defaultValue because the field might not exist.

For now, the consumer has to do the check manually:

if (title === undefined) {
  return;
}

I'll give it more thought to improve this unexpected behaviour 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

If resolving the behavior is more difficult than expected it would be good to document it.

allowHidden = schema.allowHidden.defaultValue,
type = schema.type.defaultValue,
},
] = useFormData<FormInternal>({ form });
const [isLoadingSources, setIsLoadingSources] = useState<boolean>(true);

const [timestampFieldOptions, setTimestampFieldOptions] = useState<TimestampOption[]>([]);
const [isLoadingTimestampFields, setIsLoadingTimestampFields] = useState<boolean>(false);
const [isLoadingMatchedIndices, setIsLoadingMatchedIndices] = useState<boolean>(false);
const currentLoadingMatchedIndicesRef = useRef(0);
const [allSources, setAllSources] = useState<MatchedItem[]>([]);
const [isLoadingIndexPatterns, setIsLoadingIndexPatterns] = useState<boolean>(true);
const [existingIndexPatterns, setExistingIndexPatterns] = useState<string[]>([]);
Expand Down Expand Up @@ -165,7 +176,9 @@ const IndexPatternEditorFlyoutContentComponent = ({
try {
const response = await http.get('/api/rollup/indices');
if (isMounted.current) {
setRollupIndicesCapabilities(response || {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re-assigning {} created a new object and caused effects re-run

Copy link
Contributor

Choose a reason for hiding this comment

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

oooph, good point!

if (response) {
setRollupIndicesCapabilities(response);
}
}
} catch (e) {
// Silently swallow failure responses such as expired trials
Expand Down Expand Up @@ -225,52 +238,31 @@ const IndexPatternEditorFlyoutContentComponent = ({
let newRollupIndexName: string | undefined;

const fetchIndices = async (query: string = '') => {
setIsLoadingMatchedIndices(true);
const indexRequests = [];

if (query?.endsWith('*')) {
const exactMatchedQuery = getIndices({
http,
isRollupIndex,
pattern: query,
showAllIndices: allowHidden,
searchClient,
});
indexRequests.push(exactMatchedQuery);
// provide default value when not making a request for the partialMatchQuery
indexRequests.push(Promise.resolve([]));
} else {
const exactMatchQuery = getIndices({
http,
isRollupIndex,
pattern: query,
showAllIndices: allowHidden,
searchClient,
});
const partialMatchQuery = getIndices({
http,
isRollupIndex,
pattern: `${query}*`,
showAllIndices: allowHidden,
searchClient,
});

indexRequests.push(exactMatchQuery);
indexRequests.push(partialMatchQuery);
}

const [exactMatched, partialMatched] = (await ensureMinimumTime(
indexRequests
)) as MatchedItem[][];
const currentLoadingMatchedIndicesIdx = ++currentLoadingMatchedIndicesRef.current;

const matchedIndicesResult = getMatchedIndices(
allSources,
partialMatched,
exactMatched,
allowHidden
);
setIsLoadingMatchedIndices(true);

if (isMounted.current) {
const { matchedIndicesResult, exactMatched } = !isLoadingSources
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we don't need to make a request while isLoadingSources didn't resolve. I short cut it

? await loadMatchedIndices(query, allowHidden, allSources, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to extract loadMatchedIndices into a separate function to wrap it into memoize-one. Otherwise, eslint-hooks complained that it can't resolve args.
I kept it in the current file to keep it simpler.

isRollupIndex,
http,
searchClient,
})
: {
matchedIndicesResult: {
exactMatchedIndices: [],
allIndices: [],
partialMatchedIndices: [],
visibleIndices: [],
},
exactMatched: [],
};

if (
currentLoadingMatchedIndicesIdx === currentLoadingMatchedIndicesRef.current &&
isMounted.current
) {
// we are still interested in this result
if (type === INDEX_PATTERN_TYPE.ROLLUP) {
const rollupIndices = exactMatched.filter((index) => isRollupIndex(index.name));
newRollupIndexName = rollupIndices.length === 1 ? rollupIndices[0].name : undefined;
Expand All @@ -288,7 +280,7 @@ const IndexPatternEditorFlyoutContentComponent = ({

return fetchIndices(newTitle);
},
[http, allowHidden, allSources, type, rollupIndicesCapabilities, searchClient]
[http, allowHidden, allSources, type, rollupIndicesCapabilities, searchClient, isLoadingSources]
);

useEffect(() => {
Expand Down Expand Up @@ -396,3 +388,76 @@ const IndexPatternEditorFlyoutContentComponent = ({
};

export const IndexPatternEditorFlyoutContent = React.memo(IndexPatternEditorFlyoutContentComponent);

// loadMatchedIndices is called both as an side effect inside of a parent component and the inside forms validation functions
// that are challenging to synchronize without a larger refactor
// Use memoizeOne as a caching layer to avoid excessive network requests on each key type
// TODO: refactor to remove `memoize` when https://github.com/elastic/kibana/pull/109238 is done
const loadMatchedIndices = memoizeOne(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

memoizeOne is the simplest way I can think of to share the state between validation and component side-effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking in refactoring the code code once #109238 is merged so I am not sure we need to optimise for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga, I've just tried to remove it and I get 3 network requests on each keystroke instead of 1 without it.
Do you think this quick trick doesn't worth it? I also planned to backport to at least 7.15 (looking at it like on a bug).
We can still refactor in 7.16+ when using your new API. wdyt?
I am open to reverting, just want to make sure we consider this ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool yes the trick is worth it and we can remove it later on 👍 Surprised it is 3 requests and not 2. But still worth it 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

The side effect inside the parent component

Mmmm. Can you point me to that code? The parent component also triggers a "fetchIndices()"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where fetch №2 happens:

The side effect inside the parent component

Copy link
Contributor

Choose a reason for hiding this comment

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

I see on L286

useEffect(() => {
  reloadMatchedIndices(title);
}, [allowHidden, reloadMatchedIndices, title]);

I am not sure I fully understand why we need this. I thought it was the validator inside the titleField that was triggering the reload. Why do we need also this effect?

Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand correctly, you call twice the reloadMatchedIndices? Once inside the validation (when title changes) and once in this useEffect again when title changes? It seems that the useEffect is redundant (it also has allowHidden dependency which should not be there).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(it also has allowHidden dependency which should not be there).

yes, I agree.

It seems that the useEffect is redundant

Hm, it is indeed might be redundant. I haven't tried to remove it

async (
query: string,
allowHidden: boolean,
allSources: MatchedItem[],
{
isRollupIndex,
http,
searchClient,
}: {
isRollupIndex: (index: string) => boolean;
http: IndexPatternEditorContext['http'];
searchClient: IndexPatternEditorContext['searchClient'];
}
): Promise<{
matchedIndicesResult: MatchedIndicesSet;
exactMatched: MatchedItem[];
partialMatched: MatchedItem[];
}> => {
const indexRequests = [];

if (query?.endsWith('*')) {
const exactMatchedQuery = getIndices({
http,
isRollupIndex,
pattern: query,
showAllIndices: allowHidden,
searchClient,
});
indexRequests.push(exactMatchedQuery);
// provide default value when not making a request for the partialMatchQuery
indexRequests.push(Promise.resolve([]));
} else {
const exactMatchQuery = getIndices({
http,
isRollupIndex,
pattern: query,
showAllIndices: allowHidden,
searchClient,
});
const partialMatchQuery = getIndices({
http,
isRollupIndex,
pattern: `${query}*`,
showAllIndices: allowHidden,
searchClient,
});

indexRequests.push(exactMatchQuery);
indexRequests.push(partialMatchQuery);
}

const [exactMatched, partialMatched] = (await ensureMinimumTime(
indexRequests
)) as MatchedItem[][];

const matchedIndicesResult = getMatchedIndices(
allSources,
partialMatched,
exactMatched,
allowHidden
);

return { matchedIndicesResult, exactMatched, partialMatched };
},
// compare only query and allowHidden
(newArgs, oldArgs) => newArgs[0] === oldArgs[0] && newArgs[1] === oldArgs[1]
);