Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Reporting/Screenshots] Handle page setup errors and capture the page, don't fail the job #58683

Merged
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d953b97
[Reporting] Handle error if intercepted request could not be continued
tsullivan Feb 28, 2020
052dc3e
[Reporting/Screenshots] Handle page setup errors and capture the page…
tsullivan Feb 26, 2020
32ec120
show warnings in UI
tsullivan Feb 27, 2020
45ce509
i18n todos
tsullivan Feb 27, 2020
7fe4001
Cleanup an old troubleshooting task
tsullivan Feb 27, 2020
bdc2ea7
set the default for all new timeout settings to 30 seconds
tsullivan Feb 27, 2020
d6fc7bc
fix some tests
tsullivan Feb 28, 2020
ad8579d
update error strings
tsullivan Feb 28, 2020
dd5d05f
Cleanup 2
tsullivan Feb 28, 2020
7dc9e51
fix tests 2
tsullivan Feb 28, 2020
12dc009
polish the job info map status items
tsullivan Feb 28, 2020
535dd92
More error message updating
tsullivan Feb 28, 2020
6eab0ab
Log the error that was caught
tsullivan Feb 28, 2020
12679f5
Oops fix ts
tsullivan Feb 28, 2020
86a9738
add documentation
tsullivan Feb 28, 2020
606c1f1
Merge branch 'master' into reporting/screenshot-always-capture
tsullivan Mar 3, 2020
4ce34f6
fix i18n
tsullivan Mar 3, 2020
8ff5ced
fix mocha test
tsullivan Mar 4, 2020
fc9d418
Merge branch 'master' into reporting/screenshot-always-capture
tsullivan Mar 4, 2020
d8a3d0c
Merge branch 'master' into reporting/screenshot-always-capture
tsullivan Mar 4, 2020
99066b2
Merge branch 'master' into reporting/screenshot-always-capture
tsullivan Mar 5, 2020
926b694
use the openUrl timeout as the default for navigation
tsullivan Mar 5, 2020
1723523
fix comment
tsullivan Mar 5, 2020
01cb7e4
Merge branch 'master' into reporting/screenshot-always-capture
elasticmachine Mar 5, 2020
d514aee
Merge branch 'master' into reporting/screenshot-always-capture
tsullivan Mar 5, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions docs/settings/reporting-settings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ index for any pending Reporting jobs. Defaults to `3000` (3 seconds).
[[xpack-reporting-q-timeout]]`xpack.reporting.queue.timeout`::
How long each worker has to produce a report. If your machine is slow or under
heavy load, you might need to increase this timeout. Specified in milliseconds.
If a Reporting job execution time goes over this time limit, the job will be
marked as a failure and there will not be a download available.
Defaults to `120000` (two minutes).

[float]
Expand All @@ -104,6 +106,26 @@ Defaults to `120000` (two minutes).
Reporting works by capturing screenshots from Kibana. The following settings
control the capturing process.

`xpack.reporting.capture.timeouts.openUrl`::
How long to allow the Reporting browser to wait for the initial data of the
Kibana page to load. Defaults to `30000` (30 seconds).

`xpack.reporting.capture.timeouts.waitForElements`::
How long to allow the Reporting browser to wait for the visualization panels to
load on the Kibana page. Defaults to `30000` (30 seconds).

`xpack.reporting.capture.timeouts.renderComplete`::
How long to allow the Reporting brwoser to wait for each visualization to
signal that it is done renderings. Defaults to `30000` (30 seconds).

[NOTE]
============
If any timeouts from `xpack.reporting.capture.timeouts.*` settings occur when
running a report job, Reporting will log the error and try to continue
capturing the page with a screenshot. As a result, a download will be
available, but there will likely be errors in the visualizations in the report.
============

`xpack.reporting.capture.maxAttempts`::
If capturing a report fails for any reason, Kibana will re-attempt othe reporting
job, as many times as this setting. Defaults to `3`.
Expand Down
20 changes: 20 additions & 0 deletions x-pack/legacy/plugins/reporting/__snapshots__/index.test.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions x-pack/legacy/plugins/reporting/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@ export async function config(Joi: any) {
.default(120000),
}).default(),
capture: Joi.object({
timeouts: Joi.object({
openUrl: Joi.number()
.integer()
.default(30000),
waitForElements: Joi.number()
.integer()
.default(30000),
renderComplete: Joi.number()
.integer()
.default(30000),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: These get passed to Puppeteer waitForSelector calls. Puppeteer's default for these is 30 seconds internally.

Copy link
Contributor

@joelgriffith joelgriffith Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do call https://pptr.dev/#?product=Puppeteer&version=v2.1.1&show=api-pagesetdefaulttimeouttimeout and pass in the overall job timeout, so we might be overriding the default timeouts for these APIS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ If we're passing the timeout to each of the calls, then setting page.setDefaultTimeout doesn't make a lot of sense.

I don't think we should use the global job timeout, which is the "queue timeout" because once that timeout hits there is no buffer room to recover a completed screenshot for the job. Hitting the "queue timeout" should be avoided as much as possible.

We need to think about this... Should the page.setDefaultTimeout be the max of these 3 new timeouts?

At any rate, server/browsers/chromium/driver_factory/index.ts#L120 is no longer valid in this PR

Copy link
Member Author

@tsullivan tsullivan Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this makes sense:

        // All navigation methods default to 30 seconds,
        // which can cause the job to fail even if we bump timeouts in	        // All waitFor methods have their own timeout config
        // the config. Help alleviate errors like	        page.setDefaultTimeout(this.captureConfig.timeouts.openUrl);```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Individual methods can still override this, but I like setting it to the openUrl which is where I think the bulk of the operation is for PDF reports

}).default(),
networkPolicy: Joi.object({
enabled: Joi.boolean().default(true),
rules: Joi.array()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ export const LayoutTypes = {
PRINT: 'print',
};

export const WAITFOR_SELECTOR = '.application';
export const PAGELOAD_SELECTOR = '.application';
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export interface LayoutSelectorDictionary {
renderComplete: string;
itemsCountAttribute: string;
timefilterDurationAttribute: string;
toastHeader: string;
}

export interface PdfImageSize {
Expand All @@ -40,7 +39,6 @@ export const getDefaultLayoutSelectors = (): LayoutSelectorDictionary => ({
renderComplete: '[data-shared-item]',
itemsCountAttribute: 'data-shared-items-count',
timefilterDurationAttribute: 'data-shared-timefilter-duration',
toastHeader: '[data-test-subj="euiToastHeader"]',
});

export abstract class Layout {
Expand Down Expand Up @@ -75,9 +73,11 @@ export interface LayoutParams {
dimensions: Size;
}

export type LayoutInstance = Layout & {
interface LayoutSelectors {
// Fields that are not part of Layout: the instances
// independently implement these fields on their own
selectors: LayoutSelectorDictionary;
positionElements?: (browser: HeadlessChromiumDriver, logger: LevelLogger) => Promise<void>;
};
}

export type LayoutInstance = Layout & LayoutSelectors & Size;
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ const ZOOM: number = 2;
export class PreserveLayout extends Layout {
public readonly selectors: LayoutSelectorDictionary = getDefaultLayoutSelectors();
public readonly groupCount = 1;
private readonly height: number;
private readonly width: number;
public readonly height: number;
public readonly width: number;
private readonly scaledHeight: number;
private readonly scaledWidth: number;

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ export const CONTEXT_INJECTCSS = 'InjectCss';
export const CONTEXT_WAITFORRENDER = 'WaitForRender';
export const CONTEXT_GETTIMERANGE = 'GetTimeRange';
export const CONTEXT_ELEMENTATTRIBUTES = 'ElementPositionAndAttributes';
export const CONTEXT_CHECKFORTOASTMESSAGE = 'CheckForToastMessage';
export const CONTEXT_WAITFORELEMENTSTOBEINDOM = 'WaitForElementsToBeInDOM';
export const CONTEXT_SKIPTELEMETRY = 'SkipTelemetry';
export const CONTEXT_READMETADATA = 'ReadVisualizationsMetadata';
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { i18n } from '@kbn/i18n';
import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers';
import { LayoutInstance } from '../../layouts/layout';
import { AttributesMap, ElementsPositionAndAttribute } from './types';
Expand All @@ -14,50 +15,58 @@ export const getElementPositionAndAttributes = async (
browser: HeadlessBrowser,
layout: LayoutInstance,
logger: Logger
): Promise<ElementsPositionAndAttribute[]> => {
const elementsPositionAndAttributes: ElementsPositionAndAttribute[] = await browser.evaluate(
{
fn: (selector: string, attributes: any) => {
const elements: NodeListOf<Element> = document.querySelectorAll(selector);
): Promise<ElementsPositionAndAttribute[] | null> => {
const { screenshot: screenshotSelector } = layout.selectors; // data-shared-items-container
let elementsPositionAndAttributes: ElementsPositionAndAttribute[] | null;
try {
elementsPositionAndAttributes = await browser.evaluate(
{
fn: (selector, attributes) => {
const elements: NodeListOf<Element> = document.querySelectorAll(selector);

// NodeList isn't an array, just an iterator, unable to use .map/.forEach
const results: ElementsPositionAndAttribute[] = [];
for (let i = 0; i < elements.length; i++) {
const element = elements[i];
const boundingClientRect = element.getBoundingClientRect() as DOMRect;
results.push({
position: {
boundingClientRect: {
// modern browsers support x/y, but older ones don't
top: boundingClientRect.y || boundingClientRect.top,
left: boundingClientRect.x || boundingClientRect.left,
width: boundingClientRect.width,
height: boundingClientRect.height,
// NodeList isn't an array, just an iterator, unable to use .map/.forEach
const results: ElementsPositionAndAttribute[] = [];
for (let i = 0; i < elements.length; i++) {
const element = elements[i];
const boundingClientRect = element.getBoundingClientRect() as DOMRect;
results.push({
position: {
boundingClientRect: {
// modern browsers support x/y, but older ones don't
top: boundingClientRect.y || boundingClientRect.top,
left: boundingClientRect.x || boundingClientRect.left,
width: boundingClientRect.width,
height: boundingClientRect.height,
},
scroll: {
x: window.scrollX,
y: window.scrollY,
},
},
scroll: {
x: window.scrollX,
y: window.scrollY,
},
},
attributes: Object.keys(attributes).reduce((result: AttributesMap, key) => {
const attribute = attributes[key];
(result as any)[key] = element.getAttribute(attribute);
return result;
}, {} as AttributesMap),
});
}
return results;
attributes: Object.keys(attributes).reduce((result: AttributesMap, key) => {
const attribute = attributes[key];
(result as any)[key] = element.getAttribute(attribute);
return result;
}, {} as AttributesMap),
});
}
return results;
},
args: [screenshotSelector, { title: 'data-title', description: 'data-description' }],
},
args: [layout.selectors.screenshot, { title: 'data-title', description: 'data-description' }],
},
{ context: CONTEXT_ELEMENTATTRIBUTES },
logger
);

if (elementsPositionAndAttributes.length === 0) {
throw new Error(
`No shared items containers were found on the page! Reporting requires a container element with the '${layout.selectors.screenshot}' attribute on the page.`
{ context: CONTEXT_ELEMENTATTRIBUTES },
logger
);

if (!elementsPositionAndAttributes || elementsPositionAndAttributes.length === 0) {
throw new Error(
i18n.translate('xpack.reporting.screencapture.noElements', {
defaultMessage: `An error occurred while reading the page for visualization panels: no panels were found.`,
})
);
}
} catch (err) {
elementsPositionAndAttributes = null;
}

return elementsPositionAndAttributes;
Expand Down
Loading