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.

Sign up for GitHub

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

Jump to bottom

[Reporting] add an "auto" option for the timeout settings #131852

Closed
3 tasks done
tsullivan opened this issue May 9, 2022 · 7 comments
Closed
3 tasks done

[Reporting] add an "auto" option for the timeout settings #131852

tsullivan opened this issue May 9, 2022 · 7 comments
Assignees
@eokoneyo
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead enhancement New value added to drive a business result Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@tsullivan
Copy link
Member

tsullivan commented May 9, 2022 •
edited by eokoneyo
Loading

All types of reports use timeout values, which are based on defaults in the Kibana settings. Often these defaults are not scaled high enough for uses cases where there is search latency in the cluster.

We want to add an "auto" value for the timeout settings which would allow durations lasting as long as possible without being timed out by the reporting queue's task timeout limit.

Once this change is in, users can give settings such as xpack.reporting.csv.scroll.duration: auto and each scroll duration will be based on how much available time there is for the report to execute. At that point users would be able to simply update xpack.reporting.queue.timeout to give more time to all parts of execution.

Tasks
Preview Give feedback
  1. [Reporting ] run task function simplifications of image export types #174410
    backport:skip release_note:skip v8.13.0
  2. [Reporting] Track retryAt and startedAt times on the task instance #174409
    backport:skip release_note:skip v8.13.0
  3. [Reporting] Set CSV scroll duration to time left before task can potentially be retried #174988
    Team:SharedUX
    eokoneyo
@tsullivan tsullivan added the bug Fixes for quality problems that affect the customer experience label May 9, 2022
@botelastic botelastic bot added the needs-team Issues missing a team label label May 9, 2022
@tsullivan tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead documentation Team:AppServicesUx labels May 9, 2022
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the needs-team Issues missing a team label label May 9, 2022
@tsullivan
Copy link
Member Author

Let's try to go with a similar proposal as #162841

  • add an "auto" value for the timeout settings
  • auto = allow a duration which lasts as long as possible without being timed out by the reporting queue's task timeout limit

@tsullivan tsullivan mentioned this issue Jan 9, 2024
Merged
tsullivan added a commit that referenced this issue Jan 10, 2024
@tsullivan @kibanamachine
[Reporting ] run task function simplifications of image export types (#…
5a8b7af 
…174410)

## Summary

This PR cleans up extra layers of abstraction in image export types that
could complicate progress of the proposal of "auto" timeouts. See
#131852

## Changes
Minor code cleanup of the "runTask" functions of export types that
create screenshots, by removing the `generatePdf*` / `generatePng`
helper callback functions and inlining the work those modules were
doing.

The helper modules were an integral part of the screenshotting
pipelines, but in the unit tests they were completely mocked. Now that
we have a proper mock utility of the `screenshotting` plugin start
contract, we no longer need mockable code in a separate layer of the
pipelines.

---------

Co-authored-by: kibanamachine <[email protected]>
@tsullivan tsullivan changed the title [Reporting] capture.timeout setting documentation should avoid confusion [Reporting] add an "auto" option for the timeout settings Jan 10, 2024
@tsullivan tsullivan mentioned this issue Jan 10, 2024
Closed
@tsullivan tsullivan added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) and removed Team:AppServicesUx labels Jan 10, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@tsullivan tsullivan mentioned this issue Jan 10, 2024
Merged
1 task
@tsullivan tsullivan self-assigned this Jan 11, 2024
@tsullivan tsullivan added enhancement New value added to drive a business result and removed bug Fixes for quality problems that affect the customer experience labels Jan 11, 2024
delanni pushed a commit to delanni/kibana that referenced this issue Jan 11, 2024
@tsullivan @kibanamachine @delanni
[Reporting ] run task function simplifications of image export types (e…
959b1a9 
…lastic#174410)

## Summary

This PR cleans up extra layers of abstraction in image export types that
could complicate progress of the proposal of "auto" timeouts. See
elastic#131852

## Changes
Minor code cleanup of the "runTask" functions of export types that
create screenshots, by removing the `generatePdf*` / `generatePng`
helper callback functions and inlining the work those modules were
doing.

The helper modules were an integral part of the screenshotting
pipelines, but in the unit tests they were completely mocked. Now that
we have a proper mock utility of the `screenshotting` plugin start
contract, we no longer need mockable code in a separate layer of the
pipelines.

---------

Co-authored-by: kibanamachine <[email protected]>
@tsullivan tsullivan assigned eokoneyo and unassigned tsullivan Jan 11, 2024
@tsullivan tsullivan mentioned this issue Jan 12, 2024
Merged
@tsullivan
Copy link
Member Author

tsullivan commented Jan 12, 2024 •
edited
Loading

@eokoneyo

Once this change is in, users can give settings such as xpack.reporting.csv.scroll.duration: auto and each scroll duration will be based on how much available time there is for the report to execute.

Relevant: #174511 (comment). Since we are touching and remolding the timeout settings for this task, we may want to decide on a new timeout setting for ES|QL request timeout, which is conceptually different than the scroll duration.

@eokoneyo
Copy link
Contributor

eokoneyo commented Jan 16, 2024 •
edited
Loading

Once this change is in, users can give settings such as xpack.reporting.csv.scroll.duration: auto and each scroll duration will be based on how much available time there is for the report to execute.

Relevant: #174511 (comment). Since we are touching and remolding the timeout settings for this task, we may want to decide on a new timeout setting for ES|QL request timeout, which is conceptually different than the scroll duration.

@tsullivan thanks for the heads up, following the conversation there seems to be no preference for an ES|QL specific timeout duration, I think we can go ahead and apply the same change for both ES|QL and the standard CSV generator, we might then diverge along the line if need be, wdyt?

@tsullivan
Copy link
Member Author

@eokoneyo you phrased it well: we can diverge along the line later if need be. 👍🏻

tsullivan added a commit that referenced this issue Jan 16, 2024
@tsullivan
[Reporting] Track retryAt time on the task instance (#174409)
decec37 
## Summary

Initial part of #131852

This PR moves towards auto-calculating the maximum timeouts calculated
based on the timing context provided by the running task instance.

### Other changes
* Added an optional logger parameter to the `getScreenshots` function.
When a logger is provided, the logs created by the screenshot plugin
will have the contextual tags added by the calling code.
  * Before
<img width="1198" alt="image"
src="https://github.com/elastic/kibana/assets/908371/f68a102e-6af2-4863-aedb-52f1e4a099d8">
  * After
<img width="1200" alt="image"
src="https://github.com/elastic/kibana/assets/908371/2dd4c947-ffa6-4cb3-b8a2-22893f49ddb7">

* Fixed an unreported bug where browser timezone was not utilized in PNG
reports.

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@eokoneyo eokoneyo closed this as completed Feb 6, 2024
@eokoneyo
Copy link
Contributor

eokoneyo commented Feb 6, 2024

Closed as completed

CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
@tsullivan @CoenWarmer
[Reporting] Track retryAt time on the task instance (elastic#174409)
0ef71c4 
## Summary

Initial part of elastic#131852

This PR moves towards auto-calculating the maximum timeouts calculated
based on the timing context provided by the running task instance.

### Other changes
* Added an optional logger parameter to the `getScreenshots` function.
When a logger is provided, the logs created by the screenshot plugin
will have the contextual tags added by the calling code.
  * Before
<img width="1198" alt="image"
src="https://github.com/elastic/kibana/assets/908371/f68a102e-6af2-4863-aedb-52f1e4a099d8">
  * After
<img width="1200" alt="image"
src="https://github.com/elastic/kibana/assets/908371/2dd4c947-ffa6-4cb3-b8a2-22893f49ddb7">

* Fixed an unreported bug where browser timezone was not utilized in PNG
reports.

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eokoneyo eokoneyo

Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead enhancement New value added to drive a business result Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tsullivan @eokoneyo @elasticmachine