[TSVB] Show the loading spinner while loading in dashboard #114244

dej611 · 2021-10-07T09:40:48Z

Summary

I think this is a partial fix for the related issue as it shows a Loading animation while loading the timeseries component.
Couldn't manage to find out where is the component which is missing the other Loading animation in the process: notice the initial time window, before the Gauge loads where the time series has no animation yet.

With the fix (I've exaggerated the loading time for time series to 10s to better record it):

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…o be loaded

elasticmachine · 2021-10-07T12:35:44Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

dej611 · 2021-10-11T08:26:45Z

@elasticmachine merge upstream

flash1293

I looked at the loading behavior by creating a dashboard, going to the listing page, reloading, then delaying each request by 5s and loading the dashboard. This is what happens in order (omitted irrelevant parts but we should really work on avoiding dependent requests... separate story though):

1 loading visualization embeddable (dashboard shows a spinner for this time)
2 loading TSVB data (no spinner shown during this time)
3 loading some other async TSVB chunks (no spinner shown during this time)
4 loading the vis component (spinner shown)
5 chart shown

I think to cover 3 we need to add the loading fallback here as well - something we could do on this PR as well (or maybe we can collapse the async chunks here into a single one? didn't check it though):

kibana/src/plugins/vis_types/timeseries/public/timeseries_vis_renderer.tsx

Line 71 in 60500c2

<TimeseriesVisualization

To cover 2, we probably need to move one layer up and render the loading icon once here (other charts will profit as well):

kibana/src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

Lines 305 to 306 in 5f994b1

	super.render(this.domNode);

Then let the expression loader take over the dom node once it's ready

flash1293 · 2021-10-12T07:15:16Z

x-pack/plugins/lens/public/metric_visualization/expression.test.tsx

@@ -90,7 +90,9 @@ describe('metric_expression', () => {
          reportDescription="Fancy chart description"
          reportTitle="My fanci metric chart"
        >
-          <AutoScale>


Seems like the Lens changes ended up in here on accident, do you want to remove them?

dej611 · 2021-10-12T17:19:22Z

I think to cover 3 we need to add the loading fallback here as well - something we could do on this PR as well (or maybe we can collapse the async chunks here into a single one? didn't check it though):

But there's already one there: the VisualizationContainer already contains the loading spinner with Suspense in it.
It seems the VisualizationContainer Suspense is triggered for the first panel and then other are queued like serially (or maybe it's just a reflection of the different size of each plugin)

dej611 · 2021-10-12T17:19:43Z

@elasticmachine merge upstream

flash1293 · 2021-10-13T10:37:35Z

Hm, right, maybe it's the scss import which isn't "suspended"?

kibana/src/plugins/vis_types/timeseries/public/timeseries_vis_renderer.tsx

Line 48 in 60500c2

await import('./application/index.scss');

Would it make sense to move it one layer deeper?

dej611 · 2021-10-13T13:46:54Z

It seems that fixed it 👍

dej611 · 2021-10-13T15:22:25Z

@elasticmachine merge upstream

dej611 · 2021-10-14T12:24:23Z

@elasticmachine merge upstream

dej611 · 2021-10-18T09:11:44Z

@elasticmachine merge upstream

flash1293 · 2021-10-18T09:58:51Z

This is definitely an improvement (also notice the smaller async bundle size!) but it seems like there is still one stage of async-ness missing:

I think the last one is missing here:

kibana/src/plugins/vis_types/timeseries/public/timeseries_vis_renderer.tsx

Line 54 in 6ee8270

const [palettesService, { indexPattern }] = await Promise.all([

While it's resolving this Promise.all, it's not showing a spinner. Could you fix this one as well? I think then it's handled cleanly.

dej611 · 2021-10-18T16:53:14Z

This is definitely an improvement (also notice the smaller async bundle size!) but it seems like there is still one stage of async-ness missing:

I think the last one is missing here:

kibana/src/plugins/vis_types/timeseries/public/timeseries_vis_renderer.tsx

Line 54 in 6ee8270

const [palettesService, { indexPattern }] = await Promise.all([

While it's resolving this Promise.all, it's not showing a spinner. Could you fix this one as well? I think then it's handled cleanly.

I've recorded the timings and various phases of the loading (also added a loader fallback for the case you've pointed), but it looks like the biggest gap happens between the visualize_embeddable.ts and the render.ts plugin render call, which is not handled with the Load Spinner component.

Here's a static picture of the logs with some annotations:

Some of those waiting are due to expression data loading upfront, which I suspect they are deferred in the Lens embeddable, for instance, is that right?

The point you want to address is the following, which is relatively fast (few ms on a throttled connection):

{now: 1634575325649, status: '[timeseries_vis_renderer.tsx] Loading', type: 'timeseries'}
{now: 1634575325651, status: '[timeseries_vis_renderer.tsx] Loaded', type: 'timeseries'}

flash1293 · 2021-10-18T17:35:01Z

It would be this place, right?

kibana/src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

Lines 305 to 306 in 5f994b1

	super.render(this.domNode);

Could we have spinner there as well? I think it’s worth solving this for all layers, there shouldn’t be so much jumping around when loading the page.

dej611 · 2021-10-19T17:16:39Z

Now the loading phase should be covered by the loader spinner:

dej611 · 2021-10-20T10:08:13Z

src/plugins/vis_types/timeseries/public/timeseries_vis_renderer.tsx

-          <TimeseriesVisualization
-            // it is mandatory to bind uiSettings because of "this" usage inside "get" method
-            getConfig={uiSettings.get.bind(uiSettings)}
+      render(


should it be unmounted here before render the proper one?

I'm not sure how important it is, but it should be fairly easy to unmount correctly - just call ReactDOM. unmountComponentAtNode(element) in these places.

dej611 · 2021-10-20T10:51:18Z

@flash1293 do you think it's worth adding 6kb to the visTypeTimeseries bundle size limit to avoid an async chunk round trip?
I can update the metrics config if you're ok with it.

dej611 · 2021-10-20T12:50:02Z

Ok, from the previous message I thought it was only a 6kb change, but 98kb it's another story. Will resolve it 😅

dej611 · 2021-10-20T12:58:03Z

Reversed last commit with the async chunk removal. Will track the issue in another PR.

kibanamachine · 2021-10-20T15:17:42Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`visTypeTimeseries`	488.4KB	485.0KB	-3.4KB
`visualizations`	72.8KB	73.0KB	+170.0B
total			-3.2KB

Page load bundle

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

id	before	after	diff
`visTypeTimeseries`	14.7KB	14.8KB	+106.0B
`visualizations`	36.1KB	36.1KB	+11.0B
total			+117.0B

Unknown metric groups

async chunk count

id	before	after	diff
`visTypeTimeseries`	12	11	-1

History

💔 Build #163159 failed e8870d4
💚 Build #162854 succeeded f9c64dc
💚 Build #161918 succeeded b56880e
💚 Build #160875 succeeded 7bcabbd
💚 Build #160535 succeeded 7df5e7e

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

flash1293

Works perfectly, thanks. Left a comment about unmounting the various loading spinners

flash1293 · 2021-10-21T10:50:02Z

src/plugins/vis_types/timeseries/public/timeseries_vis_renderer.tsx

-          <TimeseriesVisualization
-            // it is mandatory to bind uiSettings because of "this" usage inside "get" method
-            getConfig={uiSettings.get.bind(uiSettings)}
+      render(


I'm not sure how important it is, but it should be fairly easy to unmount correctly - just call ReactDOM. unmountComponentAtNode(element) in these places.

dej611 · 2021-10-25T08:49:05Z

@elasticmachine merge upstream

kibanamachine · 2021-10-25T11:00:01Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`visTypeTimeseries`	488.7KB	485.3KB	-3.4KB
`visualizations`	72.8KB	73.0KB	+170.0B
total			-3.2KB

Page load bundle

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

id	before	after	diff
`visTypeTimeseries`	14.7KB	14.8KB	+142.0B
`visualizations`	36.1KB	36.1KB	+11.0B
total			+153.0B

Unknown metric groups

async chunk count

id	before	after	diff
`visTypeTimeseries`	12	11	-1

History

💔 Build #1017 failed c9da308
💔 Build #919 failed 4817c6a

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

…14244) * 🐛 Fix metric rescale * 📸 Restored old snapshots * 🐛 Extend the fix to all scenarios * 📸 Refresh snapshots for new fix * 💄 Add suspense with loading spinner while waiting for the component to be loaded * 🐛 Move the styling one level deeper to avoid loading gaps * 🐛 show loader since the beginning and speed up a bit embeddable load * 💄 Show the loader while waiting for the palette * ⚡ Remove one chunk from TSVB async series * 🐛 Restore previous async chunk * 👌 Integrate feedback * 🚨 Fix linting issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-10-25T12:32:43Z

💚 Backport successful

Status	Branch	Result
✅	7.16

This backport PR will be merged automatically after passing CI.

…116122) * 🐛 Fix metric rescale * 📸 Restored old snapshots * 🐛 Extend the fix to all scenarios * 📸 Refresh snapshots for new fix * 💄 Add suspense with loading spinner while waiting for the component to be loaded * 🐛 Move the styling one level deeper to avoid loading gaps * 🐛 show loader since the beginning and speed up a bit embeddable load * 💄 Show the loader while waiting for the palette * ⚡ Remove one chunk from TSVB async series * 🐛 Restore previous async chunk * 👌 Integrate feedback * 🚨 Fix linting issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

dej611 added 6 commits October 5, 2021 17:22

🐛 Fix metric rescale

90a2f43

📸 Restored old snapshots

ef7f55f

🐛 Extend the fix to all scenarios

a011b10

📸 Refresh snapshots for new fix

0e41714

Merge remote-tracking branch 'upstream/master' into fix/104574

81006d7

💄 Add suspense with loading spinner while waiting for the component t…

6feda55

…o be loaded

dej611 marked this pull request as ready for review October 7, 2021 12:35

dej611 requested a review from a team as a code owner October 7, 2021 12:35

Merge branch 'master' into fix/105678

8bd940f

flash1293 reviewed Oct 12, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/105678

87960b0

Merge branch 'master' into fix/105678

60d99cf

dej611 added 2 commits October 13, 2021 15:41

Merge branch 'fix/105678' of github.com:dej611/kibana into fix/105678

3659936

🐛 Move the styling one level deeper to avoid loading gaps

6ee8270

Merge branch 'master' into fix/105678

7df5e7e

Merge branch 'master' into fix/105678

7bcabbd

Merge branch 'master' into fix/105678

b56880e

dej611 added 3 commits October 19, 2021 15:52

Merge remote-tracking branch 'upstream/master' into fix/105678

1dbc762

🐛 show loader since the beginning and speed up a bit embeddable load

c20b34c

💄 Show the loader while waiting for the palette

f9c64dc

⚡ Remove one chunk from TSVB async series

e8870d4

dej611 commented Oct 20, 2021

View reviewed changes

🐛 Restore previous async chunk

584aaae

Merge remote-tracking branch 'upstream/master' into fix/105678

f1b2dd2

flash1293 approved these changes Oct 21, 2021

View reviewed changes

dej611 and others added 2 commits October 21, 2021 15:00

👌 Integrate feedback

4817c6a

🚨 Fix linting issue

c9da308

Merge branch 'master' into fix/105678

3d4f1e9

dej611 merged commit 560cb6f into elastic:master Oct 25, 2021

kibanamachine mentioned this pull request Oct 25, 2021

[7.16] [TSVB] Show the loading spinner while loading in dashboard (#114244) #116122

Merged

DianaDerevyankina mentioned this pull request Nov 3, 2021

[TSVB] Fix reappearing of hidden series on refresh and styles loading #117311

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TSVB] Show the loading spinner while loading in dashboard #114244

[TSVB] Show the loading spinner while loading in dashboard #114244

dej611 commented Oct 7, 2021

elasticmachine commented Oct 7, 2021

dej611 commented Oct 11, 2021

flash1293 left a comment •

edited

Loading

flash1293 Oct 12, 2021

dej611 commented Oct 12, 2021 •

edited

Loading

dej611 commented Oct 12, 2021

flash1293 commented Oct 13, 2021

dej611 commented Oct 13, 2021

dej611 commented Oct 13, 2021

dej611 commented Oct 14, 2021

dej611 commented Oct 18, 2021

flash1293 commented Oct 18, 2021

dej611 commented Oct 18, 2021

flash1293 commented Oct 18, 2021

dej611 commented Oct 19, 2021

dej611 Oct 20, 2021

flash1293 Oct 21, 2021

dej611 commented Oct 20, 2021

dej611 commented Oct 20, 2021

dej611 commented Oct 20, 2021

kibanamachine commented Oct 20, 2021

async chunk count

flash1293 left a comment

flash1293 Oct 21, 2021

dej611 commented Oct 25, 2021

kibanamachine commented Oct 25, 2021

async chunk count

kibanamachine commented Oct 25, 2021

[TSVB] Show the loading spinner while loading in dashboard #114244

[TSVB] Show the loading spinner while loading in dashboard #114244

Conversation

dej611 commented Oct 7, 2021

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Oct 7, 2021

dej611 commented Oct 11, 2021

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 Oct 12, 2021

Choose a reason for hiding this comment

dej611 commented Oct 12, 2021 • edited Loading

dej611 commented Oct 12, 2021

flash1293 commented Oct 13, 2021

dej611 commented Oct 13, 2021

dej611 commented Oct 13, 2021

dej611 commented Oct 14, 2021

dej611 commented Oct 18, 2021

flash1293 commented Oct 18, 2021

dej611 commented Oct 18, 2021

flash1293 commented Oct 18, 2021

dej611 commented Oct 19, 2021

dej611 Oct 20, 2021

Choose a reason for hiding this comment

flash1293 Oct 21, 2021

Choose a reason for hiding this comment

dej611 commented Oct 20, 2021

dej611 commented Oct 20, 2021

dej611 commented Oct 20, 2021

kibanamachine commented Oct 20, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Oct 21, 2021

Choose a reason for hiding this comment

dej611 commented Oct 25, 2021

kibanamachine commented Oct 25, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

kibanamachine commented Oct 25, 2021

💚 Backport successful

flash1293 left a comment •

edited

Loading

dej611 commented Oct 12, 2021 •

edited

Loading