[rfc][skip-ci] Screenshot Mode Service by tsullivan · Pull Request #59084 · elastic/kibana

tsullivan · 2020-03-02T21:07:57Z

Summary

RFC Process: https://github.com/elastic/kibana/tree/master/rfcs

rfcs/text/0009_screenshot_mode_service.md

kibanamachine · 2020-03-03T00:51:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cfb9e91

History

💚 Build #30240 succeeded 06ab40e9878ae328b4ace2802025752206559a79
💚 Build #30206 succeeded d424d3d812530dc5d72092f095dc1ba85144f992

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

mattkime · 2020-03-03T15:06:04Z

I think this likely makes sense but I think the RFC assumes some knowledge of how reporting currently works and is used. What is the lifecycle of creating a pdf report? Start kibana, load url, save pdf, and exit?

It seems to me there are two benefits to this - simply hiding a UI from rendering and preventing unnecessary (and slow) work from occurring. Oh, there's a third, the popup case - where you need different behavior in the reporting context.

Its my understanding that the UI has some slow bits (such as the news feed) that could be easily disabled. Past that, I'd be curious to see some performance benchmarks to see where most of the time is being spent creating a report.

tsullivan · 2020-03-04T01:31:34Z

On Mar 3, 2020, at 8:06 AM, Matthew Kime ***@***.***> wrote: I think this likely makes sense but I think the RFC assumes some knowledge of how reporting currently works and is used. What is the lifecycle of creating a pdf report? Start kibana, load url, save pdf, and exit?

I think these really are important points that would help all Kibana devs understand why we want to do this, what we’re solving and the value it provides. Along with adding a quick summary of the screenshot capture flow, do you think it would help if we had a separate issue taking about the current state of performance with capturing a page? We can get data from Kibana and Puppeteer verbose logs and get some event timings, but that will take some time to put together.

mattkime · 2020-03-04T20:17:55Z

@tsullivan That sounds good.

I know those suggestions can mean more work so I leave it in your hands as to decide if and how you want to go about these things.

tsullivan · 2020-03-05T00:47:21Z

Thanks @mattkime. I'm going to work on a performance issue against Reporting, because I think it would help this RFC to have it as a reference. It will help readers understand how Reporting works, and where wasted time is happening when we capture a screenshot.

WIP issue: #59396

tsullivan · 2020-03-09T17:54:05Z

@elasticmachine merge upstream

rfcs/text/0009_screenshot_mode_service.md

kobelb · 2020-03-10T20:49:58Z

rfcs/text/0009_screenshot_mode_service.md

+
+There are a few ways for the service to obtain the data for its internal state.
+The most typical way would be to have a URL query string variable with the
+raw data and have the service read it when an API client is constructed. The


By "API client", are you referring to an instance of ScreenshotModeServiceApi described below?

Pretty much. I reworded this paragraph: c33de1b

Instead of "when the API client is constructed" I think I mean "in the setup method of the plugin" where it reads the URL and forms its interface data.

rfcs/text/0009_screenshot_mode_service.md

…to rfcs/screenshot_mode

brandon's grammer fix Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

…to rfcs/screenshot_mode

mshustov · 2020-03-13T14:08:24Z

rfcs/text/0009_screenshot_mode_service.md

+features. The solution to those downsides is to have Kibana pages themselves
+become more capable at presenting themselves for screenshot capture reports.
+With the Screenshot Mode Service available, Reporting could drop the
+task that it currently has which hurt performance: wasted rendering that is


According to your investigation.
there are 2 steps being performance bottlenecks in the PDF rendering:

open_url. which is hurt by bloated artifacts size.

get_number_of_items. I don't think we spend several seconds accessing DOM. Do we wait for data to be fetched on this step?

How would the current proposal improve them?

To improve the performance of open_url, we can have the applications make fewer requests on page load by disabling the interactive features that aren't needed for capturing screenshots.

To improve the performance of get_number_of_items (and wait_for_render), we'll look for ways to streamline the UX of the applications for screenshot capture mode to get them to be in a "ready" state sooner.

I don't think we spend several seconds accessing DOM. Do we wait for data to be fetched on this step?

We're waiting for the application to inject the meta data into the DOM, so the time we're spending here is waiting for the DOM selectors to yield a result.

mshustov · 2020-03-13T14:20:01Z

rfcs/text/0009_screenshot_mode_service.md

+screenshotRouter.get(
+  { path: '/my-endpoint' },
+  async (context, req, res) => {
+    const isScreenshotMode = context.screenshots.isScreenshotMode();


What is the expected result of this route handler? A fully rendered HTML page?

Perhaps, but the example is generic to show a way that the service can be used in a route.

If the route serves rendered HTML and the HTML can be streamlined for screenshot capture, then we can integrate it in the route handler and customize the payload for screenshot capture.

If the route serves JSON, and the JSON needs to be customized for screenshot capture, we could do the same thing. I'm not sure if we should, but it may be a solution for bugs such as #17950. That issue says: when the data table is captured as an image, there needs to be the complete set of rows shown in the table because the user can't paginate or scroll the table in a rasterized image. If the data table's rows were populated by XHR, I would think about customizing the response to show all the rows of the table to make for the complete data capture.

mshustov · 2020-03-13T14:29:07Z

rfcs/text/0009_screenshot_mode_service.md

+- Print media query CSS
+  Many things can be improved on capturing screenshots from the page using
+  `@print` CSS media query selecting. Also, CSS makes things hidden on the
+  page, but they are still loading in the DOM and take browser memory


Is it a really problem? AFAIK you can command a browser not to load styles using media attribute:

<link href="print.css" rel="stylesheet" media="print">

The reason we will use print media query CSS as a supplement is because we can't address page loading performance using just this solution. We need faster page loading performance than we have today to deliver the reports in a timely manner.

How can I better understand the performance issues we face here with Reporting where media queries aren't sufficient? As I understand it, any performance issues we have in Reporting are also issues every-day users experience (bundle size, etc). I am concerned that the RFC suggestion doesn't address the underlying issue, but would like to understand further.

@tylersmalley

My understanding is also that a lot of the problems happening in Reporting are issues every-day users face. I doubt that we can do anything about bundle sizes, even if this service existed. But I think we can find ways to decrease memory usage for non-interactive experiences. It would be wonderful if this helps reports run faster, but we're also still very concerned about stability and we think that streamlining where we can will help more reports complete without Chromium running out of memory.

Having this service in place would give us leverage to streamline more things. The background polling requests seem to be the primary negative factor on Reporting performance, in causing higher memory consumption. We're working with the idea that the plugins that do background polling can be adjusted for screenshot capture, where only the initial data should be needed.

We can't quantify how much of a hit that it's taking on performance because we don't have the tools to isolate that. The Screenshot Mode Service would be the tool that would let us tweak the screenshot capture experience. Being able to flip some switches would help us understand more about what is impacting performance, while we measure things happening with APM.

mshustov · 2020-03-13T14:31:23Z

rfcs/text/0009_screenshot_mode_service.md

+plugin to support custom endpoints include the screenshot data as part of the
+route handling context. Example:
+```
+const screenshotRouter = plugins.screenshots.wrapRouter(router);


What if we need to implement another mode? Dashboard mode, for example? Should plugin be aware of all of them?

I can't think of a reason why not.

mshustov · 2020-03-13T14:32:13Z

rfcs/text/0009_screenshot_mode_service.md

+
+```
+http://localhost:5601/app/kibana
+?screenshots=(enabled:!t,layout:(height:500,width:400),timezone:PST)


who is going to parse this query in OSS mode?

The Screenshot Mode Service will be a plugin in OSS Kibana.

mshustov · 2020-03-13T14:37:57Z

rfcs/text/0009_screenshot_mode_service.md

+  against the toast timeout window.
+- Avoid collection and sending of telemetry from the browser when page is
+  loaded for screenshot capture.
+- Turn off autocomplete features and auto-refresh features that weigh on


Can it be disabled by the Screenshot Mode Service explicitly via ui settings or plugin API?

The plan for what the Screenshot Mode Service will have the applications and plugins turn off their own features as needed for more optimal screen capture. We're purposefully not having the service do anything automatically - I'm not sure that would even be possible as the service is a plugin that applications have to explicitly integrate with.

tsullivan · 2020-03-20T18:19:00Z

Looks like I forgot to put this RFC into a final comment period, but this week was the final comment period.

I'm going to close this without merging and collaborate more with the Reporting team on a new RFC to replace this one. After some internal discussions, we found what we need the most is a way to distribute the application-specific rendering concerns away from the Reporting codebase and into the respective applications ownership. That notion is partially here, but it's obscured by performance topics. The performance problems are all over Kibana right now, and there are plans in New Platform to speed up page load time.

tsullivan force-pushed the rfcs/screenshot_mode branch 3 times, most recently from 07fb556 to 06ab40e Compare March 2, 2020 22:54

screenshot mode service rfc

cfb9e91

tsullivan force-pushed the rfcs/screenshot_mode branch from 06ab40e to cfb9e91 Compare March 2, 2020 23:04

tsullivan changed the title ~~screenshot mode service rfc~~ [rfc][skip-ci] Screenshot Mode Service Mar 2, 2020

tsullivan added the RFC label Mar 2, 2020

joelgriffith reviewed Mar 2, 2020

View reviewed changes

rfcs/text/0009_screenshot_mode_service.md Outdated Show resolved Hide resolved

joelgriffith reviewed Mar 2, 2020

View reviewed changes

rfcs/text/0009_screenshot_mode_service.md Outdated Show resolved Hide resolved

joelgriffith reviewed Mar 2, 2020

View reviewed changes

rfcs/text/0009_screenshot_mode_service.md Outdated Show resolved Hide resolved

tsullivan added 2 commits March 4, 2020 13:03

Merge branch 'master' into rfcs/screenshot_mode

7fe20d5

wording adjustments

f2b2265

add more info about why to do this and link to the reporting perf issue

427f087

tsullivan mentioned this pull request Mar 5, 2020

[Reporting] Improvements needed for screenshot capture flow #59396

Closed

tsullivan added 2 commits March 5, 2020 15:39

Merge branch 'master' into rfcs/screenshot_mode

99ed0af

remove a questionable barrier point

69d4da3

tsullivan marked this pull request as ready for review March 9, 2020 17:02

tsullivan added backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Mar 9, 2020

Merge branch 'master' into rfcs/screenshot_mode

25ea316

joshdover reviewed Mar 9, 2020

View reviewed changes

rfcs/text/0009_screenshot_mode_service.md Outdated Show resolved Hide resolved

kobelb reviewed Mar 10, 2020

View reviewed changes

Merge branch 'rfcs/screenshot_mode' of github.com:tsullivan/kibana in…

1913070

…to rfcs/screenshot_mode

tsullivan and others added 9 commits March 12, 2020 10:42

Merge branch 'master' into rfcs/screenshot_mode

f2d3edf

fix url param in url example

9f59e22

define it as a plugin, not a core service

1711ecc

Update rfcs/text/0009_screenshot_mode_service.md

a827610

brandon's grammer fix Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

brandon's spelling fix

b58ff10

Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

brandon's spelling fix

326dc88

Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

brandon's grammar fix

cfde7d5

Co-Authored-By: Brandon Kobel <brandon.kobel@gmail.com>

Merge branch 'rfcs/screenshot_mode' of github.com:tsullivan/kibana in…

7ea51b8

…to rfcs/screenshot_mode

clarification on service instantiation

c33de1b

mshustov reviewed Mar 13, 2020

View reviewed changes

tsullivan mentioned this pull request Mar 14, 2020

[Reporting] Catalog of Reporting Tech Debt #52927

Closed

tsullivan closed this Mar 20, 2020

tsullivan deleted the rfcs/screenshot_mode branch March 20, 2020 18:31

tsullivan mentioned this pull request Mar 3, 2021

[rfc][skip-ci] Screenshot Mode Service #93496

Merged

Conversation

tsullivan commented Mar 2, 2020

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Mar 3, 2020

💚 Build Succeeded

History

Uh oh!

mattkime commented Mar 3, 2020

Uh oh!

tsullivan commented Mar 4, 2020 via email

Uh oh!

mattkime commented Mar 4, 2020

Uh oh!

tsullivan commented Mar 5, 2020

Uh oh!

tsullivan commented Mar 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tylersmalley Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tsullivan commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tsullivan Mar 16, 2020 •

edited

Loading

tylersmalley Mar 16, 2020 •

edited

Loading

tsullivan Mar 17, 2020 •

edited

Loading