[rfc][skip-ci] Screenshot Mode Service

[tsullivan]

Summary

Re-open: #59084

Rendered mode: https://github.com/elastic/kibana/blob/f68c0d4/rfcs/text/0009_screenshot_mode_service.md

[elasticmachine]

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

[elasticmachine]

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

[ppisljar]

should we add a possibility of enabling this thru url parameter ? (for easier testing)

[streamich]

Whose responsibility would it be to hide the sidebar and topnav? Will this new plugin also do some UI tweaks like hiding sidenav and topnav? Or should the rendered app hide those? Or those don't need to be hidden for reporting?

[streamich]

Should we use this param not just for testing? It would be like a real alternative to the header.

[tsullivan]

I'm not sure there needs to be a lot of detail about how the client-side of plugin is aware that the page load request had a special header. Since there are a few options on how to do it, it comes down to internal implementation details which will get figured out during development.

I imagined there would be a server-side piece that acts like middleware to requests: runs on every request, keeps its own state, and is able to render the state into the DOM before the initial HTML is returned. The model would be like how the client-side UiSettings service works.

Looking at that now, there could be invalid assumptions about what a server-side plugin can do. I avoided proposing a URL parameter because my thinking was the URL query string is reserved for the app. If plugins are modifying it, it could create conflicts with the app. I could be wrong though.

[Dosant]

@tsullivan, is there any reason at all for it to be a header? Or could it be just a part of the URL? If it is just URL, then the whole setup seems simpler.

If plugins are modifying it, it could create conflicts with the app.

I think screenshotMode plugin could pick it up in setup and put the flag into memory. (I think this could be an exception to the agreement that plugins shouldn't mess with the URL and only apps can read/write the URL)
or
If screenshotMode is not a plugin, but part of core, then it could check and persist that query param before running any plugin, so no one would have a chance to mess it up.

[ppisljar]

i think we should think thru the internal implementation as well, once we agree on the public interface, which seems quite simple:

/***
* Call this method if you are a lower level plugin without an app and you want a quick way to disable yourself when kibana is in screenshot mode. Try to avoid using this, talk to app-services team when in doubt
***/
isScreenshotMode: () => boolean

so how do we get that information in there, without doing some weird hacky thing that's gonna be the source of errors and is gonna work on client and server.

[Dosant]

@ppisljar Could you please give an example of server usage?

[ppisljar]

yeah good point, lets not over complicate :)

[tsullivan]

Whose responsibility would it be to hide the sidebar and topnav? Will this new plugin also do some UI tweaks like hiding sidenav and topnav? Or should the rendered app hide those? Or those don't need to be hidden for reporting?

I think this responsibility falls under core. The application is mounted onto the page using Core APIs (I apologize if this oversimplifies it) and core renders the navigation and header as automatic functions. That means an application wouldn't be able to hide it, even if we're on a custom URL for Reporting.

Point of terminology: I prefer if we way "turn off" instead of "hide" the UI components. Hiding implies that CSS takes care of making it not render, although still in the page DOM. Being on the page means is triggered network requests for bundles and assets of different apps and plugins that we don't want. By saying "turn off" the UI components, it emphasis that the code for those components is nowhere on the page.

[Dosant]

Some suggestions and questions on the current approach,
also, probably some new arguments, why IMO we could consider making this part of the core (even though we want core to be as lean as possible)
Also, a thought on generalizing this into static mode

👇

[Dosant]

Just a note to consider:
If it is a plugin, then core can't use it and core's UI becomes an exception from the general guideline.

Header and side navigation is part of core and there is an API that apps use:

coreStart.chrome.setIsVisible(false);

I think the screenshotMode plugin shouldn't try to call this API, as there is a surface for bugs where an app's side effect makes it visible again. So probably apps should control this in the first place:

// app mount code 

if (plugins.screenshotMode.isScreenshotMode()) {
  coreStart.chrome.setIsVisible(false);
}

[Dosant]

Now, that's a bit of a bummer, that core's UI will have to be handled in one way (where apps call imperative core APIs), but lower-level plugins will check plugins.screenshotMode themselves.

For this particular case, it would have been a bit nicer if new screenshotMode is part of core

[ppisljar]

apps will already control the UI, and shouldn't use this service to determine if chrome needs to be hidden.

as app is the one providing reporting with the URL, it needs to make sure that URL is printable. that means disabling the chrome. and it shouldn't use our service (well it could, but it's free to choose, most likely it will have a special url parameter or even a special endpoint for this)

maybe we should follow the same approach with other things, so app should disable telemetry in this case etc. and then we don't need the screenshotMode service at all. But this service might make it easier for us to quickly disable some plugins, but as @lukeelmers mentioned below, should be used rarely

[Dosant]

For this particular example, would be even simpler if the newsfeed client-side plugin not loaded at all in screenshot mode?
I am curious if this is possible during "the hook-into rendering process" that you've described.
Or this could definitely be possible if screenshot thingy is part of the core and plugins can declare if they need to be skipped in screenshot mode in kibana.json

[Dosant]

Another thought here:

We have an agreement that plugins don't manage URL and only an App should talk to the URL.
Any state propagation should be done: URL -> app -> plugin -> app -> URL. reasoning here

I am curious if this is also applicable here. and the state propagation then should be something like this (with an exception that screenshot mode plugin will read from the URL screenshot state once in setup phase):

screenshot mode plugin -> app -> all other plugins 
                            | -> core

this would also address core vs other plugins inconsistency mentioned in the comment above, without making moving screenshot mode to core.

[ppisljar]

i would prefer no exception to that, the reasoning is reasonable imo :)

[tsullivan]

I'm not sure if I follow.

[ppisljar]

we are discussing puting a flag in the URL, and figured that would require plugin to manage the URL, but our current mentality is not to have plugins manage the URLs but have apps full responsible for it.

[Dosant]

I wonder if screenshot mode ownership and performance problems are essentially the same problems that we have and will have to address at some point for dashboard embed mode.

• Loading a single dashboard in embed mode loads too much code
• There are a lot of interactivity bugs. Most of them due to the only dashboard is aware of embed mode, but not lower-level plugins [meta] iFrame embedding inconsitencies  #93200

So I am genuinely curious if screenshot mode should be generalized into static mode. Handle all screenshots, embeds, and lighter view-only kibana use cases. Become part of the core to be able to optimize on-page load and on plugin bundling/loading level. See for example: #93496 (comment)

[ppisljar]

there are similar but different requirements i think. the most notable is that in embedded mode you most likely still want to collect telemetry and probably you still want a bit more interactivity that you would in report.

[lukeelmers]

We're talking about a lot of possible changes here with widespread ramifications, so my suggestion would be to stick with what can be done iteratively. To me, it seems the minimum things we'd need to get both a performance boost and offload responsibility for rendering reports boils down to what Tim outlined above:

1. Apps need to provide reporting with their "print/report-friendly" url and work to keep their reporting UI as lightweight as possible.
2. And we might need a lower-level service as described here (which should be rarely used) for plugins that don't register applications (or use URLs) to conditionally render themselves based on if it's screenshot/static mode.

I agree it would be a more convenient DX if core handled things like auto-hiding chrome, however as long as core (and other plugins) expose the right APIs for hiding nav elements, then I don't think there's any reason this has to live in core at the outset; plugins can still explicitly call these APIs. It also wouldn't preclude us from adding this to core further down the line if it made sense and doing these things manually turned out to be too burdensome.

Regarding performance, plugins should be aggressively working to keep their initial page load bundles as small as possible, and reporting is yet another reason why this is important. I think we're already going to see performance gains if plugins are loading modules asynchronously based on whether they're being rendered for a report.

TBH the idea of modifying the plugin manifest to allow plugins to ask core to conditionally load their bundles makes me a bit nervous for the reasons Josh mentions. I would instead vote to move forward with a simpler approach, and see how it improves performance as apps implement their own lightweight reporting views, and then consider more drastic measures only if we're confident they are necessary and have more evidence to justify them. (i.e. When we have a robust perf testing suite)

[ppisljar]

LGTM

[mshustov]

loads 163 requests.

And most of them performed to fetch plugin bundles?
Doesn't Headless browser support caching? puppeteer supports it
https://pptr.dev/#?product=Puppeteer&version=v2.0.0&show=api-pagesetcacheenabledenabled
Can we use Puppeteer or borrow their caching implementation?
If no, Core can consider serving a single bundle for all the Kiban plugins. We decided not to implement this logic when we've added long-term caching for bundle assets. So ideally, we would fix the caching problem for the headless browser.

[tsullivan]

And most of them performed to fetch plugin bundles?

That is correct. Since they are not fetched as background requests of plugins (e.g internal API requests for telemetry and newsfeed) there are limited options for a plugin to be able to reduce the 163 number.

Doesn't Headless browser support caching? puppeteer supports it
https://pptr.dev/#?product=Puppeteer&version=v2.0.0&show=api-pagesetcacheenabledenabled
Can we use Puppeteer or borrow their caching implementation?

Caching is disabled since there is network request interception in-place in Kibana Reporting, to implement the network policy rules, and prevent leaking credentials to 3rd parties. When network request interception is enabled, caching is disabled

Can we use Puppeteer or borrow their caching implementation?

We are using Puppeteer today.

NOTE: I just noticed this PR has been merged, which can offer help: puppeteer/puppeteer#6996

Even if we are able to restore caching, it should be understood that the cache is cleared in between Reporting jobs. The browser is "run as" multiple different users so we do not want to use a single cache for multiple jobs.

If no, Core can consider serving a single bundle for all the Kiban plugins. We decided not to implement this logic when we've added long-term caching for bundle assets. So ideally, we would fix the caching problem for the headless browser.

This would be fantastic for Reporting!

Thanks for your comments!

[oatkiller]

Hey all,
Question: instead of the approach mentioned here, could it make sense to use the Webpack DefinePlugin to inject a variable that indicates that rendering is being done? This would involve building Kibana once with that variable as false and once with true. I don't fully understand all of the concerns laid out in this issue, so forgive me if this has already been covered.

Here is some justification for my idea:

• The Kibana client code changes would only involve adding conditional statements that reference a global variable (e.g. process.env.isReporting
• A Kibana client instance, in memory, (e.g. a single browser tab showing Kibana) is always either reporting or not, it never changes, therefore there should be no need for sophisticated dynamism.
• By handling this at build time, other optimization could be made, e.g. removing plugins entirely from the build.

Anyway, if these ideas are totally off base then please feel free to ignore :)

[tsullivan]

Hey all,
Question: instead of the approach mentioned here, could it make sense to use the Webpack DefinePlugin to inject a variable that indicates that rendering is being done? This would involve building Kibana once with that variable as false and once with true. I don't fully understand all of the concerns laid out in this issue, so forgive me if this has already been covered.

Here is some justification for my idea:

• The Kibana client code changes would only involve adding conditional statements that reference a global variable (e.g. process.env.isReporting
• A Kibana client instance, in memory, (e.g. a single browser tab showing Kibana) is always either reporting or not, it never changes, therefore there should be no need for sophisticated dynamism.
• By handling this at build time, other optimization could be made, e.g. removing plugins entirely from the build.

Anyway, if these ideas are totally off base then please feel free to ignore :)

This is a fascinating idea. Today, we use the same build of Kibana UI for Reporting jobs as the one served to interactive users. If we did, Reporting could go a lot further to run with an optimized UI. However, I think this would make testing harder and maybe slow down development of UI features. Every change made in UI code could create unexpected results in the "Reporting build" of the UI, whereas that is not the UI that developers would have their hands on most of the time.

[pgayvallet]

By handling this at build time, other optimization could be made, e.g. removing plugins entirely from the build.

FWIW, the list of bundles to load is not determined at build time but at run time in the rendering service. Forcing some plugins to not load for reporting would still require runtime modifications in core with this approach.

[oatkiller]

@tsullivan and @pgayvallet: thanks for the explanations. I appreciate hearing your reasons and I learned something as well.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 93496 or prevent reminders by adding the backport:skip label.