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] remove forceNow hack #110568

Closed
3 tasks
ppisljar opened this issue Aug 31, 2021 · 7 comments
Closed
3 tasks

[Reporting] remove forceNow hack #110568

ppisljar opened this issue Aug 31, 2021 · 7 comments
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead discuss Feature:Reporting:CSV Reporting issues pertaining to CSV file export impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort needs-team Issues missing a team label technical debt Improvement of the software architecture and operational architecture

Comments

@ppisljar
Copy link
Member

ppisljar commented Aug 31, 2021

Kibana accepts forceNow url parameter (which should be a date) which will change how our search infrastructure works and will not use the actual time in datemath but the value passed in over URL. This was implemented to allow reports generated with the fixed time at when the user requested the report (rather than the time at which report was generated).

This is quite a big tech depth spreading across over 30 files and making a lot of already complicated services even more complicated so we should try to remove it asap.

reporting can instead convert time to absolute when report is being requested if that is desired and doesn't need to do any other special handling.

  • update reporting csv export v2 to no longer depend on forceNow
  • (optional?) update reporting csv export v1 to no longer depend on forceNow
  • remove forceNow from everywhere

Possibly we want to make sure existing reports continue to work. Reporting could take the forceNow and update searchSource in such cases with absolute times.

@ppisljar ppisljar added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppServices technical debt Improvement of the software architecture and operational architecture labels Aug 31, 2021
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Sep 13, 2021
@exalate-issue-sync exalate-issue-sync bot changed the title remove forceNow hack [Reporting] remove forceNow hack Sep 27, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort and removed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Sep 27, 2021
@tsullivan
Copy link
Member

I agree 100% that csv export types should not use forceNow. I don't think that was ever intended.

The forceNow parameter is required for eliminating time drift in PDF and PNG reports when the time filter uses a relative time range. When the dashboard is set to a relative time range, the resulting report should still show the same data the user sees at the time they requested the report to be generated. It is also how Canvas gets its "now" value, since it doesn't use the time filter.

Consider the test case given when forceNow was added: #16236 (comment). BTW this is bug currently evident with the V2 PDF and PNG export types.

The report job does need a way to track the time the user requested the report, or else relative time ranges become useless for dashboard reports created by a live user.

I agree with the premise of the issue, that the tracking of that time does need to be less hacky. It should be possible for the app to find its now value using its own locator state.

@ppisljar
Copy link
Member Author

ppisljar commented Oct 7, 2021

@tsullivan i think we should be able to do the same thing in PDF/PNG reports: if user is interested in specific time (and not the time when report runs) the time should be converted to absolute before the report is being generated (or as part of report generation process, app should provide all the correct configuration and we shouldn't need to do such hacks imo.

I also think using locator params for this seems wrong. application shouldn't need to handle such time overrides, it already handles passing in the time range which is just what we are looking for. But reporting shouldn't be tied to any specific applications or contain any application specific code, so it shouldnt do anything to locatorParams or even assume some options are there. It should just get the locatorParams provided by application and pass them on without any modification/handling.

@tsullivan
Copy link
Member

tsullivan commented Oct 7, 2021

if user is interested in specific time (and not the time when report runs) the time should be converted to absolute before the report is being generated (or as part of report generation process, app should provide all the correct configuration and we shouldn't need to do such hacks imo.

@ppisljar The app doesn't know if the report will be a one-off report, or automated/scheduled report. For a one-off report, the "job parameters" are used one time. For a scheduled report, the job parameters are re-used for each recurrence of the report.

The below message was edited, as I had to take more time to think about the cons of this hack, and that there already is a good solution around it:

It sounds like you are saying the app should convert its relative time filter to absolute in its sharing data it provides to reporting. This will work as long as when the user copies the "POST URL," they get an unconverted time filter value [1]. @jloleysens is doing this with Discover/CSV export here: #114274, and it looks to be working well.

One thing this means is that when scheduled reports is available, Reporting will not be able to support the following flow:

  • user creates a one-off report
  • they go to Management > Reporting, select the report that was just created
  • click a new option: "run this report on a schedule" and define an interval for the schedule.

Instead, the flow will take a few more clicks:

  • user creates a one-off report
  • they go to Management > Reporting, select the report that was just created
  • click a new option: "view the Dashboard of this report"
  • they go to the Dashboard which is set to absolute time because it was converted that way for the one-off report
  • they have to change the time filter to the relative time they want
  • they schedule a report from the Dashboard

[1] When scheduled reports is available, the user needs an unconverted time filter value when they schedule a report.

@ppisljar
Copy link
Member Author

ppisljar commented Oct 11, 2021

The app doesn't know if the report will be a one-off report, or automated/scheduled report. For a one-off report, the "job parameters" are used one time. For a scheduled report, the job parameters are re-used for each recurrence of the report.

That's what probably should be changed.

I think we agree, that forceNow hack is only needed for the case when user does a one-off report (and expects the exactly same time in report as he is seeing in kibana). Its not needed for scheduled reports, as in those cases user actually wants the time to be "moving".

forceNow is actually needed for scheduled reports as well. If someone wants to schedule a report to run weekly on tuesday and report on data from monday 12am - tuesday 12am they can't rely on relative time ranges alone as the report might not run exactly at 12am. This gets worse if somebody wants to run multiple reports over the same time range.

So we already have two separate buttons for the two actions in question: generate PDF report which will trigger a one-off report and copy post URL which will be used to schedule the reports. What if we use absolute time ranges when user clicks generate PDF report and keep relative time ranges for copy POST URL button, and thus remove reliance on the forceNow hack ?

cc @vadimkibana

@ppisljar
Copy link
Member Author

It seems using forceNow is cruical to reporting so it can't be removed.

When user wants to generate a one-off report he wants to make sure the data in the report will use time when the report was generated and not the time when report was executed (which could be much later)

when user wants to schedule a report they need to make sure its using the time they request, not the time when report was generated (as we would loose precision when generating multiple reports in sequence) or worse at the time report is executed (which could be much later).

we can't substitute this for using absolute time ranges as that would require reporting to know how time ranges are consumed by specific applications.

@sophiec20 sophiec20 added the Feature:Reporting:CSV Reporting issues pertaining to CSV file export label 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
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead discuss Feature:Reporting:CSV Reporting issues pertaining to CSV file export impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort needs-team Issues missing a team label technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

4 participants