[Screenshotting] Fix potential race condition when screenshotting #123820

jloleysens · 2022-01-26T13:38:08Z

Summary

Closes #122391

We reduce unnecessary reflow by:

Approximating screenshot view port size at page creation time. In all cases width will be set correctly, for print-optimized the viewport will be height adjusted, causing a vertical-only reflow. This improves the need to adjust charts horizontally which has been observed to be an issue.
We naively chain setting viewport size before looking for "render complete" visualisations which should create a slight buffer for the browser to complete reflow and redraw.

How to test

Manually test creating a visual report for:

Canvas (regular + full page)
Dashboard (regular + print-optimized)
Visualize

In all cases check that the resulting report looks as expected.

Notes

A further step could be to create a "ReflowObserver" that we'd use in the HeadlessChromiumDriver when setting the viewport size (inspired by https://stackoverflow.com/a/66646270/5546621).

markov00 · 2022-01-27T17:24:38Z

x-pack/plugins/screenshotting/server/screenshots/observable.ts

+        return driver
+          .setViewport(viewport, this.logger)
+          .then(() =>
+            waitForVisualizations(driver, this.logger, waitTimeout, itemsCount, this.layout)
+          );


Hi, I've just a side note, not related to this PR but something that can be considered in the future to improve the rendering time: what about considering setting the viewport before opening the page? this will skip at least 1 rendering cycle to resize all the visualizations. We can probably get this information from the URL and/or the saved object related to that dashboard/viz.

Hey @markov00 ! That's a great idea. I was thinking we might have to do something a bit more complicated (like inject a resize observer) to really know when the reflow is done 😅 . Your suggestion sounds a lot simpler and definitely worth looking into.

It seems the reason we do this here is because this is the point where we know how many items there are to render. For print layout, we have to know the number of items to resize the viewport: https://github.com/elastic/kibana/blob/main/x-pack/plugins/screenshotting/server/layouts/print_layout.ts#L47

For a later PR perhaps: let's consider making this whole mergeMap conditional, and used only if the report job is the "optimized for print" layout mode.

if (this.layout.id === LayoutTypes.PRINT) { // set the viewport to the dimentions from the job, to allow elements to flow into print-optimized layout const viewport = this.layout.getViewport(itemsCount) || getDefaultViewPort(); await driver.setViewport(viewport, this.logger); }

That's a good point @tsullivan , it looks like the resize is for height in that case so I think we should at least be able to address a "width" reflow which seems could be more problematic.

I'll open up an issue to capture your suggested change!

jloleysens · 2022-01-28T11:27:18Z

@elasticmachine merge upstream

kibanamachine · 2022-01-28T11:27:19Z

merge conflict between base and head

…fix-potential-race-condition-when-screenshotting * 'main' of github.com:elastic/kibana: (75 commits) [Reporting] Logging improvements while generating reports (elastic#123802) [Uptime] Default alert connectors email settings (elastic#123244) Update comparison series styles to match the main series (elastic#123858) [RAC][Uptime] remove extra dot from the uptime alert connector message (elastic#124000) [Exploratory view] Allow ability add extra actions in lens embeddable (elastic#123713) [SecuritySolution][Investigations] Add message about missing index in data view in analyzer (elastic#122859) [TSVB] Formatting in the left axis is not respected when I have two separate axis (elastic#123903) [Discover] Remove services from component dependencies (elastic#121691) Stop IM rule execution if there are no events (elastic#123811) [Security Solution][Endpoint] Update Fleet Trusted Apps and Host Isolation Exception cards to use exception list summary API (elastic#123900) [Security Solution][Exceptions] Switches modal to flyout component (elastic#123408) [Workplace Search] Fix bug where modal visible after deleting a group (elastic#123976) [Alerting] Remove state variables from action variable menu (elastic#123702) replace deprecated api usage (elastic#123970) Fix package policy merge logic for boolean values (elastic#123974) [Security Solution][Endpoint][Policy] Remove GET policy list api route (elastic#123873) Reenable alert_add test suite (elastic#123862) [Fleet] Remove usage of IFieldType in Fleet (elastic#123960) [Lists] Add an instance of `ExceptionListClient` with server extension points turned off to context object provided to callbacks (elastic#123885) [Maps] Add execution context (elastic#123651) ... # Conflicts: # x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.ts

… viewport for every page that is created

… have a "undefined" set as a key value

jloleysens · 2022-01-31T10:42:59Z

@elasticmachine merge upstream

…when-screenshotting

elasticmachine · 2022-01-31T10:43:09Z

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

elasticmachine · 2022-01-31T10:43:11Z

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

jloleysens · 2022-02-01T11:01:38Z

@elasticmachine merge upstream

…when-screenshotting

jloleysens · 2022-02-01T16:36:14Z

@elasticmachine merge upstream

…when-screenshotting

jloleysens · 2022-02-03T08:29:44Z

@elasticmachine merge upstream

…when-screenshotting

jloleysens · 2022-02-03T10:31:14Z

@elasticmachine merge upstream

…when-screenshotting

tsullivan · 2022-02-03T20:12:58Z

x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.ts

+          // browser reflow. In most cases only the height needs to be adjusted
+          // before taking a screenshot.
+          // NOTE: _.defaults assigns to the target object, so we copy it.
+          defaultViewport: _.defaults({ ...defaultViewport }, DEFAULT_VIEWPORT),


Is there a reason why this can't be just:

Suggested change

defaultViewport: _.defaults({ ...defaultViewport }, DEFAULT_VIEWPORT),

defaultViewport: { ...DEFAULT_VIEWPORT, ...defaultViewport },

The reason I chose _.defaults is because the override behaviour is different with own props (it ignores overrides that are undefined, so we won't end up with width: undefined for ex.).

Do you think we should change the type of defaultViewport making both width and height required values? I see they are both required in the Size object. WDYT?

I tested this locally, and it looks like height can currently be undefined when passing it directly from layout.height to the createPage call resulting in this error:

[2022-02-07T10:36:53.499+01:00][INFO ][plugins.reporting.runTask.printablePdfV2.printable_pdf_v2.execute-job.kzci20um17zm9d0062eb528s] Compiling PDF using "preserve_layout" layout... [2022-02-07T10:36:55.542+01:00][INFO ][plugins.reporting.runTask] Saved printable_pdf_v2 job /.reporting-2022-02-06/_doc/kzci20um17zm9d0062eb528s [2022-02-07T10:36:59.710+01:00][INFO ][plugins.screenshotting.screenshot.browser-driver] Creating browser page driver Unhandled Promise rejection detected: Error: Protocol error (Emulation.setDeviceMetricsOverride): Invalid parameters Failed to deserialize params.height - BINDINGS: mandatory field missing at position 88 at /Users/jeanlouisleysens/repos/work/kibana/node_modules/puppeteer/src/common/Connection.ts:291:57 at new Promise (<anonymous>) at CDPSession.send (/Users/jeanlouisleysens/repos/work/kibana/node_modules/puppeteer/src/common/Connection.ts:290:12) at EmulationManager.emulateViewport (/Users/jeanlouisleysens/repos/work/kibana/node_modules/puppeteer/src/common/EmulationManager.ts:41:20) at Page.setViewport (/Users/jeanlouisleysens/repos/work/kibana/node_modules/puppeteer/src/common/Page.ts:2383:54) at Function.create (/Users/jeanlouisleysens/repos/work/kibana/node_modules/puppeteer/src/common/Page.ts:444:37) at runMicrotasks (<anonymous>) at processTicksAndRejections (node:internal/process/task_queues:96:5) at Browser._createPageInContext (/Users/jeanlouisleysens/repos/work/kibana/node_modules/puppeteer/src/common/Browser.ts:468:18) at Observable._subscribe (/Users/jeanlouisleysens/repos/work/kibana/x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.ts:125:20)

I think, for now, we can leave this default using the lodash algo. WDYT?

tsullivan · 2022-02-03T20:16:36Z

x-pack/plugins/screenshotting/server/layouts/create_layout.ts

+/**
+ * We naively round all numeric values in the object, this will break screenshotting
+ * if ever a have a non-number set as a value, but this points to an issue
+ * in the code responsible for creating the dimensions object.


would it be possible to have a unit test that explores this?

Hm, I think we could, but it may need to be a funcitonal test because the "break" occurs when we pass any non integer value to Chromium when launching (so floats or NaN would break it).

Is that the kind of test you had in mind?

tsullivan · 2022-02-03T20:22:18Z

x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.ts

@@ -36,6 +37,12 @@ import { getMetrics, PerformanceMetrics } from './metrics';

 interface CreatePageOptions {
  browserTimezone?: string;
+  defaultViewport?: {


Should this be a required property instead of optional? It looks like the only area it's not given is in tests.

Yeah, let's do that! I'll also make both height and width required values!

Great! If it doesn't impact anything (existing POST URLs?), it would make things more clear.

jloleysens · 2022-02-07T09:17:30Z

@elasticmachine merge upstream

…when-screenshotting

dokmic · 2022-02-08T09:18:27Z

x-pack/plugins/screenshotting/server/screenshots/observable.ts

+        return driver
+          .setViewport(viewport, this.logger)
+          .then(() =>
+            waitForVisualizations(driver, this.logger, waitTimeout, itemsCount, this.layout)
+          );


Suggested change

return driver

.setViewport(viewport, this.logger)

.then(() =>

waitForVisualizations(driver, this.logger, waitTimeout, itemsCount, this.layout)

);

await driver.setViewport(viewport, this.logger)

await waitForVisualizations(driver, this.logger, waitTimeout, itemsCount, this.layout);

jloleysens · 2022-02-08T09:24:41Z

@elasticmachine merge upstream

…when-screenshotting

jloleysens · 2022-02-08T09:31:46Z

Hey @dokmic and @tsullivan I think I've addressed all your feedback. Do you think you could take another look?

kibana-ci · 2022-02-08T11:01:16Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #22048 succeeded deb0e4e
💛 Build #21548 was flaky 4c293fa
💔 Build #21515 failed f5f8fbe
💚 Build #21307 succeeded 58d50b6
💔 Build #21251 failed 10bebea
💔 Build #21119 failed eb45d09

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

dokmic

LGTM. Great job 👍

jloleysens · 2022-02-09T10:43:42Z

@tsullivan happy to return to this in a follow up PR #123820 (comment)!

Let me know what you think, I decided to merge this in the meantime.

…astic#123820) * extract message from error objects * only warn for 400 and up status codes * naively wait for vis ready after resizing the browser viewport * use a single default viewport size, enable layout to set default page viewport for every page that is created * refactor viewport -> windowSize in chromium args * allow overriding defaults and use new windowSize arg for chromium args * always round page dimension numbers. note: this will break if we ever have a "undefined" set as a key value * added comment * update snapshot to new width value * make defaultViewport a required field on createPage * added comment * style: use async-await rather than .then chaining. also added a comment Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit b2b60ff)

…23820) (#125055) * extract message from error objects * only warn for 400 and up status codes * naively wait for vis ready after resizing the browser viewport * use a single default viewport size, enable layout to set default page viewport for every page that is created * refactor viewport -> windowSize in chromium args * allow overriding defaults and use new windowSize arg for chromium args * always round page dimension numbers. note: this will break if we ever have a "undefined" set as a key value * added comment * update snapshot to new width value * make defaultViewport a required field on createPage * added comment * style: use async-await rather than .then chaining. also added a comment Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit b2b60ff)

…23820) (#125054) * extract message from error objects * only warn for 400 and up status codes * naively wait for vis ready after resizing the browser viewport * use a single default viewport size, enable layout to set default page viewport for every page that is created * refactor viewport -> windowSize in chromium args * allow overriding defaults and use new windowSize arg for chromium args * always round page dimension numbers. note: this will break if we ever have a "undefined" set as a key value * added comment * update snapshot to new width value * make defaultViewport a required field on createPage * added comment * style: use async-await rather than .then chaining. also added a comment Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit b2b60ff)

jloleysens added 3 commits January 26, 2022 11:43

extract message from error objects

d597622

only warn for 400 and up status codes

c0e1bab

naively wait for vis ready after resizing the browser viewport

bdf381a

jloleysens added discuss (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services v8.1.0 v8.2.0 v8.0.1 labels Jan 26, 2022

jloleysens requested a review from tsullivan January 26, 2022 13:38

markov00 reviewed Jan 27, 2022

View reviewed changes

jloleysens and others added 6 commits January 28, 2022 12:32

use a single default viewport size, enable layout to set default page…

9522cd2

… viewport for every page that is created

refactor viewport -> windowSize in chromium args

bf82e12

allow overriding defaults and use new windowSize arg for chromium args

79f3af6

always round page dimension numbers. note: this will break if we ever…

6ab39e8

… have a "undefined" set as a key value

added comment

c8583e2

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

4877026

…when-screenshotting

jloleysens marked this pull request as ready for review January 31, 2022 10:43

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

73eb3c5

…when-screenshotting

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

eb45d09

…when-screenshotting

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

58d50b6

…when-screenshotting

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

f5f8fbe

…when-screenshotting

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

4c293fa

…when-screenshotting

tsullivan reviewed Feb 3, 2022

View reviewed changes

kibanamachine and others added 3 commits February 7, 2022 04:17

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

5b072ee

…when-screenshotting

make defaultViewport a required field on createPage

a119d82

added comment

deb0e4e

dokmic reviewed Feb 8, 2022

View reviewed changes

kibanamachine and others added 2 commits February 8, 2022 04:24

Merge branch 'main' into screenshotting/fix-potential-race-condition-…

9f7eb66

…when-screenshotting

style: use async-await rather than .then chaining. also added a comment

b77f4cf

dokmic approved these changes Feb 8, 2022

View reviewed changes

jloleysens merged commit b2b60ff into elastic:main Feb 9, 2022

jloleysens deleted the screenshotting/fix-potential-race-condition-when-screenshotting branch February 9, 2022 10:42

jloleysens mentioned this pull request Feb 9, 2022

[8.1] [Screenshotting] Fix potential race condition when screenshotting (#123820) #125054

Merged

This was referenced Feb 9, 2022

[8.0] [Screenshotting] Fix potential race condition when screenshotting (#123820) #125055

Merged

[Reporting] When capturing screenshots only set viewport size for print-layout reports #125056

Closed

exalate-issue-sync bot mentioned this pull request Jun 27, 2022

[Screenshotting] Remove unnecessary viewport resizing #135225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Screenshotting] Fix potential race condition when screenshotting #123820

[Screenshotting] Fix potential race condition when screenshotting #123820

jloleysens commented Jan 26, 2022 •

edited

Loading

markov00 Jan 27, 2022

jloleysens Jan 28, 2022

tsullivan Feb 3, 2022

tsullivan Feb 4, 2022

jloleysens Feb 7, 2022

jloleysens commented Jan 28, 2022

kibanamachine commented Jan 28, 2022

jloleysens commented Jan 31, 2022

elasticmachine commented Jan 31, 2022

elasticmachine commented Jan 31, 2022

jloleysens commented Feb 1, 2022

jloleysens commented Feb 1, 2022

jloleysens commented Feb 3, 2022

jloleysens commented Feb 3, 2022

tsullivan Feb 3, 2022

jloleysens Feb 7, 2022

jloleysens Feb 7, 2022 •

edited

Loading

tsullivan Feb 3, 2022

jloleysens Feb 7, 2022

tsullivan Feb 3, 2022

jloleysens Feb 7, 2022

tsullivan Feb 7, 2022

jloleysens commented Feb 7, 2022

dokmic Feb 8, 2022 •

edited

Loading

jloleysens commented Feb 8, 2022

jloleysens commented Feb 8, 2022

kibana-ci commented Feb 8, 2022

dokmic left a comment

jloleysens commented Feb 9, 2022

	defaultViewport: _.defaults({ ...defaultViewport }, DEFAULT_VIEWPORT),
	defaultViewport: { ...DEFAULT_VIEWPORT, ...defaultViewport },

[Screenshotting] Fix potential race condition when screenshotting #123820

[Screenshotting] Fix potential race condition when screenshotting #123820

Conversation

jloleysens commented Jan 26, 2022 • edited Loading

Summary

How to test

Notes

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

jloleysens commented Jan 28, 2022

kibanamachine commented Jan 28, 2022

jloleysens commented Jan 31, 2022

elasticmachine commented Jan 31, 2022

elasticmachine commented Jan 31, 2022

jloleysens commented Feb 1, 2022

jloleysens commented Feb 1, 2022

jloleysens commented Feb 3, 2022

jloleysens commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens Feb 7, 2022 • edited Loading

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

jloleysens commented Feb 7, 2022

dokmic Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

jloleysens commented Feb 8, 2022

jloleysens commented Feb 8, 2022

kibana-ci commented Feb 8, 2022

💚 Build Succeeded

Metrics [docs]

History

dokmic left a comment

Choose a reason for hiding this comment

jloleysens commented Feb 9, 2022

jloleysens commented Jan 26, 2022 •

edited

Loading

jloleysens Feb 7, 2022 •

edited

Loading

dokmic Feb 8, 2022 •

edited

Loading