[Reporting/PDF] Refactor screenshot pipeline for multi-url by default #48588

tsullivan · 2019-10-17T21:46:03Z

Summary

This PR is primarily improvements to Typescript and simplifying interfaces between modules. I think these changes will go towards implementing scheduled reports because the screenshot observable is easier to invoke.

This takes away the requirement that when using screenshotObservable, you would have to put it in the body of a loop to capture multiple URLs (Or ). This change makes the screenshotObservable take a string array parameter for URLs.

The change includes a refactoring of the screenshotsObservable pipeline. The changes allow for better Typescript inference of the payload type of the pipelines. The screenshot pipeline was also simplified by hoisting out a block of code into a new module called scanPage.

There are also changes to a few other interfaces:

browserDriverFactory.test() method doesn't take the layout params, they are hardcoded in the test now.
browserDriverFactory.createPage() method accepts a logger object, instead of using a class field for the logger.
browserDriverFactory.createPage() method returns an Observable of an object with 2 fields: driver and exit$. Previously, the driver object was an observable to the driver, that had to be piped out to be used. Now, driver is the raw driver. This means that the resolved driver no longer has to be streamed through most of the screenshot observable pipe, as it was before.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

tsullivan · 2019-12-12T21:58:19Z

x-pack/legacy/plugins/reporting/export_types/common/lib/screenshots/index.ts

      }),
-      first()
+      toArray()


Main changes (1) of this PR are above

Oops, having the first operator is important, so that the observable actually finishes.

tsullivan · 2019-12-12T21:58:39Z

x-pack/legacy/plugins/reporting/export_types/png/server/execute_job/index.ts

+      map(decryptedHeaders => omitBlacklistedHeaders({ job, decryptedHeaders })),
+      map(filteredHeaders => getConditionalHeaders({ server, job, filteredHeaders })),
+      concatMap(conditionalHeaders => {
+        const urls = getFullUrls({ server, job });


Main changes (2)

tsullivan · 2019-12-12T21:59:25Z

x-pack/legacy/plugins/reporting/export_types/printable_pdf/server/execute_job/index.ts

+          layout,
+          logo
+        );
+      }),


main changes (3)

tsullivan · 2019-12-12T22:00:48Z

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

@@ -206,7 +213,7 @@ export class HeadlessChromiumDriverFactory {
    // just log closing of the process
    const processClose$ = Rx.fromEvent<void>(childProcess, 'close').pipe(
      tap(() => {
-        this.logger.debug('child process closed', ['headless-browser-process']);
+        logger.debug('child process closed', ['headless-browser-process']);


main changes (4) - pass a logger object to createPage instead have a this.logger

tsullivan · 2019-12-12T22:01:21Z

x-pack/legacy/plugins/reporting/server/lib/validate/validate_browser.ts

-          throw new Error('Could not close browser client handle!');
-        }
-      });
+    return browserFactory.test(logger).then((browser: Browser | null) => {


Opportune change: the test method doesn't need a dynamic viewport

tsullivan · 2019-12-12T22:02:34Z

x-pack/legacy/plugins/reporting/export_types/common/execute_job/decrypt_job_headers.test.ts

@@ -43,7 +43,7 @@ describe('headers', () => {
    };

    const encryptedHeaders = await encryptHeaders(headers);
-    const { decryptedHeaders } = await decryptJobHeaders({
+    const decryptedHeaders = await decryptJobHeaders({


Pipeline methods no longer need to have return value destructured, because they (usually) only return what is necessary. They no longer need to return a caravan of objects to get carried down the pipeline.

This is an effect of main change (1)

joelgriffith · 2019-12-12T22:15:17Z

x-pack/legacy/plugins/reporting/export_types/common/lib/screenshots/index.ts

-              logger.debug(
-                'waiting for elements or items count attribute; or not found to interrupt'
-              );
+    return Rx.from(urls).pipe(


Any reason to go with concatMap over mergeMap? I'm assuming it has to do with the utility return signature change (I did read over both in the rxjs docs)

I've read over the docs quite a bit too, and I'll admit it is hard to understand how the difference will matter to us. The subscription pipeline handles one incoming event, and then is destroyed, so there are no incoming subsequent events that need to be in sync with the first one.

I think that is the reason I switched from mergeMap to concatMap - it seemed to make more sense with the fact that the pipeline is used for a single event, and then the subscription is cancelled. The next event gets a new subscription.

Thinking about it now, having the breakup of multiple mergeMap or concatMap is really about syntax and style. There could be just the concatMap on line 44 and it chains the async functions together and forms a payload, or cancels itself from the exit$.

Given how hard it was to try to justify, I think another change is due in this PR to simplify things here.

I think there is some justifiable reasoning in keeping the pipeline separated using observable operators, instead of having a larger function with multiple async steps. The observable pipeline is cancelable, and we're racing the pipeline against the exit$ observable.

joelgriffith

Looks good, going to pull and test shortly

tsullivan · 2020-01-06T16:36:45Z

@elasticmachine merge upstream

kibanamachine · 2020-01-06T18:08:30Z

💚 Build Succeeded

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

History

💚 Build #17903 succeeded a4123a3
💚 Build #17867 succeeded 41faac6
💔 Build #17833 failed d680066
💔 Build #17662 failed 8f6c013
💔 Build #15282 failed 2aa08df

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

…elastic#48588) * Multi-url pass to screenshotsObservable * Restore "first" operator * max attempt = 1 for testing * cleanup debug * restore more concatMap Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…#48588) (#54075) * Multi-url pass to screenshotsObservable * Restore "first" operator * max attempt = 1 for testing * cleanup debug * restore more concatMap Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

tsullivan force-pushed the reporting/screenshots-multi-url branch 3 times, most recently from 95765da to 409e56a Compare October 25, 2019 22:55

tsullivan mentioned this pull request Dec 2, 2019

[Reporting/Screenshots] add error for no shared items found on the page #52022

Merged

1 task

tsullivan force-pushed the reporting/screenshots-multi-url branch from 409e56a to 69614aa Compare December 2, 2019 22:01

tsullivan mentioned this pull request Dec 3, 2019

[Reporting] Convert Export Type Definitions to Typescript #51643

Merged

1 task

tsullivan force-pushed the reporting/screenshots-multi-url branch 6 times, most recently from dfe877d to 493598d Compare December 10, 2019 20:22

tsullivan force-pushed the reporting/screenshots-multi-url branch from 493598d to 7b40c6b Compare December 12, 2019 21:55

elastic deleted a comment from elasticmachine Dec 12, 2019

tsullivan commented Dec 12, 2019

View reviewed changes

tsullivan marked this pull request as ready for review December 12, 2019 22:02

tsullivan requested a review from joelgriffith December 12, 2019 22:03

tsullivan added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Dec 12, 2019

tsullivan added v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes review labels Dec 12, 2019

Multi-url pass to screenshotsObservable

2aa08df

tsullivan force-pushed the reporting/screenshots-multi-url branch from 7b40c6b to 2aa08df Compare December 12, 2019 22:10

joelgriffith reviewed Dec 12, 2019

View reviewed changes

joelgriffith approved these changes Dec 12, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

tsullivan added 6 commits January 2, 2020 14:47

Merge branch 'master' into reporting/screenshots-multi-url

8f6c013

Merge branch 'master' into reporting/screenshots-multi-url

d680066

Restore "first" operator

a5b1989

max attempt = 1 for testing

e6da1e9

cleanup debug

41faac6

Merge branch 'master' into reporting/screenshots-multi-url

a4123a3

elasticmachine and others added 2 commits January 6, 2020 08:36

Merge branch 'master' into reporting/screenshots-multi-url

67047a0

restore more concatMap

b31dcc8

tsullivan merged commit fa8da7c into elastic:master Jan 6, 2020

tsullivan deleted the reporting/screenshots-multi-url branch January 6, 2020 21:34

tsullivan mentioned this pull request Jan 6, 2020

[7.x] [Reporting/PDF] Refactor screenshot pipeline for multi-url by default (#48588) #54075

Merged

tsullivan mentioned this pull request Jan 7, 2020

[Reporting] Lots of room for improvement when there are multiple relativeUrls in the POST URL #41299

Closed

tsullivan mentioned this pull request Feb 5, 2020

[Reporting] Fix screenshot pipeline for multi-page #56928

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting/PDF] Refactor screenshot pipeline for multi-url by default #48588

[Reporting/PDF] Refactor screenshot pipeline for multi-url by default #48588

tsullivan commented Oct 17, 2019 •

edited

Loading

tsullivan Dec 12, 2019

tsullivan Jan 3, 2020

tsullivan Dec 12, 2019

tsullivan Dec 12, 2019

tsullivan Dec 12, 2019

tsullivan Dec 12, 2019

tsullivan Dec 12, 2019 •

edited

Loading

joelgriffith Dec 12, 2019

tsullivan Jan 2, 2020

tsullivan Jan 3, 2020

joelgriffith left a comment

This comment has been minimized.

tsullivan commented Jan 6, 2020

kibanamachine commented Jan 6, 2020

[Reporting/PDF] Refactor screenshot pipeline for multi-url by default #48588

[Reporting/PDF] Refactor screenshot pipeline for multi-url by default #48588

Conversation

tsullivan commented Oct 17, 2019 • edited Loading

Summary

Checklist

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

tsullivan Dec 12, 2019 • 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

joelgriffith left a comment

Choose a reason for hiding this comment

This comment has been minimized.

tsullivan commented Jan 6, 2020

kibanamachine commented Jan 6, 2020

💚 Build Succeeded

History

tsullivan commented Oct 17, 2019 •

edited

Loading

tsullivan Dec 12, 2019 •

edited

Loading