Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[rfc][skip-ci] Screenshot Mode Service #93496
[rfc][skip-ci] Screenshot Mode Service #93496
Changes from all commits
cc64db1
abf1a16
f0b4cc7
61d526f
309c30f
abdf4fd
4dde01a
dff5d27
6a8181f
125f9ef
3e7f468
a491a72
7eb12f6
20609cf
b577f09
f68c0d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And most of them performed to fetch plugin bundles?
Doesn't Headless browser support caching?
puppeteer
supports ithttps://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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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.
This would be fantastic for Reporting!
Thanks for your comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 coreThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):this would also address core vs other plugins inconsistency mentioned in the comment above, without making moving
screenshot mode
to core.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would prefer no exception to that, the reasoning is reasonable imo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use this param not just for testing? It would be like a real alternative to the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should think thru the internal implementation as well, once we agree on the public interface, which seems quite simple:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppisljar Could you please give an example of server usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good point, lets not over complicate :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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