From 1258fcc87786c2497452f21c403270a82eb8a4f1 Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Tue, 17 Dec 2024 10:58:50 +0100 Subject: [PATCH 1/9] start adding abortcontroller --- src/browser/browser.ts | 101 ++++++++++++++++++++++++++--------- src/browser/clustered.ts | 26 +++++---- src/browser/error.ts | 6 +++ src/browser/reusable.ts | 28 ++++++++-- src/plugin/v2/grpc_plugin.ts | 11 ++-- src/service/http-server.ts | 60 ++++++++++++--------- 6 files changed, 166 insertions(+), 66 deletions(-) create mode 100644 src/browser/error.ts diff --git a/src/browser/browser.ts b/src/browser/browser.ts index 49755391..01acdc09 100644 --- a/src/browser/browser.ts +++ b/src/browser/browser.ts @@ -9,6 +9,7 @@ import * as Jimp from 'jimp'; import { Logger } from '../logger'; import { RenderingConfig } from '../config/rendering'; import { ImageRenderOptions, RenderOptions } from '../types'; +import { TimeoutError } from './error'; import { getPDFOptionsFromURL } from './pdf'; export interface Metrics { @@ -130,7 +131,7 @@ export class Browser { ignoreHTTPSErrors: this.config.ignoresHttpsErrors, dumpio: this.config.dumpio, args: this.config.args, - defaultViewport: null + defaultViewport: null, }; if (this.config.chromeBin) { @@ -176,7 +177,7 @@ export class Browser { page.on('dialog', acceptBeforeUnload); } - async scrollToLoadAllPanels(page: puppeteer.Page, options: ImageRenderOptions): Promise { + async scrollToLoadAllPanels(page: puppeteer.Page, options: ImageRenderOptions, signal: AbortSignal): Promise { const scrollElementSelector = await page.evaluate(() => { const pageScrollbarIDSelector = '#page-scrollbar'; // the page-scrollbar ID was introduced in Grafana 11.1.0 @@ -190,19 +191,19 @@ export class Browser { 'main > div > div > div > [class*="scrollbar-view"]', 'main > div > div > div > div > [class*="scrollbar-view"]', 'main > div > div > div > div > div > [class*="scrollbar-view"]', - ] + ]; const pageScrollbarSelector = [pageScrollbarIDSelector, ...fallbackSelectors].join(','); const hasPageScrollbar = Boolean(document.querySelector(pageScrollbarSelector)); return hasPageScrollbar ? pageScrollbarSelector : 'body'; }); const scrollDelay = options.scrollDelay ?? 500; - await page.waitForSelector(scrollElementSelector); + await page.waitForSelector(scrollElementSelector, { signal }); const heights: { dashboard?: { scroll: number; client: number }; body: { client: number } } = await page.evaluate((scrollElementSelector) => { const body = { client: document.body.clientHeight }; const scrollableElement = document.querySelector(scrollElementSelector); if (!scrollableElement) { - this.log.debug('no scrollable element detected, returning without scrolling') + this.log.debug('no scrollable element detected, returning without scrolling'); return { body, }; @@ -221,7 +222,13 @@ export class Browser { } if (heights.dashboard.scroll <= heights.dashboard.client) { - this.log.debug('client height greather or equal than scroll height, no scrolling', 'scrollHeight', heights.dashboard.scroll, 'clientHeight', heights.dashboard.client) + this.log.debug( + 'client height greather or equal than scroll height, no scrolling', + 'scrollHeight', + heights.dashboard.scroll, + 'clientHeight', + heights.dashboard.client + ); return { scrolled: false, }; @@ -237,16 +244,14 @@ export class Browser { : document.querySelector(scrollElementSelector)?.scrollBy(0, scrollByHeight); }, heights.dashboard.client, - scrollElementSelector, + scrollElementSelector ); await new Promise((executor) => setTimeout(executor, scrollDelay)); } await page.evaluate((scrollElementSelector) => { - scrollElementSelector === 'body' - ? window.scrollTo(0, 0) - : document.querySelector(scrollElementSelector)?.scrollTo(0, 0); + scrollElementSelector === 'body' ? window.scrollTo(0, 0) : document.querySelector(scrollElementSelector)?.scrollTo(0, 0); }, scrollElementSelector); // Header height will be equal to 0 in Kiosk mode @@ -257,7 +262,7 @@ export class Browser { }; } - async render(options: ImageRenderOptions): Promise { + async render(options: ImageRenderOptions, signal: AbortSignal): Promise { let browser: puppeteer.Browser | undefined = undefined; let page: puppeteer.Page | undefined = undefined; @@ -272,9 +277,19 @@ export class Browser { return browser!.newPage(); }, 'newPage'); + signal.addEventListener('abort', () => { + if (page) { + this.removePageListeners(page); + page.close(); + } + if (browser) { + browser.close(); + } + }); + this.addPageListeners(page); - return await this.takeScreenshot(page, options); + return await this.takeScreenshot(page, options, signal); } finally { if (page) { this.removePageListeners(page); @@ -294,7 +309,7 @@ export class Browser { }); }; - async takeScreenshot(page: puppeteer.Page, options: ImageRenderOptions): Promise { + async takeScreenshot(page: puppeteer.Page, options: ImageRenderOptions, signal: AbortSignal): Promise { try { await this.withTimingMetrics(async () => { if (this.config.verboseLogging) { @@ -319,15 +334,20 @@ export class Browser { return page.mouse.move(+options.width, +options.height); }, 'prepare'); - await this.withTimingMetrics(() => { + // await new Promise((resolve) => setTimeout(resolve, 8000)); + // this.log.error('signal status', 'is aborted', signal.aborted); + + const res = await this.withTimingMetrics(() => { if (this.config.verboseLogging) { this.log.debug('Navigating and waiting for all network requests to finish', 'url', options.url); } - return page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000 }); + return page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal }); }, 'navigate'); + this.log.error('signal status', 'is aborted', signal.aborted, 'res', res?.url); } catch (err) { this.log.error('Error while trying to prepare page for screenshot', 'url', options.url, 'err', err.stack); + throw new TimeoutError('navigate'); } let scrollResult: DashboardScrollingResult = { @@ -337,10 +357,11 @@ export class Browser { if (options.fullPageImage) { try { scrollResult = await this.withTimingMetrics(() => { - return this.scrollToLoadAllPanels(page, options); + return this.scrollToLoadAllPanels(page, options, signal); }, 'dashboardScrolling'); } catch (err) { this.log.error('Error while scrolling to load all panels', 'url', options.url, 'err', err.stack); + throw new TimeoutError('dashboardScrolling'); } } @@ -352,10 +373,27 @@ export class Browser { this.log.debug('Waiting for dashboard/panel to load', 'timeout', `${options.timeout}s`); } - await waitForQueriesAndVisualizations(page, options); + await waitForQueriesAndVisualizations(page, options, signal); }, 'panelsRendered'); } catch (err) { - this.log.error('Error while waiting for the panels to load', 'url', options.url, 'err', err.stack); + const windowInfo = await page.evaluate(() => { + return { + usingScenes: window.__grafanaSceneContext !== null, + queryCount: window.__grafanaRunningQueryCount, + }; + }); + this.log.error( + 'Error while waiting for the panels to load', + 'url', + options.url, + 'usingScenes', + windowInfo.usingScenes, + 'queryCount', + windowInfo.queryCount, + 'err', + err.stack + ); + throw new TimeoutError('panelsRendered'); } if (!options.filePath) { @@ -422,7 +460,7 @@ export class Browser { return { filePath: options.filePath }; } - async renderCSV(options: RenderOptions): Promise { + async renderCSV(options: RenderOptions, signal: AbortSignal): Promise { let browser; let page: any; @@ -431,9 +469,20 @@ export class Browser { const launcherOptions = this.getLauncherOptions(options); browser = await puppeteer.launch(launcherOptions); page = await browser.newPage(); + + signal.addEventListener('abort', () => { + if (page) { + this.removePageListeners(page); + page.close(); + } + if (browser) { + browser.close(); + } + }); + this.addPageListeners(page); - return await this.exportCSV(page, options); + return await this.exportCSV(page, options, signal); } finally { if (page) { this.removePageListeners(page); @@ -445,7 +494,7 @@ export class Browser { } } - async exportCSV(page: any, options: RenderOptions): Promise { + async exportCSV(page: any, options: RenderOptions, signal: AbortSignal): Promise { await this.preparePage(page, options); await this.setTimezone(page, options); @@ -465,7 +514,7 @@ export class Browser { this.log.debug('Navigating and waiting for all network requests to finish', 'url', options.url); } - await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000 }); + await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal }); if (this.config.verboseLogging) { this.log.debug('Waiting for download to end'); @@ -480,7 +529,7 @@ export class Browser { } if (downloadFilePath === '') { - throw new Error(`Timeout exceeded while waiting for download to end`); + throw new TimeoutError(`CSV download`); } await watcher.close(); @@ -595,7 +644,7 @@ declare global { } } -async function waitForQueriesAndVisualizations(page: puppeteer.Page, options: ImageRenderOptions) { +async function waitForQueriesAndVisualizations(page: puppeteer.Page, options: ImageRenderOptions, signal: AbortSignal) { await page.waitForFunction( (isFullPage) => { if (window.__grafanaSceneContext) { @@ -622,7 +671,8 @@ async function waitForQueriesAndVisualizations(page: puppeteer.Page, options: Im } }); - const rowCount = document.querySelectorAll('.dashboard-row').length || document.querySelectorAll("[data-testid='dashboard-row-container']").length + const rowCount = + document.querySelectorAll('.dashboard-row').length || document.querySelectorAll("[data-testid='dashboard-row-container']").length; const totalPanelsRendered = panelsRenderedCount + rowCount; return totalPanelsRendered >= panelCount; } @@ -633,6 +683,7 @@ async function waitForQueriesAndVisualizations(page: puppeteer.Page, options: Im { timeout: options.timeout * 1000, polling: 'mutation', + signal, }, options.fullPageImage ); diff --git a/src/browser/clustered.ts b/src/browser/clustered.ts index 0c5072c0..6b791050 100644 --- a/src/browser/clustered.ts +++ b/src/browser/clustered.ts @@ -14,6 +14,7 @@ interface ClusterOptions { groupId?: string; options: RenderOptions | ImageRenderOptions; renderType: RenderType; + signal: AbortSignal; } type ClusterResponse = RenderResponse | RenderCSVResponse; @@ -71,19 +72,26 @@ export class ClusteredBrowser extends Browser { async start(): Promise { this.cluster = await this.createCluster(); await this.cluster.task(async ({ page, data }) => { - if (data.options.timezone) { + const { options, renderType, signal } = data; + if (options.timezone) { // set timezone - await page.emulateTimezone(data.options.timezone); + await page.emulateTimezone(options.timezone); } try { + signal.addEventListener('abort', () => { + if (page) { + this.removePageListeners(page); + } + }); + this.addPageListeners(page); - switch (data.renderType) { + switch (renderType) { case RenderType.CSV: - return await this.exportCSV(page, data.options); + return await this.exportCSV(page, options, signal); case RenderType.PNG: default: - return await this.takeScreenshot(page, data.options as ImageRenderOptions); + return await this.takeScreenshot(page, options as ImageRenderOptions, signal); } } finally { this.removePageListeners(page); @@ -99,13 +107,13 @@ export class ClusteredBrowser extends Browser { return undefined; }; - async render(options: ImageRenderOptions): Promise { + async render(options: ImageRenderOptions, signal: AbortSignal): Promise { this.validateImageOptions(options); - return this.cluster.execute({ groupId: this.getGroupId(options), options, renderType: RenderType.PNG }); + return this.cluster.execute({ groupId: this.getGroupId(options), options, renderType: RenderType.PNG, signal }); } - async renderCSV(options: RenderOptions): Promise { + async renderCSV(options: RenderOptions, signal: AbortSignal): Promise { this.validateRenderOptions(options); - return this.cluster.execute({ groupId: this.getGroupId(options), options, renderType: RenderType.CSV }); + return this.cluster.execute({ groupId: this.getGroupId(options), options, renderType: RenderType.CSV, signal }); } } diff --git a/src/browser/error.ts b/src/browser/error.ts new file mode 100644 index 00000000..723dca9a --- /dev/null +++ b/src/browser/error.ts @@ -0,0 +1,6 @@ +export class TimeoutError extends Error { + constructor(step) { + super('Timeout error while performing step: ' + step); + this.name = 'TimeoutError'; + } +} diff --git a/src/browser/reusable.ts b/src/browser/reusable.ts index a8cd4e36..c1ca37be 100644 --- a/src/browser/reusable.ts +++ b/src/browser/reusable.ts @@ -16,7 +16,7 @@ export class ReusableBrowser extends Browser { this.browser = await puppeteer.launch(launcherOptions); } - async render(options: ImageRenderOptions): Promise { + async render(options: ImageRenderOptions, signal: AbortSignal): Promise { let context: puppeteer.BrowserContext | undefined; let page: puppeteer.Page | undefined; @@ -32,9 +32,19 @@ export class ReusableBrowser extends Browser { await page.emulateTimezone(options.timezone); } + signal.addEventListener('abort', () => { + if (page) { + this.removePageListeners(page); + page.close(); + } + if (context) { + context.close(); + } + }); + this.addPageListeners(page); - return await this.takeScreenshot(page, options); + return await this.takeScreenshot(page, options, signal); } finally { if (page) { this.removePageListeners(page); @@ -46,7 +56,7 @@ export class ReusableBrowser extends Browser { } } - async renderCSV(options: RenderOptions): Promise { + async renderCSV(options: RenderOptions, signal: AbortSignal): Promise { let context: puppeteer.BrowserContext | undefined; let page: puppeteer.Page | undefined; @@ -60,9 +70,19 @@ export class ReusableBrowser extends Browser { await page.emulateTimezone(options.timezone); } + signal.addEventListener('abort', () => { + if (page) { + this.removePageListeners(page); + page.close(); + } + if (context) { + context.close(); + } + }); + this.addPageListeners(page); - return await this.exportCSV(page, options); + return await this.exportCSV(page, options, signal); } finally { if (page) { this.removePageListeners(page); diff --git a/src/plugin/v2/grpc_plugin.ts b/src/plugin/v2/grpc_plugin.ts index b528d09d..9ba10dad 100644 --- a/src/plugin/v2/grpc_plugin.ts +++ b/src/plugin/v2/grpc_plugin.ts @@ -53,7 +53,7 @@ const pluginV2ProtoDescriptor = grpc.loadPackageDefinition(pluginV2PackageDef); const sanitizerProtoDescriptor = grpc.loadPackageDefinition(sanitizerPackageDef); export class RenderGRPCPluginV2 implements GrpcPlugin { - constructor(private config: PluginConfig, private log: Logger) { } + constructor(private config: PluginConfig, private log: Logger) {} async grpcServer(server: grpc.Server) { const metrics = setupMetrics(); @@ -92,7 +92,7 @@ export class RenderGRPCPluginV2 implements GrpcPlugin { class PluginGRPCServer { private browserVersion: string | undefined; - constructor(private browser: Browser, private log: Logger, private sanitizer: Sanitizer, private securityCfg: SecurityConfig) { } + constructor(private browser: Browser, private log: Logger, private sanitizer: Sanitizer, private securityCfg: SecurityConfig) {} async start(browserVersion?: string) { this.browserVersion = browserVersion; @@ -100,6 +100,7 @@ class PluginGRPCServer { } async render(call: grpc.ServerUnaryCall, callback: grpc.sendUnaryData) { + const { signal } = new AbortController(); const req = call.request; const headers: HTTPHeaders = {}; @@ -141,7 +142,7 @@ class PluginGRPCServer { this.log.debug('Render request received', 'url', options.url); let errStr = ''; try { - await this.browser.render(options); + await this.browser.render(options, signal); } catch (err) { this.log.error('Render request failed', 'url', options.url, 'error', err.toString()); errStr = err.toString(); @@ -150,6 +151,8 @@ class PluginGRPCServer { } async renderCsv(call: grpc.ServerUnaryCall, callback: grpc.sendUnaryData) { + const { signal } = new AbortController(); + const req = call.request; const headers: HTTPHeaders = {}; @@ -188,7 +191,7 @@ class PluginGRPCServer { let errStr = ''; let fileName = ''; try { - const result = await this.browser.renderCSV(options); + const result = await this.browser.renderCSV(options, signal); fileName = result.fileName || ''; } catch (err) { this.log.error('Render request failed', 'url', options.url, 'error', err.toString()); diff --git a/src/service/http-server.ts b/src/service/http-server.ts index 0b23b5f1..47107558 100644 --- a/src/service/http-server.ts +++ b/src/service/http-server.ts @@ -136,7 +136,7 @@ export class HttpServer { createServer() { const { protocol, host, port } = this.config.service; if (protocol === 'https') { - const { certFile, certKey, minTLSVersion } = this.config.service + const { certFile, certKey, minTLSVersion } = this.config.service; if (!certFile || !certKey) { throw new Error('No cert file or cert key provided, cannot start HTTPS server'); } @@ -148,16 +148,16 @@ export class HttpServer { const options = { cert: fs.readFileSync(certFile), key: fs.readFileSync(certKey), - + maxVersion: 'TLSv1.3' as SecureVersion, minVersion: (minTLSVersion || 'TLSv1.2') as SecureVersion, - } - - this.server = https.createServer(options, this.app) + }; + + this.server = https.createServer(options, this.app); } else { - this.server = http.createServer(this.app) - } - + this.server = http.createServer(this.app); + } + if (host) { this.server.listen(port, host, () => { const info = this.server.address() as net.AddressInfo; @@ -176,6 +176,9 @@ export class HttpServer { } render = async (req: express.Request, res: express.Response, next: express.NextFunction) => { + const abortController = new AbortController(); + const { signal } = abortController; + if (!req.query.url) { throw boom.badRequest('Missing url parameter'); } @@ -203,22 +206,28 @@ export class HttpServer { this.log.debug('Render request received', 'url', options.url); req.on('close', (err) => { this.log.debug('Connection closed', 'url', options.url, 'error', err); + abortController.abort(); }); - const result = await this.browser.render(options); - - res.sendFile(result.filePath, (err) => { - if (err) { - next(err); - } else { - try { - this.log.debug('Deleting temporary file', 'file', result.filePath); - fs.unlinkSync(result.filePath); - } catch (e) { - this.log.error('Failed to delete temporary file', 'file', result.filePath); + try { + const result = await this.browser.render(options, signal); + + res.sendFile(result.filePath, (err) => { + if (err) { + next(err); + } else { + try { + this.log.debug('Deleting temporary file', 'file', result.filePath); + fs.unlinkSync(result.filePath); + } catch (e) { + this.log.error('Failed to delete temporary file', 'file', result.filePath); + } } - } - }); + }); + } catch (e) { + this.log.error('Render failed', 'url', options.url, 'error', e.stack); + return res.status(500).json({ error: e.message }); + } }; sanitize = async (req: express.Request, res: express.Response<{ error: string }>) => { @@ -260,6 +269,8 @@ export class HttpServer { }; renderCSV = async (req: express.Request, res: express.Response, next: express.NextFunction) => { + const { abort, signal } = new AbortController(); + if (!req.query.url) { throw boom.badRequest('Missing url parameter'); } @@ -284,8 +295,9 @@ export class HttpServer { this.log.debug('Render request received', 'url', options.url); req.on('close', (err) => { this.log.debug('Connection closed', 'url', options.url, 'error', err); + abort(); }); - const result = await this.browser.renderCSV(options); + const result = await this.browser.renderCSV(options, signal); if (result.fileName) { res.setHeader('Content-Disposition', contentDisposition(result.fileName)); @@ -298,13 +310,13 @@ export class HttpServer { this.log.debug('Deleting temporary file', 'file', result.filePath); fs.unlink(result.filePath, (err) => { if (err) { - throw err + throw err; } if (!options.filePath) { fs.rmdir(path.dirname(result.filePath), () => {}); } - }) + }); } catch (e) { this.log.error('Failed to delete temporary file', 'file', result.filePath, 'error', e.message); } From dfa3cdfe6f831af3bc36a8b3594f60ec8d39c4d4 Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Tue, 17 Dec 2024 15:47:30 +0100 Subject: [PATCH 2/9] refactor --- src/browser/browser.ts | 219 ++++++++++++++++++++-------------------- src/browser/error.ts | 2 +- src/browser/reusable.ts | 4 +- 3 files changed, 115 insertions(+), 110 deletions(-) diff --git a/src/browser/browser.ts b/src/browser/browser.ts index 01acdc09..a532e867 100644 --- a/src/browser/browser.ts +++ b/src/browser/browser.ts @@ -9,7 +9,7 @@ import * as Jimp from 'jimp'; import { Logger } from '../logger'; import { RenderingConfig } from '../config/rendering'; import { ImageRenderOptions, RenderOptions } from '../types'; -import { TimeoutError } from './error'; +import { StepTimeoutError } from './error'; import { getPDFOptionsFromURL } from './pdf'; export interface Metrics { @@ -267,15 +267,15 @@ export class Browser { let page: puppeteer.Page | undefined = undefined; try { - browser = await this.withTimingMetrics(() => { + browser = await this.withTimingMetrics('launch', () => { this.validateImageOptions(options); const launcherOptions = this.getLauncherOptions(options); return puppeteer.launch(launcherOptions); - }, 'launch'); + }); - page = await this.withTimingMetrics(() => { + page = await this.withTimingMetrics('newPage', () => { return browser!.newPage(); - }, 'newPage'); + }); signal.addEventListener('abort', () => { if (page) { @@ -310,91 +310,59 @@ export class Browser { }; async takeScreenshot(page: puppeteer.Page, options: ImageRenderOptions, signal: AbortSignal): Promise { - try { - await this.withTimingMetrics(async () => { - if (this.config.verboseLogging) { - this.log.debug( - 'Setting viewport for page', - 'width', - options.width.toString(), - 'height', - options.height.toString(), - 'deviceScaleFactor', - options.deviceScaleFactor - ); - } - - await this.setViewport(page, options); - await this.preparePage(page, options); - await this.setTimezone(page, options); + await this.performStep('prepare', options.url, signal, async () => { + if (this.config.verboseLogging) { + this.log.debug( + 'Setting viewport for page', + 'width', + options.width.toString(), + 'height', + options.height.toString(), + 'deviceScaleFactor', + options.deviceScaleFactor + ); + } - if (this.config.verboseLogging) { - this.log.debug('Moving mouse on page', 'x', options.width, 'y', options.height); - } - return page.mouse.move(+options.width, +options.height); - }, 'prepare'); + await this.setViewport(page, options); + await this.preparePage(page, options); + await this.setTimezone(page, options); - // await new Promise((resolve) => setTimeout(resolve, 8000)); - // this.log.error('signal status', 'is aborted', signal.aborted); + if (this.config.verboseLogging) { + this.log.debug('Moving mouse on page', 'x', options.width, 'y', options.height); + } + return page.mouse.move(+options.width, +options.height); + }); - const res = await this.withTimingMetrics(() => { - if (this.config.verboseLogging) { - this.log.debug('Navigating and waiting for all network requests to finish', 'url', options.url); - } + await this.performStep('navigate', options.url, signal, async () => { + if (this.config.verboseLogging) { + this.log.debug('Navigating and waiting for all network requests to finish', 'url', options.url); + } - return page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal }); - }, 'navigate'); - this.log.error('signal status', 'is aborted', signal.aborted, 'res', res?.url); - } catch (err) { - this.log.error('Error while trying to prepare page for screenshot', 'url', options.url, 'err', err.stack); - throw new TimeoutError('navigate'); - } + await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal }); + }); let scrollResult: DashboardScrollingResult = { scrolled: false, }; if (options.fullPageImage) { - try { - scrollResult = await this.withTimingMetrics(() => { - return this.scrollToLoadAllPanels(page, options, signal); - }, 'dashboardScrolling'); - } catch (err) { - this.log.error('Error while scrolling to load all panels', 'url', options.url, 'err', err.stack); - throw new TimeoutError('dashboardScrolling'); + const res = await this.performStep('dashboardScrolling', options.url, signal, () => { + return this.scrollToLoadAllPanels(page, options, signal); + }); + + if (res) { + scrollResult = res; } } const isPDF = options.encoding === 'pdf'; + await this.performStep('panelsRendered', options.url, signal, async () => { + if (this.config.verboseLogging) { + this.log.debug('Waiting for dashboard/panel to load', 'timeout', `${options.timeout}s`); + } - try { - await this.withTimingMetrics(async () => { - if (this.config.verboseLogging) { - this.log.debug('Waiting for dashboard/panel to load', 'timeout', `${options.timeout}s`); - } - - await waitForQueriesAndVisualizations(page, options, signal); - }, 'panelsRendered'); - } catch (err) { - const windowInfo = await page.evaluate(() => { - return { - usingScenes: window.__grafanaSceneContext !== null, - queryCount: window.__grafanaRunningQueryCount, - }; - }); - this.log.error( - 'Error while waiting for the panels to load', - 'url', - options.url, - 'usingScenes', - windowInfo.usingScenes, - 'queryCount', - windowInfo.queryCount, - 'err', - err.stack - ); - throw new TimeoutError('panelsRendered'); - } + await waitForQueriesAndVisualizations(page, options, signal); + }); if (!options.filePath) { options.filePath = uniqueFilename(os.tmpdir()) + (isPDF ? '.pdf' : '.png'); @@ -402,11 +370,11 @@ export class Browser { await this.setPageZoomLevel(page, this.config.pageZoomLevel); - if (this.config.verboseLogging) { - this.log.debug('Taking screenshot', 'filePath', options.filePath); - } + await this.performStep('screenshot', options.url, signal, async () => { + if (this.config.verboseLogging) { + this.log.debug('Taking screenshot', 'filePath', options.filePath); + } - await this.withTimingMetrics(async () => { if (scrollResult.scrolled) { await this.setViewport(page, { ...options, @@ -437,14 +405,14 @@ export class Browser { } return page.screenshot({ path: options.filePath, fullPage: options.fullPageImage, captureBeyondViewport: false }); - }, 'screenshot'); + }); if (options.scaleImage && !isPDF) { - const scaled = `${options.filePath}_${Date.now()}_scaled.png`; - const w = +options.width / options.scaleImage; - const h = +options.height / options.scaleImage; + await this.performStep('imageResize', options.url, signal, async () => { + const scaled = `${options.filePath}_${Date.now()}_scaled.png`; + const w = +options.width / options.scaleImage!; + const h = +options.height / options.scaleImage!; - await this.withTimingMetrics(async () => { const file = await Jimp.read(options.filePath); await file .resize(w, h) @@ -454,7 +422,7 @@ export class Browser { .writeAsync(scaled); fs.renameSync(scaled, options.filePath); - }, 'imageResize'); + }); } return { filePath: options.filePath }; @@ -495,42 +463,51 @@ export class Browser { } async exportCSV(page: any, options: RenderOptions, signal: AbortSignal): Promise { - await this.preparePage(page, options); - await this.setTimezone(page, options); + await this.performStep('prepare', options.url, signal, async () => { + await this.preparePage(page, options); + await this.setTimezone(page, options); + await page._client().send('Page.setDownloadBehavior', { behavior: 'allow', downloadPath: downloadPath }); + }); + + let downloadFilePath = ''; const downloadPath = uniqueFilename(os.tmpdir()); fs.mkdirSync(downloadPath); const watcher = chokidar.watch(downloadPath); - let downloadFilePath = ''; watcher.on('add', (file) => { if (!file.endsWith('.crdownload')) { downloadFilePath = file; } }); - await page._client().send('Page.setDownloadBehavior', { behavior: 'allow', downloadPath: downloadPath }); - - if (this.config.verboseLogging) { - this.log.debug('Navigating and waiting for all network requests to finish', 'url', options.url); - } + await this.performStep('navigateCSV', options.url, signal, async () => { + if (this.config.verboseLogging) { + this.log.debug('Navigating and waiting for all network requests to finish', 'url', options.url); + } - await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal }); + await page.goto(options.url, { waitUntil: 'networkidle0', timeout: options.timeout * 1000, signal }); + }); - if (this.config.verboseLogging) { - this.log.debug('Waiting for download to end'); - } + await this.performStep('downloadCSV', options.url, signal, async () => { + if (this.config.verboseLogging) { + this.log.debug('Waiting for download to end'); + } - const startDate = Date.now(); - while (Date.now() - startDate <= options.timeout * 1000) { - if (downloadFilePath !== '') { - break; + const startDate = Date.now(); + while (Date.now() - startDate <= options.timeout * 1000) { + if (signal.aborted) { + throw new StepTimeoutError('downloadCSV'); + } + if (downloadFilePath !== '') { + break; + } + await new Promise((resolve) => setTimeout(resolve, 500)); } - await new Promise((resolve) => setTimeout(resolve, 500)); - } - if (downloadFilePath === '') { - throw new TimeoutError(`CSV download`); - } + if (downloadFilePath === '') { + throw new StepTimeoutError('downloadCSV'); + } + }); await watcher.close(); @@ -545,7 +522,35 @@ export class Browser { return { filePath, fileName: path.basename(downloadFilePath) }; } - async withTimingMetrics(callback: () => Promise, step: string): Promise { + async performStep(step: string, url: string, signal: AbortSignal, callback: () => Promise): Promise { + if (this.config.verboseLogging) { + this.log.debug('Step begins', 'step', step, 'url', url); + } + + try { + const res = await this.withTimingMetrics(step, callback); + + if (signal.aborted) { + this.log.warn('Signal aborted while performing step', 'step', step, 'url', url); + throw new StepTimeoutError(step); + } + + if (this.config.verboseLogging) { + this.log.debug('Step ends', 'step', step, 'url', url); + } + + return res; + } catch (err) { + if (!(err instanceof puppeteer.TimeoutError)) { + this.log.error('Error while performing step', 'step', step, 'url', url); + throw err; + } + + this.log.error('Error while performing step', 'step', step, 'url', url, 'err', err.stack); + } + } + + async withTimingMetrics(step: string, callback: () => Promise): Promise { if (this.config.timingMetrics) { const endTimer = this.metrics.durationHistogram.startTimer({ step }); const res = await callback(); diff --git a/src/browser/error.ts b/src/browser/error.ts index 723dca9a..794154b0 100644 --- a/src/browser/error.ts +++ b/src/browser/error.ts @@ -1,4 +1,4 @@ -export class TimeoutError extends Error { +export class StepTimeoutError extends Error { constructor(step) { super('Timeout error while performing step: ' + step); this.name = 'TimeoutError'; diff --git a/src/browser/reusable.ts b/src/browser/reusable.ts index c1ca37be..4281400d 100644 --- a/src/browser/reusable.ts +++ b/src/browser/reusable.ts @@ -21,11 +21,11 @@ export class ReusableBrowser extends Browser { let page: puppeteer.Page | undefined; try { - page = await this.withTimingMetrics(async () => { + page = await this.withTimingMetrics('newPage', async () => { this.validateImageOptions(options); context = await this.browser.createBrowserContext(); return context.newPage(); - }, 'newPage'); + }); if (options.timezone) { // set timezone From e13ecadca32d76b9b7ff715a21e5176a862f4267 Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Tue, 17 Dec 2024 20:41:54 +0100 Subject: [PATCH 3/9] fix default rendering mode --- src/browser/browser.ts | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/browser/browser.ts b/src/browser/browser.ts index a532e867..45637db4 100644 --- a/src/browser/browser.ts +++ b/src/browser/browser.ts @@ -277,16 +277,6 @@ export class Browser { return browser!.newPage(); }); - signal.addEventListener('abort', () => { - if (page) { - this.removePageListeners(page); - page.close(); - } - if (browser) { - browser.close(); - } - }); - this.addPageListeners(page); return await this.takeScreenshot(page, options, signal); @@ -438,16 +428,6 @@ export class Browser { browser = await puppeteer.launch(launcherOptions); page = await browser.newPage(); - signal.addEventListener('abort', () => { - if (page) { - this.removePageListeners(page); - page.close(); - } - if (browser) { - browser.close(); - } - }); - this.addPageListeners(page); return await this.exportCSV(page, options, signal); From 0eff67d12ad48931fa6cc43861b8b2ed91a52e2c Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Tue, 17 Dec 2024 20:42:24 +0100 Subject: [PATCH 4/9] fix plugin mode --- src/plugin/v2/grpc_plugin.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/plugin/v2/grpc_plugin.ts b/src/plugin/v2/grpc_plugin.ts index 9ba10dad..97f2ab54 100644 --- a/src/plugin/v2/grpc_plugin.ts +++ b/src/plugin/v2/grpc_plugin.ts @@ -100,7 +100,9 @@ class PluginGRPCServer { } async render(call: grpc.ServerUnaryCall, callback: grpc.sendUnaryData) { - const { signal } = new AbortController(); + const abortController = new AbortController(); + const { signal } = abortController; + const req = call.request; const headers: HTTPHeaders = {}; @@ -140,7 +142,12 @@ class PluginGRPCServer { }; this.log.debug('Render request received', 'url', options.url); + call.on('cancelled', (err) => { + this.log.debug('Connection closed', 'url', options.url, 'error', err); + abortController.abort(); + }); let errStr = ''; + try { await this.browser.render(options, signal); } catch (err) { @@ -151,7 +158,8 @@ class PluginGRPCServer { } async renderCsv(call: grpc.ServerUnaryCall, callback: grpc.sendUnaryData) { - const { signal } = new AbortController(); + const abortController = new AbortController(); + const { signal } = abortController; const req = call.request; const headers: HTTPHeaders = {}; @@ -188,6 +196,11 @@ class PluginGRPCServer { }; this.log.debug('Render request received', 'url', options.url); + call.on('cancelled', (err) => { + this.log.debug('Connection closed', 'url', options.url, 'error', err); + abortController.abort(); + }); + let errStr = ''; let fileName = ''; try { From 85adb731f3bfa7d6831a92f8864e369291cb6022 Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:32:10 +0100 Subject: [PATCH 5/9] fix abortController usage for csv --- src/service/http-server.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/service/http-server.ts b/src/service/http-server.ts index 47107558..b4daeea7 100644 --- a/src/service/http-server.ts +++ b/src/service/http-server.ts @@ -269,7 +269,8 @@ export class HttpServer { }; renderCSV = async (req: express.Request, res: express.Response, next: express.NextFunction) => { - const { abort, signal } = new AbortController(); + const abortController = new AbortController(); + const { signal } = abortController; if (!req.query.url) { throw boom.badRequest('Missing url parameter'); @@ -295,7 +296,7 @@ export class HttpServer { this.log.debug('Render request received', 'url', options.url); req.on('close', (err) => { this.log.debug('Connection closed', 'url', options.url, 'error', err); - abort(); + abortController.abort(); }); const result = await this.browser.renderCSV(options, signal); From e81cfad2f50a330aff0cfbcc04751136602f07f8 Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:32:17 +0100 Subject: [PATCH 6/9] improve log --- src/browser/browser.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/browser/browser.ts b/src/browser/browser.ts index 45637db4..cd70d2c9 100644 --- a/src/browser/browser.ts +++ b/src/browser/browser.ts @@ -522,7 +522,7 @@ export class Browser { return res; } catch (err) { if (!(err instanceof puppeteer.TimeoutError)) { - this.log.error('Error while performing step', 'step', step, 'url', url); + this.log.error('Error while performing step', 'step', step, 'url', url, 'err', err.stack); throw err; } From 76f61a70347d1d3546abe2a4f5a5745c10b6e62b Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:56:45 +0100 Subject: [PATCH 7/9] fix csv download + improve logs --- src/browser/browser.ts | 7 +++-- src/service/http-server.ts | 52 +++++++++++++++++++++----------------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/browser/browser.ts b/src/browser/browser.ts index cd70d2c9..185b6d3d 100644 --- a/src/browser/browser.ts +++ b/src/browser/browser.ts @@ -443,15 +443,17 @@ export class Browser { } async exportCSV(page: any, options: RenderOptions, signal: AbortSignal): Promise { + let downloadFilePath = ''; + let downloadPath = ''; await this.performStep('prepare', options.url, signal, async () => { + downloadPath = uniqueFilename(os.tmpdir()); + await this.preparePage(page, options); await this.setTimezone(page, options); await page._client().send('Page.setDownloadBehavior', { behavior: 'allow', downloadPath: downloadPath }); }); - let downloadFilePath = ''; - const downloadPath = uniqueFilename(os.tmpdir()); fs.mkdirSync(downloadPath); const watcher = chokidar.watch(downloadPath); watcher.on('add', (file) => { @@ -476,6 +478,7 @@ export class Browser { const startDate = Date.now(); while (Date.now() - startDate <= options.timeout * 1000) { if (signal.aborted) { + this.log.warn('Signal aborted while performing step', 'step', 'downloadCSV', 'url', options.url); throw new StepTimeoutError('downloadCSV'); } if (downloadFilePath !== '') { diff --git a/src/service/http-server.ts b/src/service/http-server.ts index b4daeea7..e79b0678 100644 --- a/src/service/http-server.ts +++ b/src/service/http-server.ts @@ -298,30 +298,36 @@ export class HttpServer { this.log.debug('Connection closed', 'url', options.url, 'error', err); abortController.abort(); }); - const result = await this.browser.renderCSV(options, signal); - if (result.fileName) { - res.setHeader('Content-Disposition', contentDisposition(result.fileName)); - } - res.sendFile(result.filePath, (err) => { - if (err) { - next(err); - } else { - try { - this.log.debug('Deleting temporary file', 'file', result.filePath); - fs.unlink(result.filePath, (err) => { - if (err) { - throw err; - } - - if (!options.filePath) { - fs.rmdir(path.dirname(result.filePath), () => {}); - } - }); - } catch (e) { - this.log.error('Failed to delete temporary file', 'file', result.filePath, 'error', e.message); - } + try { + const result = await this.browser.renderCSV(options, signal); + + if (result.fileName) { + res.setHeader('Content-Disposition', contentDisposition(result.fileName)); } - }); + res.sendFile(result.filePath, (err) => { + if (err) { + next(err); + } else { + try { + this.log.debug('Deleting temporary file', 'file', result.filePath); + fs.unlink(result.filePath, (err) => { + if (err) { + throw err; + } + + if (!options.filePath) { + fs.rmdir(path.dirname(result.filePath), () => {}); + } + }); + } catch (e) { + this.log.error('Failed to delete temporary file', 'file', result.filePath, 'error', e.message); + } + } + }); + } catch (e) { + this.log.error('Render CSV failed', 'url', options.url, 'error', e.stack); + return res.status(500).json({ error: e.message }); + } }; } From 7797ee57ec595938cc581d47d8816ff47deee21b Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Thu, 9 Jan 2025 12:23:45 +0100 Subject: [PATCH 8/9] remove duplicated code to removePageListeneres --- src/browser/clustered.ts | 6 ------ src/browser/reusable.ts | 10 ---------- 2 files changed, 16 deletions(-) diff --git a/src/browser/clustered.ts b/src/browser/clustered.ts index 6b791050..37fc4730 100644 --- a/src/browser/clustered.ts +++ b/src/browser/clustered.ts @@ -79,12 +79,6 @@ export class ClusteredBrowser extends Browser { } try { - signal.addEventListener('abort', () => { - if (page) { - this.removePageListeners(page); - } - }); - this.addPageListeners(page); switch (renderType) { case RenderType.CSV: diff --git a/src/browser/reusable.ts b/src/browser/reusable.ts index 4281400d..1e2c66b6 100644 --- a/src/browser/reusable.ts +++ b/src/browser/reusable.ts @@ -32,16 +32,6 @@ export class ReusableBrowser extends Browser { await page.emulateTimezone(options.timezone); } - signal.addEventListener('abort', () => { - if (page) { - this.removePageListeners(page); - page.close(); - } - if (context) { - context.close(); - } - }); - this.addPageListeners(page); return await this.takeScreenshot(page, options, signal); From 386499251f837f9d1c1f1edb0055a7a30d006565 Mon Sep 17 00:00:00 2001 From: AgnesToulet <35176601+AgnesToulet@users.noreply.github.com> Date: Thu, 9 Jan 2025 12:25:47 +0100 Subject: [PATCH 9/9] remove duplicated cleanup --- src/browser/reusable.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/browser/reusable.ts b/src/browser/reusable.ts index 1e2c66b6..efd37dc5 100644 --- a/src/browser/reusable.ts +++ b/src/browser/reusable.ts @@ -60,16 +60,6 @@ export class ReusableBrowser extends Browser { await page.emulateTimezone(options.timezone); } - signal.addEventListener('abort', () => { - if (page) { - this.removePageListeners(page); - page.close(); - } - if (context) { - context.close(); - } - }); - this.addPageListeners(page); return await this.exportCSV(page, options, signal);