[Screenshot mode] Create plugin to provide "screenshot mode" awareness #99627

jloleysens · 2021-05-10T13:29:57Z

Summary

This contribution adds the new screenshot mode low-level plugin (a plugin that does not depend on other plugins) to Kibana.

The consumer plugins of this plugin should typically be fairly low-level plugins. This provides a way for resources (code, data, network requests) to not be loaded unnecessarily for an environment flagged as intended for screenshots (i.e., no user interaction). For example it does not make sense to report UI telemetry (client side) or API usage telemetry (server side).

Client-side

We expose a service that can be used by consumers to determine whether we are in screenshot mode. Screenshot mode should be detected pre-setup phase to expose this information before services or resources are started.

Server-side

We expose a flag request handler context object (RequestHandlerContext) to indicate that a request originated from a client that has optimised for taking screenshots.

To reviewers

(to all) Please see the example screenshot plugin for details on when/how the service can be consumed.
(to kibana ops) The current implementation uses require to a load a function from a file in the screenshot mode service that is passed into puppeteer, not sure whether this might have implications for any release builds? (see x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts)
(to security) will this usable for particular server-side use-cases?

Additional notes

The intended use case, per the README, is not to be used by higher-level plugins for determining when to render a reporting or print-friendly layout. The recommendation is to rather create a dedicated app route that loads only the UI and resources needed by the app.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

jloleysens · 2021-05-11T12:17:08Z

@elasticmachine merge upstream

jloleysens · 2021-05-12T07:54:03Z

@elasticmachine merge upstream

Also hooked it up to the chromium browser imitating the preload functionality of electron to set up the environment before code runs.

screenshot mode

…d on the client

jloleysens · 2021-05-14T09:36:57Z

@elasticmachine merge upstream

elasticmachine · 2021-05-14T09:37:11Z

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

elasticmachine · 2021-05-14T09:37:13Z

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

Added a server-side example for screenshot mode Export the screenshot mode header in both public and server

mistic

Changes under operations team code owners LGTM

tsullivan

Just two minor comments for now

tsullivan · 2021-05-14T22:03:00Z

src/plugins/screenshot_mode/common/constants.ts

+ * Side Public License, v 1.
+ */
+
+export const KBN_SCREENSHOT_MODE_HEADER = 'x-kbn-screenshot-mode'.toLowerCase();


Looks like .toLowerCase() can be removed

tsullivan · 2021-05-14T23:00:04Z

src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts

+ * localStorage. The ability to set a value in localStorage enables more convenient development and testing
+ * in functionality that needs to detect screenshot mode.
+ */
+export const getScreenshotMode = (): unknown => {


Shouldn't this return a boolean?

move require() to screenshotMode plugin

jloleysens · 2021-05-17T08:18:01Z

@elasticmachine merge upstream

changed the screenshotmode detection function to check for a specific value.

azasypkin · 2021-05-17T09:20:07Z

src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts

@@ -0,0 +1,51 @@
+/*


question: Just out of curiosity, I see this common file has a hard dependency on window, how does this code work on the server side?

That's a good question, it doesn't 😅 . It is intended for use by reporting which injects this function into the "preload" phase of opening a page.

Happy to capture that in a comment or consider a different implementation. WDYT?

That makes sense, thanks! I think mentioning this in the comment would be beneficial, just in case someone like me stumbles upon this file in the future.

++ it took me a minute to understand it too - but it's pretty cool how it works

azasypkin · 2021-05-17T09:41:15Z

src/plugins/screenshot_mode/server/plugin.ts

+
+export class ScreenshotModePlugin implements Plugin<ScreenshotModePluginSetup> {
+  public setup(core: CoreSetup) {
+    core.http.registerRouteHandlerContext<ScreenshotModeRequestHandlerContext, 'screenshotMode'>(


issue: having isScreenshot in the request context is great, but unfortunately request context isn't available during authentication. Authentication is performed within auth handler that doesn't have access to the request context.

Maybe you can expose an additional method from the start or setup (or both) contract too, so that the code that doesn't have access to request context for one reason or another can use it instead (with a drawback of having dependency on screenshotMode plugin)?

public setup/start() { return { isScreenshot: (request: KibanaRequest) => Object.keys(request.headers).some((header) => { return header.toLowerCase() === KBN_SCREENSHOT_MODE_HEADER; }), }; }

In the future, I believe the Kibana platform should be the one who determines in which mode Kibana is used. This way it can extend KibanaRequest with additional property or propagate this knowledge to plugins in any other way without introducing additional plugin dependencies. But it's too early to generalize, let's see if this solution is enough.

Thanks for the input @azasypkin , that trade-off makes sense to me!

azasypkin · 2021-05-17T12:08:49Z

src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts

+ * references as possible to make it portable. For instance, running inside puppeteer.
+ */
+export const setScreenshotModeEnabled = () => {
+  Object.defineProperty(window, '__KBN_SCREENSHOT_MODE_ENABLED_KEY__', {


question: Btw, should we use constant here (and in setScreenshotModeDisabled)?

Suggested change

Object.defineProperty(window, '__KBN_SCREENSHOT_MODE_ENABLED_KEY__', {

Object.defineProperty(window, KBN_SCREENSHOT_MODE_ENABLED_KEY, {

Because this function is used in puppeteer it cannot have references to external variables that are not available from within the puppeteer environment where this code is run.

I'll add an inline comment to this effect.

checks a kibana request to determine whether we in screenshot mode

…eference

azasypkin

The API exposed by the screenshotMode plugin LGTM, thanks!

jloleysens · 2021-05-18T09:13:13Z

@elasticmachine merge upstream

update reporting example

jloleysens · 2021-05-19T10:27:15Z

@elasticmachine merge upstream

kibanamachine · 2021-05-19T12:13:10Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`screenshotMode`	-	8	+8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`reporting`	130	132	+2
`screenshotMode`	-	9	+9
total			+11

Page load bundle

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

id	before	after	diff
`screenshotMode`	-	3.0KB	+3.0KB

Unknown metric groups

API count

id	before	after	diff
`reporting`	131	133	+2
`screenshotMode`	-	13	+13
total			+15

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`licensing`	18	15	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`stackAlerts`	101	95	-6
total			-161

History

💔 Build #126495 failed a6eef5b
💚 Build #126333 succeeded 788078c
💚 Build #126228 succeeded 755db33
💚 Build #126217 succeeded a0b9e3b
💚 Build #126177 succeeded cb95a41

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

elastic#99627) * initial version of the screenshot mode service * First iteration of client side of screenshot mode plugin Also hooked it up to the chromium browser imitating the preload functionality of electron to set up the environment before code runs. * First implementation of server-side logic for detecting screenshot mode * fix some type issues and do a small refactor * fix size limits, docs and ts issues * fixed types issues and made sure screenshot mode is correctly detected on the client * Moved the screenshot mode header definition to common Added a server-side example for screenshot mode Export the screenshot mode header in both public and server * move require() to screenshotMode plugin * Update chromium_driver.ts * cleaned up some comments, minor refactor in ReportingCore and changed the screenshotmode detection function to check for a specific value. * fix export * Expanded server-side screenshot mode contract with function that checks a kibana request to determine whether we in screenshot mode * added comments to explain use of literal value rather than external reference * updated comment * update reporting example Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Timothy Sullivan <[email protected]> Co-authored-by: Tim Sullivan <[email protected]> # Conflicts: # x-pack/plugins/reporting/server/core.ts # x-pack/plugins/reporting/server/plugin.ts

tsullivan

LGTM!

…areness (#99627) (#100331) * [Screenshot mode] Create plugin to provide "screenshot mode" awareness (#99627) * initial version of the screenshot mode service * First iteration of client side of screenshot mode plugin Also hooked it up to the chromium browser imitating the preload functionality of electron to set up the environment before code runs. * First implementation of server-side logic for detecting screenshot mode * fix some type issues and do a small refactor * fix size limits, docs and ts issues * fixed types issues and made sure screenshot mode is correctly detected on the client * Moved the screenshot mode header definition to common Added a server-side example for screenshot mode Export the screenshot mode header in both public and server * move require() to screenshotMode plugin * Update chromium_driver.ts * cleaned up some comments, minor refactor in ReportingCore and changed the screenshotmode detection function to check for a specific value. * fix export * Expanded server-side screenshot mode contract with function that checks a kibana request to determine whether we in screenshot mode * added comments to explain use of literal value rather than external reference * updated comment * update reporting example Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Timothy Sullivan <[email protected]> Co-authored-by: Tim Sullivan <[email protected]> # Conflicts: # x-pack/plugins/reporting/server/core.ts # x-pack/plugins/reporting/server/plugin.ts * satisfy for tslint formatting Co-authored-by: Kibana Machine <[email protected]>

elastic#99627) * initial version of the screenshot mode service * First iteration of client side of screenshot mode plugin Also hooked it up to the chromium browser imitating the preload functionality of electron to set up the environment before code runs. * First implementation of server-side logic for detecting screenshot mode * fix some type issues and do a small refactor * fix size limits, docs and ts issues * fixed types issues and made sure screenshot mode is correctly detected on the client * Moved the screenshot mode header definition to common Added a server-side example for screenshot mode Export the screenshot mode header in both public and server * move require() to screenshotMode plugin * Update chromium_driver.ts * cleaned up some comments, minor refactor in ReportingCore and changed the screenshotmode detection function to check for a specific value. * fix export * Expanded server-side screenshot mode contract with function that checks a kibana request to determine whether we in screenshot mode * added comments to explain use of literal value rather than external reference * updated comment * update reporting example Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Timothy Sullivan <[email protected]> Co-authored-by: Tim Sullivan <[email protected]>

initial version of the screenshot mode service

3bb2aca

jloleysens added v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels May 10, 2021

Merge branch 'master' into screenshot-mode/create-plugin

0472bcf

kibanamachine and others added 6 commits May 12, 2021 03:54

Merge branch 'master' into screenshot-mode/create-plugin

c488af9

First iteration of client side of screenshot mode plugin

8a4e1d0

Also hooked it up to the chromium browser imitating the preload functionality of electron to set up the environment before code runs.

First implementation of server-side logic for detecting

c48417d

screenshot mode

fix some type issues and do a small refactor

da45eef

fix size limits, docs and ts issues

79ed40a

fixed types issues and made sure screenshot mode is correctly detecte…

15f05f2

…d on the client

jloleysens requested review from tsullivan, azasypkin and mistic May 14, 2021 09:34

Merge branch 'master' into screenshot-mode/create-plugin

c10905b

jloleysens marked this pull request as ready for review May 14, 2021 09:37

jloleysens requested review from a team as code owners May 14, 2021 09:37

Moved the screenshot mode header definition to common

2b10497

Added a server-side example for screenshot mode Export the screenshot mode header in both public and server

mistic approved these changes May 14, 2021

View reviewed changes

ryankeairns approved these changes May 14, 2021

View reviewed changes

move require() to screenshotMode plugin

aaab2af

tsullivan reviewed May 14, 2021

View reviewed changes

tsullivan and others added 2 commits May 14, 2021 16:44

Update chromium_driver.ts

7dc5c0f

Merge pull request #3 from tsullivan/screenshot-mode/create-plugin

76242a6

move require() to screenshotMode plugin

kibanamachine and others added 3 commits May 17, 2021 04:18

Merge branch 'master' into screenshot-mode/create-plugin

4f95774

cleaned up some comments, minor refactor in ReportingCore and

69357bf

changed the screenshotmode detection function to check for a specific value.

fix export

cb95a41

azasypkin reviewed May 17, 2021

View reviewed changes

jloleysens added 2 commits May 17, 2021 14:56

Expanded server-side screenshot mode contract with function that

a0b9e3b

checks a kibana request to determine whether we in screenshot mode

added comments to explain use of literal value rather than external r…

755db33

…eference

azasypkin approved these changes May 17, 2021

View reviewed changes

updated comment

b96bf45

Merge branch 'master' into screenshot-mode/create-plugin

788078c

tsullivan and others added 2 commits May 18, 2021 12:27

update reporting example

8eb5139

Merge pull request #4 from tsullivan/screenshot-mode/create-plugin

a6eef5b

update reporting example

Merge branch 'master' into screenshot-mode/create-plugin

e5af77f

jloleysens merged commit f97aad3 into elastic:master May 19, 2021

jloleysens deleted the screenshot-mode/create-plugin branch May 19, 2021 14:03

jloleysens mentioned this pull request May 19, 2021

[7.x] [Screenshot mode] Create plugin to provide "screenshot mode" awareness (#99627) #100331

Merged

tsullivan reviewed May 19, 2021

View reviewed changes

jloleysens mentioned this pull request Jun 3, 2021

[Reporting/Newsfeed] Prevent fetching newsfeed items when generating a report #101250

Closed

sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Screenshot mode] Create plugin to provide "screenshot mode" awareness #99627

[Screenshot mode] Create plugin to provide "screenshot mode" awareness #99627

jloleysens commented May 10, 2021 •

edited

Loading

jloleysens commented May 11, 2021

jloleysens commented May 12, 2021

jloleysens commented May 14, 2021

elasticmachine commented May 14, 2021

elasticmachine commented May 14, 2021

mistic left a comment

tsullivan left a comment

tsullivan May 14, 2021

tsullivan May 14, 2021

jloleysens commented May 17, 2021

azasypkin May 17, 2021

jloleysens May 17, 2021

azasypkin May 17, 2021

tsullivan May 17, 2021

azasypkin May 17, 2021

jloleysens May 17, 2021

azasypkin May 17, 2021

jloleysens May 17, 2021

azasypkin left a comment

jloleysens commented May 18, 2021

jloleysens commented May 19, 2021

kibanamachine commented May 19, 2021

API count

References to deprecated APIs

tsullivan left a comment

	Object.defineProperty(window, '__KBN_SCREENSHOT_MODE_ENABLED_KEY__', {
	Object.defineProperty(window, KBN_SCREENSHOT_MODE_ENABLED_KEY, {

[Screenshot mode] Create plugin to provide "screenshot mode" awareness #99627

[Screenshot mode] Create plugin to provide "screenshot mode" awareness #99627

Conversation

jloleysens commented May 10, 2021 • edited Loading

Summary

Client-side

Server-side

To reviewers

Additional notes

Checklist

jloleysens commented May 11, 2021

jloleysens commented May 12, 2021

jloleysens commented May 14, 2021

elasticmachine commented May 14, 2021

elasticmachine commented May 14, 2021

mistic left a comment

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

jloleysens commented May 18, 2021

jloleysens commented May 19, 2021

kibanamachine commented May 19, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

References to deprecated APIs

History

tsullivan left a comment

Choose a reason for hiding this comment

jloleysens commented May 10, 2021 •

edited

Loading