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] Create reports with full state required to generate the report #101048

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jun 1, 2021

Summary

New export types

  • printable_pdf_v2
  • png_v2

The general strategy was to copy as much as possible from the existing PDF and PNG export types altering only how the existing export types handle their URLs. Specifically, we want to use locators to power forwarding state and navigation.

Reporting client-side redirect app

This contribution also introduces a new Reporting redirect client app that will be used by the new export types to load app state without using URLs. The strategy is as follows:

  1. Client app requests a new report of type PNG/PDF v2 and provides locator params containing the client's locator ID (i.e., clients need a locator implementation), the state version (which will be used to migrate locator state) and the actual state (contained inside of params). Collectively these are the locatorParams and they are stored in an array so that multiple pages can be included in single report.
  2. Reporting creates a new job saved object that will contain the locatorParams.
  3. When the report job executes, the job saved object will be loaded and a URL + locatorParam combination will be created for each page that needs to be included in the report.
  4. Each of the URLs is navigated to at which point the redirect app will read the job locatorParams injected by chromium and use each of these to retrieve a locator
  5. Navigation is performed the share plugin which also migrates the app state

This contribution does not contain

The following should be addressed in a follow up PR(s):

  • A way to pass on forceNow to apps that are generating vis. It is not clear that Reporting should need to do this in practice, the plugin rendering the UI should be able to determine this via the locator state or by detecting that they are in screenshot mode (or a combination of those). Open to other input here though!
  • Documentation about how to integrate, although the reporting example app was updated to use both the PDF and PNG v2 export types.
  • Test coverage for the new export types
  • Improved redirect error handling

To reviewers

To test this functionality:

  1. Start kibana (with yarn start --run-examples) and ES on basic
  2. Go to the "Developer examples" plugins
  3. Go to Reporting integration
  4. Generate a PDF/PNG

You should get a report that looks like:

Screenshot 2021-07-22 at 15 28 41

  • note the additional forwarded state and the migrated value that gets added from running the locator based migrations
  • note the forceNow value that is added to the locator params. Current thinking is that consumer locators can use this to forward to the place we are navigating to control the "now" of when the report is being generated.

Notes on the current implementation

We wanted to preserve all state stored on report/job saved object. This meant using the short URL service is slightly less desirable (but possible).

We also did not want to rewrite the share redirect endpoint to achieve this functionality.

Screenshots

Error state in redirect app

(This particular error is out-of-date with the current code, but the presentation of errors is still the same).

Screenshot 2021-07-15 at 11 31 37

Future work

  • Tests! It would be great to add a test that checks whether a given PDF matches a snapshot PDF (as suggested by @tsullivan )
  • Improved error reporting from the redirect process
  • A nice enhancement would be for the new v2 reports to link users from the Reporting UI to the reporting location in Kibana (with full app state forwarded)
  • Write docs for how to integrate with the new v2 PDF/PNG report types
  • Update integration points (like dashboard)

Checklist

  the report for **PDF**
- Removed v2 of the reporting since it looks like we may be able
  to make a backwards compatible change on existing PDF/PNG
  exports
@jloleysens jloleysens added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:feature Makes this part of the condensed release notes Team:AppServices labels Jun 1, 2021
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

kibanamachine and others added 4 commits June 2, 2021 08:14
…sens/kibana into reporting/new-png-pdf-report-type

* 'reporting/new-png-pdf-report-type' of github.com:jloleysens/kibana: (46 commits)
  [Security Solution] Add Ransomware canary advanced policy option (elastic#101068)
  [Exploratory view] Core web vitals (elastic#100320)
  [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034)
  [Lens] Use a setter function for the dimension panel (elastic#101123)
  [Index Patterns] Fix return saved index pattern object (elastic#101051)
  [CI] For PRs, build TS refs before public api docs check (elastic#100791)
  [Maps] fix line and polygon label regression (elastic#101085)
  Migrate CCR to new ES JS client. (elastic#100131)
  [Canvas] Switch Canvas to use React Router (elastic#100579)
  [Expressions] Use table column ID instead of name when set (elastic#99724)
  [DOCS] Updates docs landing page (elastic#100749)
  [DOCS] Corrects typo in step 3 (elastic#101079)
  [DOCS] Updates runtime example in Discover (elastic#100926)
  Migrate kibana.autocomplete config to data plugin (elastic#100586)
  [Uptime] New width/delay definition for waterfall sidebar item tooltip (elastic#100147)
  [FTR] Use importExport for saved_object/basic archive (elastic#100244)
  [Fleet] Better input for multi text input in agent policy builder (elastic#101020)
  [CI] Buildkite support with Baseline pipeline (elastic#100492)
  [Reporting/Telemetry] Do not send telemetry if we are in screenshot mode (elastic#100388)
  Create API keys with metadata (elastic#100682)
  ...
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (447 commits)
  skip flaky suite (elastic#102366)
  [Security Solution][Endpoint][Host Isolation] Isolation status badge from alert details (elastic#102274)
  Add email connector info for Elastic Cloud (elastic#91363)
  [Workplace Search] remove or replace xs props for text on source connect view (elastic#102663)
  Do not double register dashboard url generator (elastic#102599)
  [TSVB] Replaces EuiCodeEditor 👉 Monaco editor  (elastic#100684)
  [Discover] Update kibana.json adding owner and description (elastic#102292)
  [Exploratory View] Mobile experience (elastic#99565)
  chore(NA): moving @kbn/ui-shared-deps into bazel (elastic#101669)
  [TSVB] Index pattern select field disappear in Annotation tab (elastic#102314)
  [Security Solution][Endpoint][Host Isolation] Fixes bug where host isolation/unisolation works from alert details (elastic#102581)
  TSVB visualizations with no timefield do not render after upgrading from 7.12.1 to 7.13.0 (elastic#102494)
  [Logs UI] Add `event.original` fallback to message reconstruction rules (elastic#102236)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (253 commits)
  [Enterprise Search] Test coverage pass (elastic#103406)
  [App Search] Success toast polish pass  (elastic#103410)
  Role mappings: remove deprecated reset copy (elastic#103411)
  [Fleet] Add a message when log collection is disabled on the log details page (elastic#103166)
  [Query]  Remove es query dependency on format.convert (elastic#103174)
  Home & Kibana Overview Page Template Update (elastic#103003)
  [ML] Converts management app jobs list pages to new layout (elastic#103117)
  Allow additive csp configuration (elastic#102059)
  [Lens] Document common formulas in product and add formula tutorial (elastic#103154)
  [Lens] Enable actions on Lens Embeddable (elastic#102038)
  [Osquery] Return proper indices permissions for osquery_manager package (elastic#103363)
  Dashboard locator (elastic#102854)
  Maps locators (elastic#102810)
  [Fleet] Add support for constant_keyword "value" in package field definitions (elastic#103000)
  [Maps] Add capability to delete features from layer & index (elastic#103145)
  [Security Solution] Correct linux OS lookup for Endpoint Exceptions (elastic#103038)
  [Enterprise Search] Add notices for deactivated users and SMTP callout (elastic#103285)
  [canvas] Reduce bundle size by combining SCSS imports (elastic#102822)
  [Enterprise Search] Final KibanaPageTemplate cleanup (elastic#103355)
  [docs][migrations v2] Update SO migration docs to include removal of index write block when handling corrupt SOs. (elastic#103014)
  ...
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (178 commits)
  [test] Migrating to kbn_archiver from es_archiver - for the Maps app (elastic#103028)
  [Reporting] Reintroduce "ILM policy for managing reporting indices" (elastic#103850)
  [Security Solution][Endpoint] Allow activity log scrolling on small screens (elastic#103852)
  Allow zero (0) to unset unenroll_timeout field (elastic#103790)
  [TSVB] Metric count is depicted as `-` instead of 0 (elastic#103717)
  [Query] Es query/field base (elastic#103177)
  Remove add data button from nav (elastic#103810)
  Fix telemetry advanced setting style (elastic#103838)
  [Transform] Fix default naming and sorting fields suggestion for `top_metrics` agg (elastic#103690)
  [APM] use conventional error rate color for correlations (elastic#103500)
  Endpoint Telemetry: Agents Metrics + Policy Config / Response (elastic#102171)
  [Alerting] Fixed search results are not updated when search term is removed on Rules and Connectors page (elastic#103663)
  fix too many rernders (elastic#103672)
  [APM] Add “Analyze Data” button (elastic#103485)
  [Lens] Fix value popover spacing (elastic#103081)
  [TSVB] Fix TSVB is not reporting all categories of Elasticsearch error (elastic#102926)
  [SECURITY] Adds security links to doc link service (elastic#102676)
  Update dependency @elastic/charts to v31 (elastic#102078)
  [Security Solution][CTI] Investigation time enrichment UI (elastic#103383)
  Adds ECS guide to doc links service (elastic#102246)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (292 commits)
  bring back KQL autocomplete in timeline + fix last updated (elastic#105380)
  [Maps] Change TOC pop-up wording to reflect filter change, not search bar change (elastic#105163)
  Updating urls to upstream elastic repo (elastic#105250)
  [Maps] Move new vector layer wizard card down (elastic#104797)
  Exclude registering the cases feature if not enabled (elastic#105292)
  [Uptime] Alerts - Monitor status alert - check monitor status by monitor.timespan (elastic#104541)
  updated UI copy (elastic#105184)
  Log a warning when documents of unknown types are detected during migration (elastic#105213)
  [Logs UI] Register log threshold rule as lifecycle rule (elastic#104341)
  [Ingest pipelines] add network direction processor (elastic#103436)
  [Console] Autocomplete definitions (manual backport) (elastic#105086)
  [Security Solution] User can make Exceptions for Memory protection alerts (elastic#102196)
  [Lens] Formula: add validation for multiple field/metrics (elastic#104092)
  Removing async from file upload and data visualizer plugins start lifecycle (elastic#105197)
  Fix error when validating the form with non blocking validations (elastic#103629)
  [ML] Fix "View by" swim lane with applied filter and sorting by score  (elastic#105217)
  Update dependency @elastic/charts to v32 (elastic#104625)
  [CTI] shortens large numbers on Dashboard Link Panel (elastic#105269)
  [Security Solution][Endpoint][Host Isolation] Fixes bug to remove excess host metadata status toasts on non user initiated errors (elastic#105331)
  [Cases] Fix pushing alerts count on every push to external service (elastic#105030)
  ...

# Conflicts:
#	x-pack/plugins/reporting/common/types.ts
const pageLoadSelector =
page > 1 ? `[data-shared-page="${page}"]` : DEFAULT_PAGELOAD_SELECTOR;
p > 1 ? `[data-shared-page="${p}"]` : DEFAULT_PAGELOAD_SELECTOR;
Copy link
Member

@tsullivan tsullivan Aug 10, 2021

Choose a reason for hiding this comment

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

Perhaps all of the above code in this function could just be part of the openUrl function, to factor out some logic from this complex file?

},
},
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's consolidate these:

  • x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/pdf
  • x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf

into:

  • x-pack/plugins/reporting/server/export_types/common/pdf

Copy link
Contributor Author

@jloleysens jloleysens Aug 11, 2021

Choose a reason for hiding this comment

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

This shaved off about 500 lines from this PR!

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.

A few more comments to help move things forward.

…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (101 commits)
  [ML] APM Latency Correlations: Field/value candidates prioritization (elastic#107370)
  [Reporting] Add lenience to a test on the order of asserted logs (elastic#108135)
  [Lens] fix do not submit invalid query in filtered metric (elastic#107542)
  skip flaky test (elastic#108043)
  fix newly introduced type error (elastic#107593)
  [Reporting] server side code clean up (elastic#106940)
  [build_ts_refs] improve caches, allow building a subset of projects (elastic#107981)
  [APM] Add new ftr_e2e to kibana CI and remove current e2e tests. (elastic#107593)
  add manage rules link to alerts dropdown (elastic#107950)
  [ML] Enable Index data visualizer document count chart to update time range query (elastic#106438)
  [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (elastic#107230)
  [Actions UI] Fixed Jira Api token label. (elastic#107776)
  [Alerting UI] Fixed display permissions for edit/delete buttons when user has read only access. (elastic#107996)
  [Maps] fix code owners (elastic#108106)
  Update EMS landing page url (elastic#108102)
  Do not render page header for loading domains (elastic#108078)
  Update dependency @elastic/charts to v33.2.2 (elastic#107939)
  [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds (elastic#107850)
  [Fleet] Fix all category count (elastic#108089)
  [Security Solution][Bug] - Disable alert table RBAC until fields sorted (elastic#108034)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/lib/screenshots/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
@jloleysens jloleysens added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 12, 2021
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

Thanks for the review @tsullivan ! I think I have addressed all of your feedback. Would you mind taking another look?

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! Great job! Looking forward to having these changes merged in.

@jloleysens jloleysens merged commit f08005e into elastic:master Aug 12, 2021
@jloleysens jloleysens deleted the reporting/new-png-pdf-report-type branch August 12, 2021 16:40
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 12, 2021
…eport (elastic#101048)

* very wip

* - Reached first iteration of reporting body value being saved with
  the report for **PDF**
- Removed v2 of the reporting since it looks like we may be able
  to make a backwards compatible change on existing PDF/PNG
  exports

* reintroduced pdfv2 export type, see https:/elastic/kibana/issues/99890\#issuecomment-851527878

* fix a whol bunch of imports

* mapped out a working version for pdf

* refactor to tuples

* added v2 pdf to export type registry

* a lot of hackery to get reports generated in v2

* added png v2, png reports with locator state

* wip: refactored for loading the saved object on the redirect app URL

* major wip: initial stages of reporting redirect app, need to add a way to generate v2 reports!

* added a way to generate a v2 pdf from the example reporting plugin

* updated reporting example app to read and accept forwarded app state

* added reporting locator and updated server-side route to not use Boom

* removed reporting locator for now, first iteration of reports being generated using the reporting redirect app

* version with PNG working

* moved png/v2 -> png_v2

* moved printable_pdf/v2 -> printable_pdf_v2

* updated share public setup and start mocks

* fix types after merging master

* locator -> locatorParams AND added a new endpoint for getting locator params to client

* fix type import

* fix types

* clean up bad imports

* forceNow required on v2 payloads

* reworked create job interface for PNG task payload and updated consumer code report example for forcenow

* put locatorparams[] back onto the reportsource interface because on baseparams it conflicts with the different export type params

* move getCustomLogo and generatePng to common for export types

* additional import fixes

* urls -> url

* chore: fix and update types and fix jest import mocks

* - refactored v2 behaviour to avoid client-side request for locator
  instead this value is injected pre-page-load so that the
  redirect app can use it
- refactored the interface for the getScreenshot observable
  factory. specifically we now expect 'urlsOrUrlTuples' to be
  passed in. tested with new and old report types.

* updated the reporting example app to use locator migration for v2 report types

* added functionality for setting forceNow

* added forceNow to job payload for v2 report types and fixed shared components for v2

* write the output of v2 reports to stream

* fix types for forceNow

* added tests for execute job

* added comments, organized imports, removed selectors from report params

* fix some type issues

* feedback: removed duplicated PDF code, cleaned screenshot observable function and other minor tweaks

* use variable (not destructured values) and remove unused import

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

jloleysens added a commit that referenced this pull request Aug 13, 2021
…eport (#101048) (#108404)

* very wip

* - Reached first iteration of reporting body value being saved with
  the report for **PDF**
- Removed v2 of the reporting since it looks like we may be able
  to make a backwards compatible change on existing PDF/PNG
  exports

* reintroduced pdfv2 export type, see https:/elastic/kibana/issues/99890\#issuecomment-851527878

* fix a whol bunch of imports

* mapped out a working version for pdf

* refactor to tuples

* added v2 pdf to export type registry

* a lot of hackery to get reports generated in v2

* added png v2, png reports with locator state

* wip: refactored for loading the saved object on the redirect app URL

* major wip: initial stages of reporting redirect app, need to add a way to generate v2 reports!

* added a way to generate a v2 pdf from the example reporting plugin

* updated reporting example app to read and accept forwarded app state

* added reporting locator and updated server-side route to not use Boom

* removed reporting locator for now, first iteration of reports being generated using the reporting redirect app

* version with PNG working

* moved png/v2 -> png_v2

* moved printable_pdf/v2 -> printable_pdf_v2

* updated share public setup and start mocks

* fix types after merging master

* locator -> locatorParams AND added a new endpoint for getting locator params to client

* fix type import

* fix types

* clean up bad imports

* forceNow required on v2 payloads

* reworked create job interface for PNG task payload and updated consumer code report example for forcenow

* put locatorparams[] back onto the reportsource interface because on baseparams it conflicts with the different export type params

* move getCustomLogo and generatePng to common for export types

* additional import fixes

* urls -> url

* chore: fix and update types and fix jest import mocks

* - refactored v2 behaviour to avoid client-side request for locator
  instead this value is injected pre-page-load so that the
  redirect app can use it
- refactored the interface for the getScreenshot observable
  factory. specifically we now expect 'urlsOrUrlTuples' to be
  passed in. tested with new and old report types.

* updated the reporting example app to use locator migration for v2 report types

* added functionality for setting forceNow

* added forceNow to job payload for v2 report types and fixed shared components for v2

* write the output of v2 reports to stream

* fix types for forceNow

* added tests for execute job

* added comments, organized imports, removed selectors from report params

* fix some type issues

* feedback: removed duplicated PDF code, cleaned screenshot observable function and other minor tweaks

* use variable (not destructured values) and remove unused import

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Jean-Louis Leysens <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
reporting 59 65 +6

Async chunks

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

id before after diff
reporting 68.3KB 71.1KB +2.8KB

Page load bundle

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

id before after diff
reporting 65.0KB 67.1KB +2.1KB
share 83.1KB 83.4KB +256.0B
total +2.4KB
Unknown metric groups

async chunk count

id before after diff
reporting 3 4 +1

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants