Retry the entire screenshotStitcher call (elastic#20770)

* Retry the entire screenshotStitcher call

* Go back to a single run

* Only retry for this specific error.  Post more information including the git issue link

x-pack/plugins/reporting/server/browsers/chromium/driver/index.js

@@ -10,7 +10,7 @@ import moment from 'moment';
 import { promisify, delay } from 'bluebird';
 import { transformFn } from './transform_fn';
 import { ignoreSSLErrorsBehavior } from './ignore_ssl_errors';
-import { screenshotStitcher } from './screenshot_stitcher';
+import { screenshotStitcher, CapturePngSizeError } from './screenshot_stitcher';
 
 export class HeadlessChromiumDriver {
   constructor(client, { maxScreenshotDimension, logger }) {
@@ -84,16 +84,34 @@ export class HeadlessChromiumDriver {
       };
     }
 
-    return await screenshotStitcher(outputClip, this._zoom, this._maxScreenshotDimension, async screenshotClip => {
-      const { data } = await Page.captureScreenshot({
-        clip: {
-          ...screenshotClip,
-          scale: 1
+    // Wrapping screenshotStitcher function call in a retry because of this bug:
+    // https://github.com/elastic/kibana/issues/19563. The reason was never found - it only appeared on ci and
+    // debug logic right after Page.captureScreenshot to ensure the correct size made the bug disappear.
+    let retryCount = 0;
+    const MAX_RETRIES = 3;
+    while (true) {
+      try {
+        return await screenshotStitcher(outputClip, this._zoom, this._maxScreenshotDimension, async screenshotClip => {
+          const { data } = await Page.captureScreenshot({
+            clip: {
+              ...screenshotClip,
+              scale: 1
+            }
+          });
+          this._logger.debug(`Captured screenshot clip ${JSON.stringify(screenshotClip)}`);
+          return data;
+        }, this._logger);
+      } catch (error) {
+        const isCapturePngSizeError = error instanceof CapturePngSizeError;
+        if (!isCapturePngSizeError || retryCount === MAX_RETRIES) {
+          throw error;
+        } else {
+          this._logger.error(error.message);
+          this._logger.error('Trying again...');
+          retryCount++;
         }
-      });
-      this._logger.debug(`Captured screenshot clip ${JSON.stringify(screenshotClip)}`);
-      return data;
-    }, this._logger);
+      }
+    }
   }
 
   async _writeData(writePath, base64EncodedData) {

x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/combine.ts

@@ -13,6 +13,20 @@ import { ObservableInput } from 'rxjs';
 import { map, mergeMap, reduce, switchMap, tap, toArray } from 'rxjs/operators';
 import { Logger, Screenshot, Size } from './types';
 
+export class CapturePngSizeError extends Error {
+  constructor(
+    actualSize: { width: number; height: number },
+    expectedSize: { width: number; height: number }
+  ) {
+    super();
+    this.message =
+      `Capture PNG size error. Please visit https://github.com/elastic/kibana/issues/19563 to report this error. ` +
+      `Screenshot captured of size ${actualSize.width}x${
+        actualSize.height
+      } was not of expected size ${expectedSize.width}x${expectedSize.height}`;
+  }
+}
+
 // if we're only given one screenshot, and it matches the given size
 // we're going to skip the combination and just use it
 const canUseFirstScreenshot = (
@@ -69,14 +83,9 @@ export function $combine(
           png.width !== screenshot.rectangle.width ||
           png.height !== screenshot.rectangle.height
         ) {
-          const errorMessage = `Screenshot captured with width:${png.width} and height: ${
-            png.height
-          }) is not of expected width: ${screenshot.rectangle.width} and height: ${
-            screenshot.rectangle.height
-          }`;
-
-          logger.error(errorMessage);
-          throw new Error(errorMessage);
+          const error = new CapturePngSizeError(png, screenshot.rectangle);
+          logger.error(error.message);
+          throw error;
         }
         return { screenshot, png };
       }

x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/index.ts

@@ -4,88 +4,5 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { map, mergeMap, switchMap, toArray } from 'rxjs/operators';
-import { $combine } from './combine';
-import { $getClips } from './get_clips';
-import { Logger, Rectangle, Screenshot } from './types';
-
-const scaleRect = (rect: Rectangle, scale: number): Rectangle => {
-  return {
-    height: rect.height * scale,
-    width: rect.width * scale,
-    x: rect.x * scale,
-    y: rect.y * scale,
-  };
-};
-
-/**
- * Returns a stream of data that should be of the size outputClip.width * zoom x outputClip.height * zoom
- * @param outputClip - The dimensions the final image should take.
- * @param zoom - Determines the resolution want the final screenshot to take.
- * @param maxDimensionPerClip - The maximimum dimension, in any direction (width or height) that we should allow per
- * screenshot clip. If zoom is 10 and maxDimensionPerClip is anything less than or
- * equal to 10, each clip taken, before  being zoomed in, should be no bigger than 1 x 1.
- * If zoom is 10 and maxDimensionPerClip is 20, then each clip taken before being zoomed in should be no bigger than 2 x 2.
- * @param captureScreenshotFn - a function to take a screenshot from the page using the dimensions given. The data
- * returned should have dimensions not of the clip passed in, but of the clip passed in multiplied by zoom.
- * @param logger
- */
-export async function screenshotStitcher(
-  outputClip: Rectangle,
-  zoom: number,
-  maxDimensionPerClip: number,
-  captureScreenshotFn: (rect: Rectangle) => Promise<string>,
-  logger: Logger
-): Promise<string> {
-  // We have to divide the max by the zoom because we will be multiplying each clip's dimensions
-  // later by zoom, and we don't want those dimensions to end up larger than max.
-  const maxDimensionBeforeZoom = Math.ceil(maxDimensionPerClip / zoom);
-  const screenshotClips$ = $getClips(outputClip, maxDimensionBeforeZoom);
-
-  const screenshots$ = screenshotClips$.pipe(
-    mergeMap(clip => captureScreenshotFn(clip), (clip, data) => ({ clip, data }), 1)
-  );
-
-  // when we take the screenshots we don't have to scale the rects
-  // but the PNGs don't know about the zoom, so we have to scale them
-  const screenshotPngRects$ = screenshots$.pipe(
-    map(({ data, clip }) => {
-      // At this point we don't care about the offset - the screenshots have been taken.
-      // We need to adjust the x & y values so they all are adjusted for the top-left most
-      // clip being at 0, 0.
-      const x = clip.x - outputClip.x;
-      const y = clip.y - outputClip.y;
-
-      const scaledScreenshotRects = scaleRect(
-        {
-          height: clip.height,
-          width: clip.width,
-          x,
-          y,
-        },
-        zoom
-      );
-      return {
-        data,
-        rectangle: scaledScreenshotRects,
-      };
-    })
-  );
-
-  const scaledOutputRects = scaleRect(outputClip, zoom);
-  return screenshotPngRects$
-    .pipe(
-      toArray(),
-      switchMap<Screenshot[], string>((screenshots: Screenshot[]) =>
-        $combine(
-          screenshots,
-          {
-            height: scaledOutputRects.height,
-            width: scaledOutputRects.width,
-          },
-          logger
-        )
-      )
-    )
-    .toPromise<string>();
-}
+export { screenshotStitcher } from './screenshot_stitcher';
+export { CapturePngSizeError } from './combine';

x-pack/plugins/reporting/server/browsers/chromium/driver/screenshot_stitcher/screenshot_stitcher.ts

@@ -0,0 +1,91 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { map, mergeMap, switchMap, toArray } from 'rxjs/operators';
+import { $combine } from './combine';
+import { $getClips } from './get_clips';
+import { Logger, Rectangle, Screenshot } from './types';
+
+const scaleRect = (rect: Rectangle, scale: number): Rectangle => {
+  return {
+    height: rect.height * scale,
+    width: rect.width * scale,
+    x: rect.x * scale,
+    y: rect.y * scale,
+  };
+};
+
+/**
+ * Returns a stream of data that should be of the size outputClip.width * zoom x outputClip.height * zoom
+ * @param outputClip - The dimensions the final image should take.
+ * @param zoom - Determines the resolution want the final screenshot to take.
+ * @param maxDimensionPerClip - The maximimum dimension, in any direction (width or height) that we should allow per
+ * screenshot clip. If zoom is 10 and maxDimensionPerClip is anything less than or
+ * equal to 10, each clip taken, before  being zoomed in, should be no bigger than 1 x 1.
+ * If zoom is 10 and maxDimensionPerClip is 20, then each clip taken before being zoomed in should be no bigger than 2 x 2.
+ * @param captureScreenshotFn - a function to take a screenshot from the page using the dimensions given. The data
+ * returned should have dimensions not of the clip passed in, but of the clip passed in multiplied by zoom.
+ * @param logger
+ */
+export async function screenshotStitcher(
+  outputClip: Rectangle,
+  zoom: number,
+  maxDimensionPerClip: number,
+  captureScreenshotFn: (rect: Rectangle) => Promise<string>,
+  logger: Logger
+): Promise<string> {
+  // We have to divide the max by the zoom because we will be multiplying each clip's dimensions
+  // later by zoom, and we don't want those dimensions to end up larger than max.
+  const maxDimensionBeforeZoom = Math.ceil(maxDimensionPerClip / zoom);
+  const screenshotClips$ = $getClips(outputClip, maxDimensionBeforeZoom);
+
+  const screenshots$ = screenshotClips$.pipe(
+    mergeMap(clip => captureScreenshotFn(clip), (clip, data) => ({ clip, data }), 1)
+  );
+
+  // when we take the screenshots we don't have to scale the rects
+  // but the PNGs don't know about the zoom, so we have to scale them
+  const screenshotPngRects$ = screenshots$.pipe(
+    map(({ data, clip }) => {
+      // At this point we don't care about the offset - the screenshots have been taken.
+      // We need to adjust the x & y values so they all are adjusted for the top-left most
+      // clip being at 0, 0.
+      const x = clip.x - outputClip.x;
+      const y = clip.y - outputClip.y;
+
+      const scaledScreenshotRects = scaleRect(
+        {
+          height: clip.height,
+          width: clip.width,
+          x,
+          y,
+        },
+        zoom
+      );
+      return {
+        data,
+        rectangle: scaledScreenshotRects,
+      };
+    })
+  );
+
+  const scaledOutputRects = scaleRect(outputClip, zoom);
+  return screenshotPngRects$
+    .pipe(
+      toArray(),
+      switchMap<Screenshot[], string>((screenshots: Screenshot[]) =>
+        $combine(
+          screenshots,
+          {
+            height: scaledOutputRects.height,
+            width: scaledOutputRects.width,
+          },
+          logger
+        )
+      )
+    )
+    .toPromise<string>();
+}

x-pack/scripts/functional_tests.js

@@ -6,9 +6,8 @@
 
 require('@kbn/plugin-helpers').babelRegister();
 require('@kbn/test').runTestsCli([
-  // Uncomment when https://github.com/elastic/kibana/issues/19563 is resolved.
-  // require.resolve('../test/reporting/configs/chromium_api.js'),
-  // require.resolve('../test/reporting/configs/chromium_functional.js'),
+  require.resolve('../test/reporting/configs/chromium_api.js'),
+  require.resolve('../test/reporting/configs/chromium_functional.js'),
   require.resolve('../test/reporting/configs/phantom_api.js'),
   require.resolve('../test/reporting/configs/phantom_functional.js'),
   require.resolve('../test/functional/config.js'),

x-pack/test/reporting/configs/chromium_api.js

@@ -21,7 +21,6 @@ export default async function ({ readConfigFile }) {
       serverArgs: [
         ...reportingApiConfig.kbnTestServer.serverArgs,
         `--xpack.reporting.capture.browser.type=chromium`,
-        `--logging.verbose=true`,
       ],
     },
   };

x-pack/test/reporting/configs/chromium_functional.js

@@ -21,7 +21,6 @@ export default async function ({ readConfigFile }) {
       serverArgs: [
         ...functionalConfig.kbnTestServer.serverArgs,
         `--xpack.reporting.capture.browser.type=chromium`,
-        `--logging.verbose=true`,
       ],
     },
   };