[DataViews] Fix excessive `resolve_index` requests in create data view flyout by Dosant · Pull Request #109500 · elastic/kibana

Dosant · 2021-08-20T15:39:34Z

Summary

close #108854

Tried to reduce the number of network calls:

Opening the flyout and typing `abc`:

BEFORE

AFTER

There could be other edge cases

Dosant · 2021-08-20T15:40:38Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

+  // to get a stable reference to avoid hooks re-run and reduce number of excessive requests
+  const [
+    {
+      title = schema.title.defaultValue,


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

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

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!

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:

You call the hook and ask for the value --> the field is not mounted yet so its value is undefined

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 👍

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

Dosant · 2021-08-20T15:41:33Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

      try {
        const response = await http.get('/api/rollup/indices');
        if (isMounted.current) {
-          setRollupIndicesCapabilities(response || {});


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

oooph, good point!

Dosant · 2021-08-20T15:42:03Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx


      const fetchIndices = async (query: string = '') => {
+        const currentRequestRef = {};
+        currentLoadingMatchedIndicesRef.current = currentRequestRef;


I added this to avoid a race-condition where we can override more recent request with older response

Dosant · 2021-08-20T15:42:55Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

-
-        if (isMounted.current) {
+        const { matchedIndicesResult, exactMatched } = !isLoadingSources
+          ? await loadMatchedIndices(query, allowHidden, allSources, {


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.

Dosant · 2021-08-20T15:43:44Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

-        );
-
-        if (isMounted.current) {
+        const { matchedIndicesResult, exactMatched } = !isLoadingSources


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

Dosant · 2021-08-20T15:45:04Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

+// 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
+const loadMatchedIndices = memoizeOne(


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

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

You can see in the documentation I wrote how this will be implemented here

https://github.com/elastic/kibana/pull/109238/files#diff-4ca1882173722332a4f281526d2ac2e11e0ad7fbe3acbf88e1641d58ba859093R296

@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 ^^

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 😊

The side effect inside the parent component

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

This is where fetch №2 happens:

The side effect inside the parent component

kibana/src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

Line 295 in 0a8d42c

reloadMatchedIndices(title);

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?

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).

(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

elasticmachine · 2021-08-20T18:05:58Z

Pinging @elastic/kibana-app-services (Team:AppServices)

mattkime

changes look good and work well!

mattkime · 2021-08-22T02:39:03Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

+  // to get a stable reference to avoid hooks re-run and reduce number of excessive requests
+  const [
+    {
+      title = schema.title.defaultValue,


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!

mattkime · 2021-08-22T02:39:45Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

      try {
        const response = await http.get('/api/rollup/indices');
        if (isMounted.current) {
-          setRollupIndicesCapabilities(response || {});


oooph, good point!

sebelga

Thanks for working on this @Dosant ! I indeed saw those requests and there a few optimisation that the form lib could do to reduce them.

A long standing one to debounce the validation requests (#79607)
I just opened this one: add an option to indicate that a validation is asynchronous (#109628)

As I added in my comment I would revert the changes (memoizeOne()) you made to improve the indirection of loading the indices from the validation as #109238 will solve that problem and will make the code much easier to reason about.

sebelga · 2021-08-23T11:27:05Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

+// 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
+const loadMatchedIndices = memoizeOne(


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

sebelga · 2021-08-23T11:28:37Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

+// 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
+const loadMatchedIndices = memoizeOne(


You can see in the documentation I wrote how this will be implemented here

https://github.com/elastic/kibana/pull/109238/files#diff-4ca1882173722332a4f281526d2ac2e11e0ad7fbe3acbf88e1641d58ba859093R296

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

sebelga · 2021-08-23T11:33:02Z

src/plugins/index_pattern_editor/public/components/index_pattern_editor_flyout_content.tsx

      let newRollupIndexName: string | undefined;

      const fetchIndices = async (query: string = '') => {
+        const currentRequestRef = {};


I usually use a counter for this purpose and simply increment it.

const currentLoadingMatchedIndicesRef = useRef(0); ... const fetchIndices = async (query: string = '') => { const idx = ++currentLoadingMatchedIndicesRef.current; ... if (idx === currentLoadingMatchedIndicesRef.current) { // update states } }

looks good, easier to understand then the trick with ref to an object

sebelga

Happy to merge with your fix to reduce 3 requests to 1 and refactor once the new form lib API is merged. Great work! 👍

kibanamachine · 2021-08-23T16:15:29Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`indexPatternEditor`	106	107	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`indexPatternEditor`	174.0KB	175.7KB	+1.7KB

History

💚 Build #147390 succeeded 66c1641

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…w flyout (elastic#109500)

kibanamachine · 2021-08-23T16:39:41Z

💚 Backport successful

Status	Branch	Result
✅	7.15
✅	7.x

The backport PRs will be merged automatically after passing CI.

…w flyout (#109500) (#109700) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

…w flyout (#109500) (#109701) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

send less network requsts

66c1641

Dosant added Feature:Data Views Data Views code and UI - index patterns before 8.0 v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 20, 2021

Dosant commented Aug 20, 2021

View reviewed changes

Dosant marked this pull request as ready for review August 20, 2021 18:05

Dosant assigned mattkime and sebelga Aug 20, 2021

Dosant requested review from mattkime and sebelga August 20, 2021 18:07

Dosant unassigned mattkime and sebelga Aug 20, 2021

mattkime approved these changes Aug 22, 2021

View reviewed changes

sebelga reviewed Aug 23, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into dev/fix-resolve

d811b59

Dosant requested a review from sebelga August 23, 2021 12:37

review clean up

01d8c9a

sebelga approved these changes Aug 23, 2021

View reviewed changes

Dosant added auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Aug 23, 2021

Dosant merged commit bea4865 into elastic:master Aug 23, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 23, 2021

[DataViews] Fix excessive resolve_index requests in create data vie…

2b57cc5

…w flyout (elastic#109500)

kibanamachine mentioned this pull request Aug 23, 2021

[7.15] [DataViews] Fix excessive resolve_index requests in create data view flyout (#109500) #109700

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 23, 2021

[DataViews] Fix excessive resolve_index requests in create data vie…

23ff5a4

…w flyout (elastic#109500)

kibanamachine mentioned this pull request Aug 23, 2021

[7.x] [DataViews] Fix excessive resolve_index requests in create data view flyout (#109500) #109701

Merged

kibanamachine added a commit that referenced this pull request Aug 23, 2021

[DataViews] Fix excessive resolve_index requests in create data vie…

c8470b0

…w flyout (#109500) (#109700) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

kibanamachine added a commit that referenced this pull request Aug 23, 2021

[DataViews] Fix excessive resolve_index requests in create data vie…

e87ef74

…w flyout (#109500) (#109701) Co-authored-by: Anton Dosov <anton.dosov@elastic.co>

sebelga mentioned this pull request Aug 24, 2021

[Form lib] Allow dynamic data to be passed to validator functions #109238

Merged

Dosant added the bug Fixes for quality problems that affect the customer experience label Aug 24, 2021

Dosant mentioned this pull request Aug 25, 2021

[DataViews] Fix checking remote clusters in empty state #110054

Merged

Conversation

Dosant commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Opening the flyout and typing abc:

BEFORE

AFTER

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 20, 2021

Uh oh!

mattkime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 23, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Aug 23, 2021

Dosant commented Aug 20, 2021 •

edited

Loading

Opening the flyout and typing `abc`: