Adds some new configuration options and remove browser timings (grafa…

…na#123) Configuration for rendering args and dumpio setting to ease investigation of problems. Removes browser timings due to possibly async issues.
lasertiger · Mar 30, 2020 · 23f90be · 23f90be
1 parent 5ac51b2
commit 23f90be
Show file tree

Hide file tree

Showing 12 changed files with 111 additions and 192 deletions.
diff --git a/default.json b/default.json
@@ -19,12 +19,15 @@
     "timezone": null,
     "chromeBin": null,
     "ignoresHttpsErrors": false,
-    "timingMetrics": false,
     "mode": "default",
     "clustering": {
       "mode": "browser",
       "maxConcurrency": 5
     },
-    "verboseLogging": false
+    "verboseLogging": false,
+    "dumpio": false,
+    "args": [
+      "--no-sandbox"
+    ]
   }
 }
diff --git a/dev.json b/dev.json
@@ -19,12 +19,16 @@
     "timezone": null,
     "chromeBin": null,
     "ignoresHttpsErrors": false,
-    "timingMetrics": false,
     "mode": "default",
     "clustering": {
       "mode": "browser",
       "maxConcurrency": 5
     },
-    "verboseLogging": false
+    "verboseLogging": false,
+    "dumpio": true,
+    "args": [
+      "--no-sandbox",
+      "--disable-setuid-sandbox"
+    ]
   }
 }
diff --git a/src/app.ts b/src/app.ts
@@ -4,16 +4,14 @@ import * as _ from 'lodash';
 import { GrpcPlugin } from './plugin/grpc-plugin';
 import { HttpServer } from './service/http-server';
 import { ConsoleLogger, PluginLogger } from './logger';
-import { NoOpBrowserTiming, createBrowser } from './browser';
+import { createBrowser } from './browser';
 import * as minimist from 'minimist';
 import { defaultPluginConfig, defaultServiceConfig, readJSONFileSync, PluginConfig, ServiceConfig } from './config';
-import { MetricsBrowserTimings } from './metrics_browser_timings';
 
 async function main() {
   const argv = minimist(process.argv.slice(2));
   const env = Object.assign({}, process.env);
   const command = argv._[0];
-  let timings = new NoOpBrowserTiming();
 
   if (command === undefined) {
     const config: PluginConfig = defaultPluginConfig;
@@ -29,7 +27,7 @@ async function main() {
     }
 
     const logger = new PluginLogger();
-    const browser = createBrowser(config.rendering, logger, timings);
+    const browser = createBrowser(config.rendering, logger);
     const plugin = new GrpcPlugin(config, logger, browser);
     plugin.start();
   } else if (command === 'server') {
@@ -47,12 +45,8 @@ async function main() {
 
     populateServiceConfigFromEnv(config, env);
 
-    if (config.service.metrics.enabled && config.rendering.timingMetrics) {
-      timings = new MetricsBrowserTimings();
-    }
-
     const logger = new ConsoleLogger(config.service.logging);
-    const browser = createBrowser(config.rendering, logger, timings);
+    const browser = createBrowser(config.rendering, logger);
     const server = new HttpServer(config, logger, browser);
 
     await server.start();
@@ -136,4 +130,18 @@ function populateServiceConfigFromEnv(config: ServiceConfig, env: NodeJS.Process
   if (env['RENDERING_VERBOSE_LOGGING']) {
     config.rendering.verboseLogging = env['RENDERING_VERBOSE_LOGGING'] === 'true';
   }
+
+  if (env['RENDERING_DUMPIO']) {
+    config.rendering.dumpio = env['RENDERING_DUMPIO'] === 'true';
+  }
+
+  if (env['RENDERING_ARGS']) {
+    const args = env['RENDERING_ARGS'] as string;
+    if (args.length > 0) {
+      const argsList = args.split(',');
+      if (argsList.length > 0) {
+        config.rendering.args = argsList;
+      }
+    }
+  }
 }
diff --git a/src/browser/browser.ts b/src/browser/browser.ts
@@ -21,43 +21,37 @@ export interface RenderResponse {
   filePath: string;
 }
 
-export interface BrowserTimings {
-  launch(callback: () => Promise<PuppeteerBrowser>): Promise<PuppeteerBrowser>;
-  newPage(callback: () => Promise<Page>): Promise<Page>;
-  navigate(callback: () => Promise<void>): Promise<void>;
-  panelsRendered(callback: () => Promise<void>): Promise<void>;
-  screenshot(callback: () => Promise<void>): Promise<void>;
-}
-
-export class NoOpBrowserTiming {
-  async launch(callback: () => Promise<PuppeteerBrowser>) {
-    return await callback();
-  }
-
-  async newPage(callback: () => Promise<void>) {
-    return await callback();
-  }
-
-  async navigate(callback: () => Promise<void>) {
-    return await callback();
-  }
-
-  async panelsRendered(callback: () => Promise<void>) {
-    return await callback();
-  }
-
-  async screenshot(callback: () => Promise<void>) {
-    return await callback();
-  }
-}
-
 export class Browser {
-  constructor(protected config: RenderingConfig, protected log: Logger, protected timings: BrowserTimings) {}
+  constructor(protected config: RenderingConfig, protected log: Logger) {
+    this.log.info(
+      'Browser initiated',
+      'chromeBin',
+      this.config.chromeBin,
+      'ignoresHttpsErrors',
+      this.config.ignoresHttpsErrors,
+      'timezone',
+      this.config.timezone,
+      'args',
+      this.config.args,
+      'dumpio',
+      this.config.dumpio,
+      'verboseLogging',
+      this.config.verboseLogging
+    );
+  }
 
   async getBrowserVersion(): Promise<string> {
-    const launcherOptions = this.getLauncherOptions({});
-    const browser = await puppeteer.launch(launcherOptions);
-    return browser.version();
+    let browser;
+
+    try {
+      const launcherOptions = this.getLauncherOptions({});
+      browser = await puppeteer.launch(launcherOptions);
+      return await browser.version();
+    } finally {
+      if (browser) {
+        await browser.close();
+      }
+    }
   }
 
   async start(): Promise<void> {}
@@ -84,7 +78,8 @@ export class Browser {
     const launcherOptions: any = {
       env: env,
       ignoreHTTPSErrors: this.config.ignoresHttpsErrors,
-      args: ['--no-sandbox'],
+      dumpio: this.config.dumpio,
+      args: this.config.args,
     };
 
     if (this.config.chromeBin) {
@@ -101,18 +96,8 @@ export class Browser {
     try {
       this.validateOptions(options);
       const launcherOptions = this.getLauncherOptions(options);
-
-      browser = await this.timings.launch(
-        async () =>
-          // launch browser
-          await puppeteer.launch(launcherOptions)
-      );
-      page = await this.timings.newPage(
-        async () =>
-          // open a new page
-          await browser.newPage()
-      );
-
+      browser = await puppeteer.launch(launcherOptions);
+      page = await browser.newPage();
       this.addPageListeners(page);
 
       return await this.takeScreenshot(page, options);
@@ -139,32 +124,22 @@ export class Browser {
       domain: options.domain,
     });
     await page.mouse.move(options.width, options.height);
-
-    await this.timings.navigate(async () => {
-      // wait until all data was loaded
-      await page.goto(options.url, { waitUntil: 'networkidle0' });
-    });
-
-    await this.timings.panelsRendered(async () => {
-      // wait for all panels to render
-      await page.waitForFunction(
-        () => {
-          const panelCount = document.querySelectorAll('.panel').length || document.querySelectorAll('.panel-container').length;
-          return (window as any).panelsRendered >= panelCount;
-        },
-        {
-          timeout: options.timeout * 1000,
-        }
-      );
-    });
+    await page.goto(options.url, { waitUntil: 'networkidle0' });
+    await page.waitForFunction(
+      () => {
+        const panelCount = document.querySelectorAll('.panel').length || document.querySelectorAll('.panel-container').length;
+        return (window as any).panelsRendered >= panelCount;
+      },
+      {
+        timeout: options.timeout * 1000,
+      }
+    );
 
     if (!options.filePath) {
       options.filePath = uniqueFilename(os.tmpdir()) + '.png';
     }
 
-    await this.timings.screenshot(async () => {
-      await page.screenshot({ path: options.filePath });
-    });
+    await page.screenshot({ path: options.filePath });
 
     return { filePath: options.filePath };
   }

diff --git a/src/browser/clustered.ts b/src/browser/clustered.ts
@@ -1,5 +1,5 @@
 import { Cluster } from 'puppeteer-cluster';
-import { Browser, RenderResponse, BrowserTimings, RenderOptions } from './browser';
+import { Browser, RenderResponse, RenderOptions } from './browser';
 import { Logger } from '../logger';
 import { RenderingConfig, ClusteringConfig } from '../config';
 
@@ -8,8 +8,8 @@ export class ClusteredBrowser extends Browser {
   clusteringConfig: ClusteringConfig;
   concurrency: number;
 
-  constructor(config: RenderingConfig, log: Logger, timings: BrowserTimings) {
-    super(config, log, timings);
+  constructor(config: RenderingConfig, log: Logger) {
+    super(config, log);
 
     this.clusteringConfig = config.clustering;
     this.concurrency = Cluster.CONCURRENCY_BROWSER;

diff --git a/src/browser/index.ts b/src/browser/index.ts
@@ -1,21 +1,21 @@
 import { RenderingConfig } from '../config';
 import { Logger } from '../logger';
-import { Browser, BrowserTimings, NoOpBrowserTiming } from './browser';
+import { Browser } from './browser';
 import { ClusteredBrowser } from './clustered';
 import { ReusableBrowser } from './reusable';
 
-export function createBrowser(config: RenderingConfig, log: Logger, timings: BrowserTimings): Browser {
+export function createBrowser(config: RenderingConfig, log: Logger): Browser {
   if (config.mode === 'clustered') {
     log.info('using clustered browser', 'mode', config.clustering.mode, 'maxConcurrency', config.clustering.maxConcurrency);
-    return new ClusteredBrowser(config, log, timings);
+    return new ClusteredBrowser(config, log);
   }
 
   if (config.mode === 'reusable') {
     log.info('using reusable browser');
-    return new ReusableBrowser(config, log, timings);
+    return new ReusableBrowser(config, log);
   }
 
-  return new Browser(config, log, timings);
+  return new Browser(config, log);
 }
 
-export { Browser, NoOpBrowserTiming };
+export { Browser };
diff --git a/src/browser/reusable.ts b/src/browser/reusable.ts
@@ -1,23 +1,18 @@
 import * as puppeteer from 'puppeteer';
-import { Browser, RenderResponse, BrowserTimings, RenderOptions } from './browser';
+import { Browser, RenderResponse, RenderOptions } from './browser';
 import { Logger } from '../logger';
 import { RenderingConfig } from '../config';
 
 export class ReusableBrowser extends Browser {
   browser: puppeteer.Browser;
 
-  constructor(config: RenderingConfig, log: Logger, timings: BrowserTimings) {
-    super(config, log, timings);
+  constructor(config: RenderingConfig, log: Logger) {
+    super(config, log);
   }
 
   async start(): Promise<void> {
     const launcherOptions = this.getLauncherOptions({});
-
-    this.browser = await this.timings.launch(
-      async () =>
-        // launch browser
-        await puppeteer.launch(launcherOptions)
-    );
+    this.browser = await puppeteer.launch(launcherOptions);
   }
 
   async render(options: RenderOptions): Promise<RenderResponse> {
@@ -27,12 +22,7 @@ export class ReusableBrowser extends Browser {
     try {
       this.validateOptions(options);
       context = await this.browser.createIncognitoBrowserContext();
-
-      page = await this.timings.newPage(
-        async () =>
-          // open a new page
-          await context.newPage()
-      );
+      page = await context.newPage();
 
       if (options.timezone) {
         // set timezone

diff --git a/src/config.ts b/src/config.ts
@@ -9,10 +9,11 @@ export interface RenderingConfig {
   timezone?: string;
   chromeBin?: string;
   ignoresHttpsErrors: boolean;
-  timingMetrics: boolean;
   mode: string;
   clustering: ClusteringConfig;
   verboseLogging: boolean;
+  dumpio: boolean;
+  args: string[];
 }
 
 export interface MetricsConfig {
@@ -56,13 +57,14 @@ const defaultRenderingConfig: RenderingConfig = {
   timezone: undefined,
   chromeBin: undefined,
   ignoresHttpsErrors: false,
-  timingMetrics: false,
   mode: 'default',
   clustering: {
     mode: 'browser',
     maxConcurrency: 5,
   },
   verboseLogging: false,
+  dumpio: false,
+  args: ['--no-sandbox'],
 };
 
 export const defaultServiceConfig: ServiceConfig = {