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] Decouple screenshotting plugin from the reporting #120110

Merged
merged 21 commits into from
Dec 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions docs/developer/plugin-list.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,11 @@ Elastic.
|Add tagging capability to saved objects


|{kib-repo}blob/{branch}/x-pack/plugins/screenshotting/README.md[screenshotting]
|This plugin provides functionality to take screenshots of the Kibana pages.
It uses Chromium and Puppeteer underneath to run the browser in headless mode.


|{kib-repo}blob/{branch}/x-pack/plugins/searchprofiler/README.md[searchprofiler]
|The search profiler consumes the Profile API
by sending a search API with profile: true enabled in the request body. The response contains
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ it('produces the right watch and ignore list', () => {
<absolute path>/x-pack/test/plugin_functional/plugins/resolver_test/target/**,
<absolute path>/x-pack/test/plugin_functional/plugins/resolver_test/scripts/**,
<absolute path>/x-pack/test/plugin_functional/plugins/resolver_test/docs/**,
<absolute path>/x-pack/plugins/reporting/chromium,
<absolute path>/x-pack/plugins/screenshotting/chromium,
<absolute path>/x-pack/plugins/security_solution/cypress,
<absolute path>/x-pack/plugins/apm/scripts,
<absolute path>/x-pack/plugins/apm/ftr_e2e,
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-cli-dev-mode/src/get_server_watch_paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function getServerWatchPaths({ pluginPaths, pluginScanDirs }: Options) {
/\.(md|sh|txt)$/,
/debug\.log$/,
...pluginInternalDirsIgnore,
fromRoot('x-pack/plugins/reporting/chromium'),
fromRoot('x-pack/plugins/screenshotting/chromium'),
fromRoot('x-pack/plugins/security_solution/cypress'),
fromRoot('x-pack/plugins/apm/scripts'),
fromRoot('x-pack/plugins/apm/ftr_e2e'), // prevents restarts for APM cypress tests
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-optimizer/limits.yml
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,4 @@ pageLoadAssetSize:
dataViewManagement: 5000
reporting: 57003
visTypeHeatmap: 25340
screenshotting: 17017
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind helping me understand the value of screenshotting adding a new 2kb request to every page load? It looks like it stored a very tiny amount of state which could easily be managed by the reporting plugin and just increases the overall page load size and request count, but I'm probably missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there isn't a good reason it would be great if we could set "ui": false to the kibana.json file and move the contextStorage stuff to the reporting plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That exposes a generic service working with the screenshotting context. The idea is that some of the plugins will use that to extract some serialized data without having a dependency on the reporting. This code will be merged with the screenshotMode plugin later, so won't stay long here.

22 changes: 15 additions & 7 deletions src/dev/build/tasks/install_chromium.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@
* Side Public License, v 1.
*/

import { first } from 'rxjs/operators';

// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { installBrowser } from '../../../../x-pack/plugins/reporting/server/browsers/install';
import { install } from '../../../../x-pack/plugins/screenshotting/server/utils';

export const InstallChromium = {
description: 'Installing Chromium',
Expand All @@ -22,13 +20,23 @@ export const InstallChromium = {
// revert after https://github.com/elastic/kibana/issues/109949
if (target === 'darwin-arm64') continue;

const { binaryPath$ } = installBrowser(
log,
build.resolvePathForPlatform(platform, 'x-pack/plugins/reporting/chromium'),
const logger = {
get: log.withType.bind(log),
debug: log.debug.bind(log),
info: log.info.bind(log),
warn: log.warning.bind(log),
trace: log.verbose.bind(log),
error: log.error.bind(log),
fatal: log.error.bind(log),
log: log.write.bind(log),
};

await install(
logger,
build.resolvePathForPlatform(platform, 'x-pack/plugins/screenshotting/chromium'),
platform.getName(),
platform.getArchitecture()
);
await binaryPath$.pipe(first()).toPromise();
}
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,17 @@ const log = new ToolingLog({
});

describe(`enumeratePatterns`, () => {
it(`should resolve x-pack/plugins/reporting/server/browsers/extract/unzip.ts to kibana-reporting`, () => {
it(`should resolve x-pack/plugins/screenshotting/server/browsers/extract/unzip.ts to kibana-screenshotting`, () => {
const actual = enumeratePatterns(REPO_ROOT)(log)(
new Map([['x-pack/plugins/reporting', ['kibana-reporting']]])
new Map([['x-pack/plugins/screenshotting', ['kibana-screenshotting']]])
);

expect(
actual[0].includes(
'x-pack/plugins/reporting/server/browsers/extract/unzip.ts kibana-reporting'
)
).toBe(true);
expect(actual).toHaveProperty(
'0',
expect.arrayContaining([
'x-pack/plugins/screenshotting/server/browsers/extract/unzip.ts kibana-screenshotting',
])
);
});
it(`should resolve src/plugins/charts/common/static/color_maps/color_maps.ts to kibana-app`, () => {
const actual = enumeratePatterns(REPO_ROOT)(log)(
Expand Down
2 changes: 1 addition & 1 deletion x-pack/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
/test/functional/apps/**/reports/session
/test/reporting/configs/failure_debug/
/plugins/reporting/.chromium/
/plugins/reporting/chromium/
/plugins/screenshotting/chromium/
/plugins/reporting/.phantom/
/.aws-config.json
/.env
Expand Down
1 change: 1 addition & 0 deletions x-pack/.i18nrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"xpack.reporting": ["plugins/reporting"],
"xpack.rollupJobs": ["plugins/rollup"],
"xpack.runtimeFields": "plugins/runtime_fields",
"xpack.screenshotting": "plugins/screenshotting",
"xpack.searchProfiler": "plugins/searchprofiler",
"xpack.security": "plugins/security",
"xpack.server": "legacy/server",
Expand Down
3 changes: 2 additions & 1 deletion x-pack/examples/reporting_example/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@
},
"description": "Example integration code for applications to feature reports.",
"optionalPlugins": [],
"requiredPlugins": ["reporting", "developerExamples", "navigation", "screenshotMode", "share"]
"requiredPlugins": ["reporting", "developerExamples", "navigation", "screenshotMode", "share"],
"requiredBundles": ["screenshotting"]
}
13 changes: 7 additions & 6 deletions x-pack/examples/reporting_example/public/containers/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ import type {
JobParamsPDFV2,
JobParamsPNGV2,
} from '../../../../plugins/reporting/public';
import { constants, ReportingStart } from '../../../../plugins/reporting/public';
import { LayoutTypes } from '../../../../plugins/screenshotting/public';
import { ReportingStart } from '../../../../plugins/reporting/public';

import { REPORTING_EXAMPLE_LOCATOR_ID } from '../../common';

Expand Down Expand Up @@ -87,7 +88,7 @@ export const Main = ({ basename, reporting, screenshotMode }: ReportingExampleAp
const getPDFJobParamsDefault = (): JobAppParamsPDF => {
return {
layout: {
id: constants.LAYOUT_TYPES.PRESERVE_LAYOUT,
id: LayoutTypes.PRESERVE_LAYOUT,
},
relativeUrls: ['/app/reportingExample#/intended-visualization'],
objectType: 'develeloperExample',
Expand All @@ -99,7 +100,7 @@ export const Main = ({ basename, reporting, screenshotMode }: ReportingExampleAp
return {
version: '8.0.0',
layout: {
id: constants.LAYOUT_TYPES.PRESERVE_LAYOUT,
id: LayoutTypes.PRESERVE_LAYOUT,
},
locatorParams: [
{ id: REPORTING_EXAMPLE_LOCATOR_ID, version: '0.5.0', params: { myTestState: {} } },
Expand All @@ -114,7 +115,7 @@ export const Main = ({ basename, reporting, screenshotMode }: ReportingExampleAp
return {
version: '8.0.0',
layout: {
id: constants.LAYOUT_TYPES.PRESERVE_LAYOUT,
id: LayoutTypes.PRESERVE_LAYOUT,
},
locatorParams: {
id: REPORTING_EXAMPLE_LOCATOR_ID,
Expand All @@ -131,7 +132,7 @@ export const Main = ({ basename, reporting, screenshotMode }: ReportingExampleAp
return {
version: '8.0.0',
layout: {
id: constants.LAYOUT_TYPES.PRESERVE_LAYOUT,
id: LayoutTypes.PRESERVE_LAYOUT,
},
locatorParams: {
id: REPORTING_EXAMPLE_LOCATOR_ID,
Expand All @@ -148,7 +149,7 @@ export const Main = ({ basename, reporting, screenshotMode }: ReportingExampleAp
return {
version: '8.0.0',
layout: {
id: print ? constants.LAYOUT_TYPES.PRINT : constants.LAYOUT_TYPES.PRESERVE_LAYOUT,
id: print ? LayoutTypes.PRINT : LayoutTypes.PRESERVE_LAYOUT,
dimensions: {
// Magic numbers based on height of components not rendered on this screen :(
height: 2400,
Expand Down
11 changes: 0 additions & 11 deletions x-pack/plugins/reporting/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,6 @@ export const UI_SETTINGS_CSV_SEPARATOR = 'csv:separator';
export const UI_SETTINGS_CSV_QUOTE_VALUES = 'csv:quoteValues';
export const UI_SETTINGS_DATEFORMAT_TZ = 'dateFormat:tz';

export const LAYOUT_TYPES = {
CANVAS: 'canvas',
PRESERVE_LAYOUT: 'preserve_layout',
PRINT: 'print',
};

export const DEFAULT_VIEWPORT = {
width: 1950,
height: 1200,
};

// Export Type Definitions
export const CSV_REPORT_TYPE = 'CSV';
export const CSV_JOB_TYPE = 'csv_searchsource';
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/reporting/common/test/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import type { ReportMock } from './types';
const buildMockReport = (baseObj: ReportMock) => ({
index: '.reporting-2020.04.12',
migration_version: '7.15.0',
browser_type: 'chromium',
max_attempts: 1,
timeout: 300000,
created_by: 'elastic',
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/reporting/common/types/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { Ensure, SerializableRecord } from '@kbn/utility-types';
import type { LayoutParams } from './layout';
import type { LayoutParams } from '../../../screenshotting/common';
import { LocatorParams } from './url';

export type JobId = string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import type { LayoutParams } from '../layout';
import type { LayoutParams } from '../../../../screenshotting/common';
import type { BaseParams, BasePayload } from '../base';

interface BaseParamsPNG {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { LocatorParams } from '../url';
import type { LayoutParams } from '../layout';
import type { LayoutParams } from '../../../../screenshotting/common';
import type { BaseParams, BasePayload } from '../base';

// Job params: structure of incoming user request data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import type { LayoutParams } from '../layout';
import type { LayoutParams } from '../../../../screenshotting/common';
import type { BaseParams, BasePayload } from '../base';

interface BaseParamsPDF {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { LocatorParams } from '../url';
import type { LayoutParams } from '../layout';
import type { LayoutParams } from '../../../../screenshotting/common';
import type { BaseParams, BasePayload } from '../base';

interface BaseParamsPDFV2 {
Expand Down
17 changes: 0 additions & 17 deletions x-pack/plugins/reporting/common/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@
* 2.0.
*/

import type { Size, LayoutParams } from './layout';
import type { JobId, BaseParams, BaseParamsV2, BasePayload, BasePayloadV2 } from './base';

export type { JobId, BaseParams, BaseParamsV2, BasePayload, BasePayloadV2 };
export type { Size, LayoutParams };
export type {
DownloadReportFn,
IlmPolicyMigrationStatus,
Expand All @@ -20,20 +18,6 @@ export type {
} from './url';
export * from './export_types';

export interface PageSizeParams {
pageMarginTop: number;
pageMarginBottom: number;
pageMarginWidth: number;
tableBorderWidth: number;
headingHeight: number;
subheadingHeight: number;
}

export interface PdfImageSize {
width: number;
height?: number;
}

export interface ReportDocumentHead {
_id: string;
_index: string;
Expand Down Expand Up @@ -83,7 +67,6 @@ export interface ReportSource {
*/
kibana_name?: string; // for troubleshooting
kibana_id?: string; // for troubleshooting
browser_type?: string; // no longer used since chromium is the only option (used to allow phantomjs)
timeout?: number; // for troubleshooting: the actual comparison uses the config setting xpack.reporting.queue.timeout
max_attempts?: number; // for troubleshooting: the actual comparison uses the config setting xpack.reporting.capture.maxAttempts
started_at?: string; // timestamp in UTC
Expand Down
24 changes: 0 additions & 24 deletions x-pack/plugins/reporting/common/types/layout.ts

This file was deleted.

1 change: 1 addition & 0 deletions x-pack/plugins/reporting/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"uiActions",
"taskManager",
"embeddable",
"screenshotting",
"screenshotMode",
"share",
"features"
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/reporting/public/lib/job.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ export class Job {
public timeout: ReportSource['timeout'];
public kibana_name: ReportSource['kibana_name'];
public kibana_id: ReportSource['kibana_id'];
public browser_type: ReportSource['browser_type'];

public size?: ReportOutput['size'];
public content_type?: TaskRunResult['content_type'];
Expand Down Expand Up @@ -80,7 +79,6 @@ export class Job {
this.timeout = report.timeout;
this.kibana_name = report.kibana_name;
this.kibana_id = report.kibana_id;
this.browser_type = report.browser_type;
this.browserTimezone = report.payload.browserTimezone;
this.size = report.output?.size;
this.content_type = report.output?.content_type;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,6 @@ export const ReportInfoFlyoutContent: FunctionComponent<Props> = ({ info }) => {
}),
description: info.layout?.id || UNKNOWN,
},
{
title: i18n.translate('xpack.reporting.listing.infoPanel.browserTypeInfo', {
defaultMessage: 'Browser type',
}),
description: info.browser_type || NA,
},
Comment on lines -144 to -149
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

];

const warnings = info.getWarnings();
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/reporting/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
Plugin,
PluginInitializerContext,
} from 'src/core/public';
import type { ScreenshottingSetup } from '../../screenshotting/public';
import { CONTEXT_MENU_TRIGGER } from '../../../../src/plugins/embeddable/public';
import {
FeatureCatalogueCategory,
Expand Down Expand Up @@ -73,6 +74,7 @@ export interface ReportingPublicPluginSetupDendencies {
management: ManagementSetup;
licensing: LicensingPluginSetup;
uiActions: UiActionsSetup;
screenshotting: ScreenshottingSetup;
share: SharePluginSetup;
}

Expand Down Expand Up @@ -145,6 +147,7 @@ export class ReportingPublicPlugin
home,
management,
licensing: { license$ }, // FIXME: 'license$' is deprecated
screenshotting,
share,
uiActions,
} = setupDeps;
Expand Down Expand Up @@ -203,7 +206,7 @@ export class ReportingPublicPlugin
id: 'reportingRedirect',
mount: async (params) => {
const { mountRedirectApp } = await import('./redirect');
return mountRedirectApp({ ...params, share, apiClient });
return mountRedirectApp({ ...params, apiClient, screenshotting, share });
},
title: 'Reporting redirect app',
searchable: false,
Expand Down
Loading