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

[Discussion] Generating PDF/PNG reports should not require a saved object in plugin #99890

Closed
jloleysens opened this issue May 12, 2021 · 12 comments · Fixed by #108553
Closed
Labels
discuss Feature:Reporting:Framework Reporting issues pertaining to the overall framework impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort needs-team Issues missing a team label

Comments

@jloleysens
Copy link
Contributor

jloleysens commented May 12, 2021

Summary

There are a number of places in Kibana that offer users the ability to "share" their current view by creating a PDF or PNG. This works by capturing the user's current browser state, rendering it in our report generator server side, then saving the output in Elasticsearch.

The full representation of the user's current state/view/visualisation is not necessarily captured in the URL. Typically state not captured in the URL is in a saved object, referenced in the URL, which is then loaded from Elasticsearch when generating the report.

Complication

This creates a situation where users are required to create persisted state in order to generate a report. The problem with persisted state is that users may be creating a one-off report (#18439 (comment)) and do not necessarily want to keep additional state like this in the cluster.

Solutions

There are a few ways in which this can be addressed, here are a few:

1. Read all initial app state from the URL Service

Wherever we offer the ability to "share" a view/visualisation as PDF/PNG the current view should be able to read entirely from the URL. App state does not need to be maintained in the URL, but it should be read on initial load. This will bring PDF/PNG reporting in line with CSV reporting.

How this might work from Reporting using new URL service (quoting @tsullivan):

  1. We create a new export type that takes a serialized state object
  2. When the job executes, we determine the URL based on the report job params:
    const locator = locatorClient.get(jobParams.locator.id);
    const url = locator.getLocation(jobParams.locator.params);
  3. Create a new page with Puppeteer, then open the app:
    page.open(url);

Challenges

  • This might be an issue. URLs have a character/byte length limit that may restrict the feasibility of this approach. This restriction is usually imposed by the webserver (e.g. nginx has a limit of 8K bytes on request header size). However, it might be the case that URL length will not feasibly reach this limits even if all the state is captured. This issue only exists if the request passes through a webserver, which in case of report generation it will not.
  • Chromium URL length limits? @timroes indicated this is an existing issue for dashboard.

Questions

To plugin owners: How much work will it be to get, for instance, dashboard, to a state where everything it needs is captured in the URL?

2. Offer an option to print on the client

Another way to give users the option to not create a saved object is to provide the option to view the "print-friendly" version of the view/visualisation locally and use the print functionality offered by the browser.

This approach has the advantage of making use of existing print options available to users and is in line with our expectation that in future plugins should take care of providing the print-friendly view. See this comment #93496 (comment), specifically point 1:

  1. Apps need to provide reporting with their "print/report-friendly" url and work to keep their reporting UI as lightweight as possible.

Questions

To plugin owners: How much work will it be to create "print-friendly" views that can both be used by reporting and the user's browser?

3. Make saving additional state as effortless as possible

Another solution, outlined here: #18439 (comment). We can update the UI to do the save step and the generate step in one click. This will not address the core of the issue (unnecessary saved objects) but it will help reduce the number of steps users need to take (no need to click save and specify a name for the report).

This option can be done alongside the option to "Offer an option to print on the client" but will contradict the option that captures the full state in the URL.

Related issues:

@jloleysens jloleysens added discuss loe:needs-research This issue requires some research before it can be worked on or estimated impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Reporting Services labels May 12, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@jloleysens
Copy link
Contributor Author

cc @tsullivan

@elasticmachine
Copy link
Contributor

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

@tsullivan
Copy link
Member

tsullivan commented May 13, 2021

We should migrate away from using the URL as parameter field in the JobParams of PDF / PNG export. Instead, let's look at using a serialized object that can be interpreted by the URL Service: https://github.com/streamich/kibana/blob/master/rfcs/text/0017_url_service.md

Take

as an example.

Before:

interface BaseParamsPNG {
  layout: LayoutParams;
  forceNow?: string;
  relativeUrl: string;
} 

We can make breaking changes in job parameters, if it is defined as a new type of export.

After:

interface BaseParamsBetterPNG<P> {
  layout: LayoutParams;
  forceNow?: string;
  locator: {
    id: string;
    params: P;
  };
} 

The locator object possibly needs a version field as well, in case params need to go through migrations.

@tsullivan
Copy link
Member

cc @streamich

@jloleysens
Copy link
Contributor Author

jloleysens commented May 14, 2021

@timroes @stacey-gammon @flash1293

Pinging you as you might have input on this discussion :). If we agree that this is something we want to address it would be useful to get your thoughts. Specifically on option 1 and option 2.

[Edit] of course if there are other options that are not listed feel free to add them!

@timroes
Copy link
Contributor

timroes commented May 27, 2021

Hi thanks for listing those options.

I think we can rule out Option 1 safely here. We know that we can't put all the state needed in all apps into the URL. We just recently for that reason removed actually most of the state on Dashboard from the URL altogether, because with visualizations "by-value" (i.e. embedded directly in a dashboard), we could even for more simplistic features not create URLs anymore, since the full visualization state for each panel is required within the dashboard state.

In general we had some discussion which lead to the general understanding, that we don't try to store everything inside the URL anymore (e.g. at the beginning of creating Lens), and that this won't be the way forward, given how often we ran into problems with it in the past for the older applications that did store everything in the URL. Some of the initial suggestions happened in #35812 but reading through the issue I think our conclusion was made async (and badly captured for posterity later on, but we basically said the "the maps way" in that issue is the way forward for all apps).

I like Option 2 for reporting in general. When I talked in the past with Peter about that, this seemed to be our favorite option in general, since it will (a) give the advantage of actually having an easy to print page in the browser and (b) we largely anyway have that CSS already from the way the system currently works, we'll just need to scope it to printing.

My understanding reading your description above is, that this is then not meant to work serverside (which would otherwise still cause the same issue - how do we get the state there), but we basically just replace the serverside system for a browserside system (for temporary reports).

I see one alternative option, that by the original description has been ruled out, that I'd like to understand better why it's fully been ruled out: storing temporary state (like a short URL) bringing you to the fully inflated application when the report is generated. My understanding was, that we're trying to avoid that not to have that short URL object zombieing around afterwards? But couldn't we make this depend on the report job itself? I.e. when the report is generated one time, we just delete it after report generation again. When we'd have a scheduled report system (a specific one tied to reports - that as far as my understanding was we still want to have), we can delete it whenever that scheduled report is deleted. The only thing that we couldn't do for that case is offering a "cURL" URL for the user to trigger that report manually via their 3rd party system (or otherwise we'd never know when to delete that short URL). But maybe that's a valid (and not even really bothersome?) limitation of that system, that you cannot get an API endpoint to call for report generation if it's just a temporary state?

@jloleysens
Copy link
Contributor Author

jloleysens commented May 27, 2021

@timroes and I spoke offline and concluded that option 1 can be clarified and adjusted a bit:

  1. The proposal is not for dashboard to return to a state where full app state is stored and maintained in the URL, rather that dashboard read initial state from the URL service, in this way the URL service forms part of the "contract" by which Reporting can return the initial app state to dashboard
  2. Tim shared that this is still a problem because the URL for dashboard has, historically, exceeded the max char limits allowed in browsers (something I believe @ThomThomson knows most about and raised in a meeting we had).
  3. We will need to find an alternate way for the serialized state created by plugins to be passed back to them when generating the report. @tsullivan, specifically this means using the URL Service does not sound feasible. I was thinking we provide a way to expose this state through reporting, exposing something on public setup contract that exposes the state object.

[UPDATE]
It looks like I was mistaken about how exactly URL service will interact with navigations and setting URLs in the browser. Large state objects will not need to be set in the URL as the URL service will a hidden mechanism for sharing (setting/getting) this object.

@flash1293
Copy link
Contributor

flash1293 commented May 27, 2021

It's definitely possible to hit max url lengths with what were are doing on dashboards. Historically we solved this with moving the state to session storage. To keep it shareable, the short URL service allows to fetch the saved object via API call - a special redirect app does that and puts it directly into the session storage so the URL is never actually set as URL as far as the browser is concerned. This seems very similar to the last part here:

I was thinking we provide a way to expose this state through reporting, exposing something on public setup contract that exposes the state object.

There are some plans around this from the app-services team because we have the exact same problem for search session restoration (cc @Dosant ): #85126

Any solution usable for search sessions can probably be used for reporting in the same way

@jloleysens
Copy link
Contributor Author

jloleysens commented May 31, 2021

I spoke with @streamich about the new URL service and we concluded the following flow would be feasible for avoiding the long URL problem:

[EDIT] See updated flow: #99890 (comment)

  1. Plugin (e.g., Dashboard) requests a new report, passing the serialize(able) state object (in the form of a locator object) through share which passes it to shared Reporting UI component
  2. Reporting public requests a new report, sending serialized state to server for saving in the report saved object (SO)
  3. Reporting server uses chromium APIs to pass in the serialized state to be read by reporting public
  4. Reporting server-side navigates chromium to a special reporting public app which reads the serialized object and navigates using the new URL service locator objects - setting full state on location state (not in the URL)
  5. Plugin (e.g., Dashboard) reads state from its URL service locator on initial load

The "special reporting app" will be similar to the shared goto app in that it just reads some state and navigates immediately. The above approach also avoids creating any additional saved objects which is something we'd like to try and avoid.

@flash1293 I'm not sure how well this speaks to the search session use case you had in mind, but the gist of it is that we should be able to use the URL Service to get around URL length limitations.

cc @tsullivan

@exalate-issue-sync exalate-issue-sync bot added loe:small Small Level of Effort and removed loe:needs-research This issue requires some research before it can be worked on or estimated labels Jun 21, 2021
@jloleysens
Copy link
Contributor Author

Returning to this issue after a while, I think we can revise the proposed approach given the short URL service. Per @timroes suggestion from a while ago:

I see one alternative option, that by the original description has been ruled out, that I'd like to understand better why it's fully been ruled out: storing temporary state (like a short URL)...that you cannot get an API endpoint to call for report generation if it's just a temporary state?

The proposed flow would be the following (same step 1 & 2 as before)

  1. Plugin (e.g., Dashboard) requests a new report, passing the serialize(able) state object (in the form of a locator object) through share which passes it to shared Reporting UI component
  2. Reporting public requests a new report, sending serialized state to server for saving in the report saved object (SO)
  3. When we start generating the Report we create a new short URL with the full app state (i.e., a new SO)
  4. Reporting navigates Chromium to the short URL public app which redirects us
  5. Take screenshots
  6. Delete the short URL

The limitation regarding the "Post URL" still applies, but if it is a big issue then we could consider adding a capability to Reporting to generate a report given a stored saved object ID.

@tsullivan WDYT?

@tsullivan
Copy link
Member

tsullivan commented Jul 8, 2021

@jloleysens

When we start generating the Report we create a new short URL with the full app state (i.e., a new SO)

Is it possible to just use an ephemeral URL, instead of a short URL? My understanding is that the locator service can receive the app state and return a "long" URL. A short URL has to be a stored saved object, which requires privileges to create, and has to be cleaned up later.

I'm not completely opposed to having a "special reporting app" that converts the app state into a redirect, but I think the conversion can happen on the server side and we can just open Chromium to the URL instead of having a client-side redirect.

jloleysens referenced this issue Aug 12, 2021
…eport (#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://github.com/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 referenced this issue in kibanamachine/kibana 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://github.com/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]>
jloleysens referenced this issue 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://github.com/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]>
@jloleysens jloleysens changed the title [Discussion] Generating PDF/PNG reports should not require a saved search [Discussion] Generating PDF/PNG reports should not require a saved object in plugin Aug 13, 2021
@exalate-issue-sync exalate-issue-sync bot removed the loe:small Small Level of Effort label Sep 3, 2021
@exalate-issue-sync exalate-issue-sync bot added the loe:medium Medium Level of Effort label Sep 3, 2021
@exalate-issue-sync exalate-issue-sync bot added loe:small Small Level of Effort and removed loe:medium Medium Level of Effort labels Aug 12, 2022
@sophiec20 sophiec20 added Feature:Reporting:Framework Reporting issues pertaining to the overall framework and removed (Deprecated) Team:Reporting Services labels Aug 21, 2024
@botelastic botelastic bot added the needs-team Issues missing a team label label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:Reporting:Framework Reporting issues pertaining to the overall framework impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort needs-team Issues missing a team label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants