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

Conversation

dokmic
Copy link
Contributor

@dokmic dokmic commented Dec 1, 2021

Summary

Resolves #116503. This plugin moves all the screenshot taking logic out of the reporting plugin.

Checklist

For maintainers


layout: {
zoom: config.get('capture', 'zoom'),
...options.layout,
Copy link
Member

@tsullivan tsullivan Dec 1, 2021

Choose a reason for hiding this comment

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

It feels like all of these config options could be moved over and given the renameFromRoot treatment, as the others have. Let's do that in another PR, unless you object.

  • xpack.reporting.capture.timeouts
  • xpack.reporting.capture.zoom
  • xpack.reporting.capture.loadDelay

@dokmic dokmic force-pushed the feature/KS-626 branch 2 times, most recently from 823b8b9 to 66391b4 Compare December 3, 2021 13:59
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

This is looking really good so far @dokmic !

Might be a pre-existing issue but generating a PDF report from Canvas resulted in:

[2021-12-03T15:24:37.939+01:00][WARN ][plugins.screenshotting.screenshot.browser-driver] Reporting encountered an uncaught error on the page that will be ignored: Error: TypeError: Cannot read property 'setScreenshotLayout' of undefined
    at <anonymous>:1:31
    at <anonymous>:1:58

being logged. This was really just a warning since the report did generate successfully. Not sure why that happened though, could be that Canvas just does not depend on screenshot mode.

I tested in Canvas and Dashboard and everything worked as expected. Great work!

[UPDATE]

Happy to merge once CI is green ;)

Comment on lines -144 to -149
{
title: i18n.translate('xpack.reporting.listing.infoPanel.browserTypeInfo', {
defaultMessage: 'Browser type',
}),
description: info.browser_type || NA,
},
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!

@dokmic dokmic force-pushed the feature/KS-626 branch 4 times, most recently from 6eb0310 to 6032c78 Compare December 3, 2021 17:48
jobLogger.error(err);
return Rx.throwError(err);
}),
tap({ error: (error) => jobLogger.error(error) }),
Copy link
Member

Choose a reason for hiding this comment

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

much better 😅

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

Pulled the branch and tested the code. Did yarn build to test in a VM.

Reviewed the code by merging main and looking at git diff -M -w main

@dokmic dokmic marked this pull request as ready for review December 6, 2021 08:59
@dokmic dokmic requested review from a team as code owners December 6, 2021 08:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

@dokmic
Copy link
Contributor Author

dokmic commented Dec 6, 2021

Might be a pre-existing issue but generating a PDF report from Canvas resulted in:

[2021-12-03T15:24:37.939+01:00][WARN ][plugins.screenshotting.screenshot.browser-driver] Reporting encountered an uncaught error on the page that will be ignored: Error: TypeError: Cannot read property 'setScreenshotLayout' of undefined
    at <anonymous>:1:31
    at <anonymous>:1:58

That was an actual bug that is fixed now. Thanks, @jloleysens.

@jloleysens jloleysens self-requested a review December 6, 2021 11:54
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Did not re-test, but approving based on previous testing. Great work!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
screenshotting - 8 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
reporting 162 163 +1
screenshotting - 11 +11
total +12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
reporting 56.3KB 56.3KB -41.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
reporting 14 15 +1
screenshotting - 5 +5
total +6

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
reporting 42.1KB 41.8KB -224.0B
screenshotting - 2.0KB +2.0KB
total +1.8KB
Unknown metric groups

API count

id before after diff
reporting 165 166 +1
screenshotting - 25 +25
total +26

References to deprecated APIs

id before after diff
reporting 27 25 -2
screenshotting - 12 +12
total +10

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Removing the browser_type usage field is ok for telemetry. Removing the field from the index mapping would require a reindex, as the indices are strictly mapped.
LGTM

@@ -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.

Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

LGTM

@dokmic dokmic merged commit 903e75e into elastic:main Dec 6, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 7, 2021
…-chromium-before-compiling-pdf

* 'main' of github.com:elastic/kibana: (121 commits)
  FTR should use the new kibana_system user (elastic#120436)
  [Lens] Temporarily exclude Mosaic/Waffle from the suggestions list (elastic#120488)
  [Reporting] Fix slow CSV with large max size bytes (elastic#120365)
  [CTI] Threat Intel Card on Overview page needs to accommodate Fleet TI integrations -  (elastic#120459)
  [Dashboard] Added KibanaThemeProvider.  (elastic#120122)
  add more rule_registry unit tests (elastic#120323)
  [Lens] update xpack.lens.pie.smallValuesWarningMessage text (elastic#120478)
  [Fleet] Simplified package policy create API, enriching default values (elastic#119739)
  mock `elastic-apm-node` in `@kbn/test` jest preset (elastic#120324)
  [RAC] Rename occurrences of alert_type to rule_type in Infra (elastic#120455)
  [Security Solutions] Removes tech debt of exporting all from linter rule for timeline plugin (elastic#120437)
  [Workplace Search] Add API Keys view to replace Access tokens (elastic#120147)
  [Security Solutions] Removes tech debt of exporting all from linter rule for cases plugin in the server section (elastic#120411)
  Add support for threat.feed.name (elastic#120250)
  [Rule Registry] Rewrite APM registry rules for Observability (elastic#117740)
  [docs] Fix artifact layout formatting (elastic#119386)
  [Workplace Search] Add a technical preview of connecting GitHub via GitHub apps (elastic#119764)
  add osquery notes for 7.16 (elastic#120407)
  chore(NA): splits types from code on @kbn/docs-utils (elastic#120533)
  [Reporting] Decouple screenshotting plugin from the reporting (elastic#120110)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.test.ts
#	x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts
@yakhinvadim
Copy link
Contributor

yakhinvadim commented Dec 7, 2021

I'm on M1X chip and I see an error when running yarn start that looks related to this PR.

Here's what I have in the console:

[2021-12-07T11:02:37.881-08:00][INFO ][plugins.screenshotting.config] Chromium sandbox provides an additional layer of protection, and is supported for Darwin OS. Automatically enabling Chromium sandbox.
[2021-12-07T11:02:37.881-08:00][ERROR][plugins.screenshotting] Error in screenshotting setup, it may not function properly.
Unhandled Promise rejection detected:

Error: Unsupported platform: darwin-arm64
    at install (/Users/yakhinvadim/Projects/kibana/x-pack/plugins/screenshotting/server/browsers/install.ts:37:11)
    at /Users/yakhinvadim/Projects/kibana/x-pack/plugins/screenshotting/server/plugin.ts:47:132
    at ScreenshottingPlugin.setup (/Users/yakhinvadim/Projects/kibana/x-pack/plugins/screenshotting/server/plugin.ts:53:7)
    at PluginWrapper.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/plugins/plugin.ts:108:26)
    at PluginsSystem.setupPlugins (/Users/yakhinvadim/Projects/kibana/src/core/server/plugins/plugins_system.ts:129:40)
    at PluginsService.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/plugins/plugins_service.ts:171:52)
    at Server.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/server.ts:282:26)
    at Root.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/root/index.ts:52:14)
    at bootstrap (/Users/yakhinvadim/Projects/kibana/src/core/server/bootstrap.ts:101:5)
    at Command.<anonymous> (/Users/yakhinvadim/Projects/kibana/src/cli/serve/serve.js:244:5)

Terminating process...
 server crashed  with status code 1

Let me know if I can help with the debugging. Happy to pair.


EDIT: Just found that a PR has already been created to fix it! #120659

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 8, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 120110 or prevent reminders by adding the backport:skip label.

@tsullivan tsullivan removed the v8.0.0 label Dec 8, 2021
@tsullivan
Copy link
Member

Removed the 8.0 version label as we are past feature freeze for that branch

@dokmic dokmic added the v8.0.0 label Dec 9, 2021
dokmic added a commit to dokmic/kibana that referenced this pull request Dec 9, 2021
…c#120110)

* Add screenshotting plugin
* Move screenshotting plugin configuration options
* Remove unused browser type configuration option
# Conflicts:
#	packages/kbn-optimizer/limits.yml
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/export_types/png/execute_job/index.ts
#	x-pack/plugins/reporting/server/export_types/png_v2/execute_job.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf/lib/tracker.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/tracker.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
dokmic added a commit that referenced this pull request Dec 9, 2021
… (#120937)

* Add screenshotting plugin
* Move screenshotting plugin configuration options
* Remove unused browser type configuration option
# Conflicts:
#	packages/kbn-optimizer/limits.yml
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/export_types/png/execute_job/index.ts
#	x-pack/plugins/reporting/server/export_types/png_v2/execute_job.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf/lib/tracker.ts
#	x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/tracker.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 9, 2021
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
…c#120110)

* Add screenshotting plugin
* Move screenshotting plugin configuration options
* Remove unused browser type configuration option
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
…c#120110)

* Add screenshotting plugin
* Move screenshotting plugin configuration options
* Remove unused browser type configuration option
@dokmic dokmic deleted the feature/KS-626 branch July 29, 2022 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screenshot Service