Skip to content

[Reporting] Handle error gracefully when browser driver unexpectedly exits or crashes. #71989

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
Merged
merged 16 commits into from
Aug 3, 2020
Merged

[Reporting] Handle error gracefully when browser driver unexpectedly exits or crashes. #71989

merged 16 commits into from
Aug 3, 2020

Conversation

@tsullivan
Copy link
Member

@tsullivan tsullivan commented Jul 16, 2020 •
edited
Loading

Summary

Closes #71481

I also ran into the scenario where Chromium ran out of memory on the Kibana page, and an unhandled promise rejection from that caused the Kibana server to crash.

Release note: fixed a bug where the Kibana server could crash if the Reporting server-side headless browser crashes.

Checklist

Delete any items that are not applicable to this PR.

Screenshots

image

image

tsullivan
tsullivan commented Jul 16, 2020
View reviewed changes
x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts Outdated
Copy link
Member Author

@tsullivan tsullivan Jul 16, 2020

Choose a reason for hiding this comment

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

Stringifying error objects in a message has shown to cause problems where the error context or error data gets lost. I searched the code to see if this is happening in other places and fixed them in this PR.

tsullivan
tsullivan commented Jul 16, 2020
View reviewed changes
joelgriffith
joelgriffith suggested changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

I think the Rx.Subject calls might use fromEvent to make it a little cleaner.

@tsullivan tsullivan force-pushed the reporting/no-crash branch from f6b3187 to 7bc1733 Compare July 16, 2020 18:01
@tsullivan
Copy link
Member Author

tsullivan commented Jul 16, 2020

As they are these changes fix the problem of the server crashing on the error event of the Page EventEmitter. It's still not enough to fix #71481. Still WIP

@tsullivan tsullivan force-pushed the reporting/no-crash branch 2 times, most recently from e6126b5 to d491dab Compare July 16, 2020 21:34
@tsullivan tsullivan changed the title [Reporting] Handle page error event directly with Puppeteer [Reporting] Handle error gracefully when browser driver unexpectedly exits or crashes. Jul 16, 2020
@tsullivan tsullivan marked this pull request as ready for review July 16, 2020 21:44
@tsullivan tsullivan marked this pull request as draft July 16, 2020 21:56
@tsullivan tsullivan force-pushed the reporting/no-crash branch 2 times, most recently from ebcc598 to 36ae0cf Compare July 17, 2020 00:36
tsullivan
tsullivan commented Jul 17, 2020
View reviewed changes
x-pack/plugins/reporting/server/browsers/chromium/index.ts Outdated
Copy link
Member Author

@tsullivan tsullivan Jul 17, 2020

Choose a reason for hiding this comment

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

Exporting this as a function that returns an Error, so that when we need to throw the error, the stack trace makes sense.

@tsullivan tsullivan force-pushed the reporting/no-crash branch 2 times, most recently from 573eb22 to cdb8b25 Compare July 17, 2020 00:40
@tsullivan tsullivan force-pushed the reporting/no-crash branch from cdb8b25 to ad52460 Compare July 17, 2020 00:42
@tsullivan tsullivan marked this pull request as ready for review July 17, 2020 00:42
@tsullivan
Copy link
Member Author

tsullivan commented Jul 17, 2020

A Reporting functional test is failing, because the archived data used in tests violates the new network policy in the reporting_api_integration config.

@tsullivan
Copy link
Member Author

tsullivan commented Jul 31, 2020

I was out for a few days. Coming back to this...

@tsullivan
Copy link
Member Author

tsullivan commented Aug 3, 2020

@elasticmachine merge upstream

joelgriffith
joelgriffith approved these changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Thanks for adding the network_policy test 👏

@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 3, 2020

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@tsullivan tsullivan merged commit e00d7d2 into elastic:master Aug 3, 2020
@tsullivan tsullivan deleted the reporting/no-crash branch August 3, 2020 23:03
@tsullivan tsullivan mentioned this pull request Aug 3, 2020
Merged
tsullivan added a commit to tsullivan/kibana that referenced this pull request Aug 3, 2020
@tsullivan @elasticmachine
[Reporting] Handle error gracefully when browser driver unexpectedly …
0045afe 
…exits or crashes. (elastic#71989)

* [Reporting] Handle page error event directly with Puppeteer

* clean up logger.error that was stringifying error

* use fromEvent

* better handling and error messaging if browser was closed in the middle of the screenshot pipeline

* fix pdf functional api tests

* fix i18n error

* fix jest

* fix ts

* fix i18n

* tweaks

* ok to throw error in callback

* fix ts

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
tsullivan added a commit that referenced this pull request Aug 4, 2020
@tsullivan @elasticmachine
[Reporting] Handle error gracefully when browser driver unexpectedly …
1ae9eb8 
…exits or crashes. (#71989) (#74190)

* [Reporting] Handle page error event directly with Puppeteer

* clean up logger.error that was stringifying error

* use fromEvent

* better handling and error messaging if browser was closed in the middle of the screenshot pipeline

* fix pdf functional api tests

* fix i18n error

* fix jest

* fix ts

* fix i18n

* tweaks

* ok to throw error in callback

* fix ts

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@joelgriffith joelgriffith joelgriffith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release_note:fix v7.9.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reporting network policy violation should not cause unhandled promise rejection

4 participants

@tsullivan @kibanamachine @joelgriffith @elasticmachine