-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Revisit handling timeouts for different phases of screenshot capture #113807
[Reporting] Revisit handling timeouts for different phases of screenshot capture #113807
Conversation
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
d22df3d
to
40a20cb
Compare
@elasticmachine merge upstream |
6452e75
to
3d6f9d9
Compare
1c11553
to
8ed81a8
Compare
8ed81a8
to
8900e24
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments @tsullivan , changes LGTM.
…dler.ts Co-authored-by: Michael Dokolin <[email protected]>
…dler.ts Co-authored-by: Michael Dokolin <[email protected]>
…dler.ts Co-authored-by: Michael Dokolin <[email protected]>
…dler.ts Co-authored-by: Michael Dokolin <[email protected]>
…dler.ts Co-authored-by: Michael Dokolin <[email protected]>
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/jobs/categorization_field_examples·ts.apis Machine Learning jobs Categorization example endpoint - invalid, too many tokens.Standard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
…hot capture (elastic#113807) * [Reporting] Revisit handling timeouts for different phases of screenshot capture * remove translations for changed text * add wip unit test * simplify class * todo more testing * fix ts * update snapshots * simplify open_url * fixup me * move setupPage to a method of the ObservableHandler class * do not pass entire config object to helper functions * distinguish internal timeouts vs external timeout * add tests for waitUntil * checkIsPageOpen test * restore passing of renderErrors * updates per feedback * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * fix parsing * apply simplifications consistently * dont main waitUntil a higher order component * resolve the timeouts options outside of the service * comment correction Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Dokolin <[email protected]>
…hot capture (#113807) (#115418) * [Reporting] Revisit handling timeouts for different phases of screenshot capture * remove translations for changed text * add wip unit test * simplify class * todo more testing * fix ts * update snapshots * simplify open_url * fixup me * move setupPage to a method of the ObservableHandler class * do not pass entire config object to helper functions * distinguish internal timeouts vs external timeout * add tests for waitUntil * checkIsPageOpen test * restore passing of renderErrors * updates per feedback * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * Update x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts Co-authored-by: Michael Dokolin <[email protected]> * fix parsing * apply simplifications consistently * dont main waitUntil a higher order component * resolve the timeouts options outside of the service * comment correction Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Dokolin <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Dokolin <[email protected]>
Summary
Closes #110718
This PR revises the timeout controls around the 3 different phases of screenshot capturing, and fixes an issue in the original implementation of #58683, which is that waiting for render can take up the entire reporting queue timeout time.
Other changes
getScreenshots$
functionYou may need to increase '{configKey}'
. This now checks if the error is an actual Rx.TimeoutError object.Checklist
Delete any items that are not applicable to this PR.