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

[TSVB] Custom renderer #83554

Merged
merged 22 commits into from
Nov 23, 2020
Merged

[TSVB] Custom renderer #83554

merged 22 commits into from
Nov 23, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Nov 17, 2020

Summary

Part of #46801

  • Implement own renderer for TSVB visualization;

  • remove NoDataComponent, changed to use common for all visualizations VisualizationNoResults component;

  • lazy load the timeseries visualization component -> shrink initial bundle size, split with more chunks - for the visualization and the editor itself:

    1. dashboard mode -> tsvb panel without data -> visualization is not loaded

    image

    1. extend timerange to show data -> additional visualization's chunks loaded

    image

    1. edit the visualization -> the last and the heaviest chunk loaded

    image

  • Implement toExpressionAst function for building pipeline;

  • cleanup the code, remove extra wrappers, convert to typescript

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@@ -267,7 +267,6 @@ export const visPayloadSchema = schema.object({
})
),
}),
savedObjectId: schema.maybe(schema.string()),
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 found any usage of this param, removed as legacy

@sulemanof sulemanof mentioned this pull request Nov 18, 2020
13 tasks
@sulemanof sulemanof added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0 Feature:TSVB TSVB (Time Series Visual Builder) labels Nov 18, 2020
*/

// @ts-expect-error
import { TimeseriesVisualization as timeseries } from './timeseries/vis';
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about lazy loading of these components? I just think if the user sees only one type on the dashboard, why do we need to load all

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 a good idea. I tried to implement this approach, but noticed that it creates a lot of small chunks.
So I have a concern, what is a better approach here..
You could take a look at 2 screenshots below:

  • lazy load each visualization type, present couple of them in dashboard mode -> 13 small sized chunks for 4 vis types:

    image

  • current approach -> 2 chunks, but they are not so huge also

    image

So I wonder, is it worthy to do such a split?.. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@stratoula what do you think? I like idea with small chunks cause not sure that users use all tabs of TSVB on dashboard. But I'm ok with both solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea too 🙂 I don't think that they load each TSVB chart type on a dashboard too. But as Daniil said the two chunks are not so big so I don't have strong objections here

Copy link
Contributor

Choose a reason for hiding this comment

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

@sulemanof please do what you think is right =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexwizp @stratoula I'll pay more attention at the loading time of both variants in that case, and will post a comparison )

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great

@sulemanof sulemanof marked this pull request as ready for review November 19, 2020 16:07
@sulemanof sulemanof requested a review from a team November 19, 2020 16:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! thank you

filters?: any;
state?: any;
query?: any;
timerange: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Well done for anys removal 👏

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, tested it locally on Safari, no regressions found. This seems to be our last renderer! Conrats @sulemanof, great job 💯

@sulemanof
Copy link
Contributor Author

@stratoula @alexwizp
I did a non-deep analysis of loading time between both variants and concluded there is no critical difference between those in the current frame of loading the tsvb app.
But when the other steps of optimizing will be done, I believe we could save more.
I created an issue to track those optimizations #84063

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 469 470 +1

Async chunks

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

id before after diff
visTypeTimeseries 1.7MB 1.8MB +83.9KB

Distributable file count

id before after diff
default 43023 43056 +33
oss 22550 22583 +33

Page load bundle

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

id before after diff
visTypeTimeseries 135.0KB 137.1KB +2.1KB
visualizations 169.9KB 169.0KB -946.0B
total +1.2KB
Unknown metric groups

async chunk count

id before after diff
visTypeTimeseries 1 12 +11

History

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

@sulemanof sulemanof merged commit bb023c5 into elastic:master Nov 23, 2020
@sulemanof sulemanof deleted the feat/tsvb_renderer branch November 23, 2020 16:41
sulemanof pushed a commit to sulemanof/kibana that referenced this pull request Nov 23, 2020
* Implement custom renderer

* Remove legacy code

* Use custom expression

* Convert to typescript

* Remove savedObjectId extra param

* Other updates

* Fix types

* Cleanup

* Fix functional tests

* Bind uiSettings

* Update snapshot

* Update types

* Remove extra params

* Move common types

* Return back validation error message

* Use panel types enum

* Fix types

* Lazy load visualizations
sulemanof pushed a commit that referenced this pull request Nov 23, 2020
* Implement custom renderer

* Remove legacy code

* Use custom expression

* Convert to typescript

* Remove savedObjectId extra param

* Other updates

* Fix types

* Cleanup

* Fix functional tests

* Bind uiSettings

* Update snapshot

* Update types

* Remove extra params

* Move common types

* Return back validation error message

* Use panel types enum

* Fix types

* Lazy load visualizations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants