[Dashboard] [Controls] Improve async controls flyout behaviour

[Heenawter]

Closes #153572

Summary

This PR makes the calculation of the controls field registry truly async so that it can complete much faster - it also adds a better loading state to the field picker using the useAsync hook as a way to determine when the promise for the field registry has actually returned.

Previously, even though the function was marked as async and awaited, all of the internal logic was synchronous which meant that it always took O(3 * n) (where n is the number of fields, and 3 comes from the number of factories that are currently available for controls). In other words, if there are 10,000 fields in a given data view, the loop to generate the field registry would require 30,000 iterations.

This was such an intensive process that, once the calculation started, everything was completely consumed by it and Kibana would appear "frozen" - even the animation to open the flyout couldn't be completed, which is why it appeared to hang for so long; in fact, the animation could even get stuck in a "halfway open" state if there was a slight delay before the field registry calculation started.

Opening Flyout

• Before

  Screen.Recording.2023-03-30.at.2.13.56.PM.mov

• After

  Screen.Recording.2023-03-30.at.2.14.47.PM.mov

Changing Data View

• Before

  Screen.Recording.2023-03-30.at.2.19.58.PM.mov

• After

  Screen.Recording.2023-03-30.at.2.19.09.PM.mov

How to Test

1. Load a large dataset with a large number of fields into Kibana - for example, you can run the following (which assumes that your Kibana instance is running on port 5601 and your ES server is running on port 9200):
   
   

   node scripts/es_archiver load test/functional/fixtures/es_archiver/large_fields --es-url=http://elastic:changeme@localhost:9200/ --kibana-url=http://elastic:changeme@localhost:5601/
   

   If you used the above technique to load the data, you'll also need to add a data view for that index.
2. Make this new large_fields data view your default data view
3. Create a new dashboard and add a control from the new large_fields data view
4. The flyout should open quickly despite the fact that loading all the fields might take a little while

Checklist

• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[Heenawter]

All I did here was remove the outer <> ... </> to remove an unnecessary layer 😆

[Heenawter]

I'm applying the height to the EuiSelectable via the fieldPickerSelectable class instead - this makes it so that switching to and from the loading state does not cause layout shifting:

Before	After

Not sure if this is actually preferable? I'm torn 🤷 I tried to make the rounded box fill the space for the loading message, but it causes weird issues with the field type filter box... Not sure what's going on there exactly, but if we would prefer this, I can continue to fight with it :) Just didn't want to hold up the PR for such a small thing, especially if we decide to go with the "before" behaviour anyway.

cc @elastic/kibana-design

[andreadelrio]

@Heenawter no layout shifting is definitely preferred. I suggest you make the height of the "Loading" container the same as that of the field list. Then you can vertically center the Loading options + spinner elements. I think that'll achieve an even cleaner UI.

[Heenawter]

Done in 8e852cb 💃

[ThomThomson]

That looks so much better!

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[ThomThomson]

This is looking / working so much better. Ran this locally with the 10k fields and looked through the code.

The only comment I have is that I think creating a list of promises for each field and using promise.all incurs some slight overhead.

I did some quick tests with a single top-level promise instead and was able to take the time to get the 10k fields from roughly 75ms to roughly 20ms. This isn't a huge performance improvement or anything, but I think it also results in cleaner code.

loadFieldRegistryFromDataView would look like:

const loadFieldRegistryFromDataView = async (
  dataView: DataView
): Promise<DataControlFieldRegistry> => {
  const {
    controls: { getControlTypes, getControlFactory },
  } = pluginServices.getServices();

  const controlFactories = getControlTypes().map(
    (controlType) => getControlFactory(controlType) as IEditableControlFactory
  );
  const fieldRegistry: DataControlFieldRegistry = {};
  return new Promise<DataControlFieldRegistry>((resolve) => {
    dataView.fields.getAll().map((field) => {
      const compatibleControlTypes = [];
      for (const factory of controlFactories) {
        if (factory.isFieldCompatible && factory.isFieldCompatible(field)) {
          compatibleControlTypes.push(factory.type);
        }
      }
      if (compatibleControlTypes.length > 0) {
        fieldRegistry[field.name] = { field, compatibleControlTypes };
      }
    });
    resolve(fieldRegistry);
  });
};

[ThomThomson]

Nice cleanup!

[nreese]

One other performance consideration is to use for loop for iterating through large collections. Each time map is run, a new function is created and this creates a large garbage collection penalty.

[ThomThomson]

True! I bet we could increase this performance even further, cause the code snipped that I suggested also uses map unnecessarily.

[Heenawter]

Changed to forEach in 9d29b33

Changed to for loop in afb1718

[Heenawter]

@ThomThomson Good point - that's a lot cleaner, I agree :) Done in 9d29b33

[ThomThomson]

Thanks for implementing the suggestion! Re-reviewed and looked over the code again, everything LGTM!

[nreese]

for ... of is not much better. How about an imperative for loop, the one with the iterator for(i=0; i<length;i++)

[Heenawter]

I did some performance testing of all 3 options:

1. Using .forEach with a callback - took 11.64ms on average over 5 tries
2. Using for ... of for loop - took 12.52ms on average over 5 tries
3. Using for(let i=0....) imperative for loop - took 12.80ms on average over 5 tries.

So it seems like the imperative for loop is actually the worst of the three, although the differences here are so minimal I'm sure all three options would average out to be more-or-less equivalent over 100+ tests instead of 5 (for example, in my tests for the imperative for loop there was one run that randomly took 17.9ms - doing more iterations would eliminate this variance).

I'm wondering if it is worth going this deep over such minimal improvements (~1ms for 10,000 fields), though? I'm in favour of sticking with readability in a situation like this, and I personally vote for the for ... of loop in this case because it matches the style of the internal factory loop. 🤷

[nreese]

right, looking at https://leanylabs.com/blog/js-forEach-map-reduce-vs-for-for_of/ for..in and a traditional for loop have about the same performance, with for..in having slightly better performance.

[Heenawter]

Was just going to link the same article 😆 Awesome, thanks!

[ThomThomson]

I was reading that one too hahaha.

[nreese]

LGTM

[andreadelrio]

Design changes LGTM. Nice work @Heenawter 🎉

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: c9be3a2

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
controls	153	156	+3

Async chunks

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

id	before	after	diff
controls	189.4KB	190.4KB	+1002.0B
presentationUtil	129.1KB	129.6KB	+520.0B
total			+1.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
controls	11	10	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
controls	33.3KB	33.1KB	-132.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
securitySolution	432	435	+3

Total ESLint disabled count

id	before	after	diff
securitySolution	512	515	+3

History

• 💚 Build #117459 succeeded afb1718
• 💚 Build #117406 succeeded 1622dd4
• 💚 Build #116944 succeeded 3b1d23a
• 💚 Build #116874 succeeded 2ca669b
• 💚 Build #116812 succeeded 4a18b61
• 💔 Build #116591 failed af4f50c37fbd5ac12e87328f4cee3157ce358547

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

cc @Heenawter

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.7	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 154004

Questions ?

Please refer to the Backport tool documentation

[Heenawter]

💚 All backports created successfully

Status	Branch	Result
✅	8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation