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

Chromium reports via puppeteer #21788

Merged
merged 44 commits into from
Aug 30, 2018

Conversation

chrisdavies
Copy link
Contributor

@chrisdavies chrisdavies commented Aug 8, 2018

This changes our Chromium reports to use an upgraded, custom build of Chromium. It changes the driver to use Puppeteer.

I've tested it repeatedly in CI using this branch: #22383, which strips out all tests but the Chromium ones, so I could iterate CI faster.

Testing

  • Be sure to configure Kibana to use Chromium (xpack.reporting.capture.browser.type: chromium)
  • Run a bunch of reports
  • Play around with dimensions, making really big visualizations, dashboards with lots of visualizations, etc
  • If you have a Windows VM, it'd be nice to get more tests on Windows. I have a GCP VM for this purpose that I can open to the reviewers, if you want.

@chrisdavies chrisdavies added WIP Work in progress :Sharing labels Aug 8, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Moved the main scripting logic into Python, as it's a pre-req for
building Chromium, and it's more suited to system scripting of
this nature. Added scripts to initialize the Chromium build
environments.
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

attempt to get around a runtime error.
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

omg this PR is awesome!

Code looks good, let me pull down and play around with some reports before LGTM.

Build instructions are great, and I followed for linux, and am in the process of building. Not going to go through all of them due to time constraints. :)

@@ -135,6 +136,7 @@
"polished": "^1.9.2",
"prop-types": "^15.6.0",
"puid": "1.0.5",
"puppeteer-core": "^1.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think now we can drop chrome-remote-interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Yes. You're right. I thought I'd done it. This is why we have code-reviews, eh?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisdavies
Copy link
Contributor Author

retest

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

🎉 🏅 🍾 🔥 🎈 SO EXCITED about this pr!!!!!

LGTM!!!!!!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisdavies chrisdavies merged commit 22aa6ca into elastic:master Aug 30, 2018
@chrisdavies chrisdavies deleted the reporting/puppeteer branch August 30, 2018 00:16
jarpy added a commit to elastic/kibana-docker that referenced this pull request Sep 17, 2018
Kibana installs headless browsers at runtime. We can't control the
permissions of these files at build time, so we should not make
assertions about those permissions.

This patch excludes new files that were added by elastic/kibana#21788.

Fixes #99
jarpy added a commit to elastic/kibana-docker that referenced this pull request Sep 17, 2018
Kibana installs headless browsers at runtime. We can't control the
permissions of these files at build time, so we should not make
assertions about those permissions.

This patch excludes new files that were added by elastic/kibana#21788.

Fixes #99
jarpy added a commit to elastic/kibana-docker that referenced this pull request Sep 17, 2018
Kibana installs headless browsers at run time. We can't control the
permissions of these files at build time, so we should not make
assertions about those permissions.

This patch excludes new files that were added by elastic/kibana#21788.

Fixes #99
jarpy added a commit to elastic/kibana-docker that referenced this pull request Sep 17, 2018
Kibana installs headless browsers at run time. We can't control the
permissions of these files at build time, so we should not make
assertions about those permissions.

This patch excludes new files that were added by elastic/kibana#21788.

Fixes #99
jarpy added a commit to elastic/kibana-docker that referenced this pull request Sep 17, 2018
Kibana installs headless browsers at run time. We can't control the
permissions of these files at build time, so we should not make
assertions about those permissions.

This patch excludes new files that were added by elastic/kibana#21788.

Fixes #99

for (let pageNum = 0; pageNum <= expectedPages; ++pageNum) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, I think we need to bring back the retry loop. Just hit the "Failed to convert page to image" error in a PR here: #24109 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants