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

Puppeteer: upgrade to v21 #433

Merged
merged 20 commits into from Aug 19, 2023
Merged

Puppeteer: upgrade to v21 #433

merged 20 commits into from Aug 19, 2023

Conversation

Clarity-89
Copy link
Contributor

Upgrade Puppeteer to the latest version to take advantage of the new features.

Fixes #432

@Clarity-89 Clarity-89 self-assigned this Jun 13, 2023
@Clarity-89 Clarity-89 changed the title Puppeteer upgrade to v20 Puppeteer; upgrade to v20 Jun 13, 2023
@Clarity-89 Clarity-89 changed the title Puppeteer; upgrade to v20 Puppeteer: upgrade to v20 Jun 13, 2023
Copy link
Member

@spinillos spinillos left a comment

Choose a reason for hiding this comment

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

I tested with some dashboards.

@Clarity-89
Copy link
Contributor Author

I tested with some dashboards.

It works locally, but the build fails as some dependencies need to be updated there as well.

@joanlopez
Copy link
Collaborator

It works locally, but the build fails as some dependencies need to be updated there as well.

Do we know exactly which dependencies need to be updated? 🤔

@Clarity-89
Copy link
Contributor Author

It works locally, but the build fails as some dependencies need to be updated there as well.

Do we know exactly which dependencies need to be updated? 🤔

We're relying on some internals when downloading Chronium, which have been removed in the latest versions. Need to investigate the alternatives.

package.json Outdated Show resolved Hide resolved
scripts/download_chromium.js Outdated Show resolved Hide resolved
scripts/download_chromium.js Outdated Show resolved Hide resolved
scripts/package_target.sh Outdated Show resolved Hide resolved
scripts/package_target.sh Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@AgnesToulet
Copy link
Contributor

I tested this in plugin mode on Windows and there was one issue with chromium path.
The line https://github.com/grafana/grafana-image-renderer/blob/master/src/app.ts#L12 should be updated to:

const chromeFolderPrefix = 'chromium'

Apart from that, it seems to work fine 👍

If we don't use Chrome in this PR, I think we should do it in a follow-up one but also look into the performance to ensure it isn't worse (it shouldn't but we should check). I would leave #432 open to keep track of that.

@Clarity-89
Copy link
Contributor Author

I tested this in plugin mode on Windows and there was one issue with chromium path. The line https://github.com/grafana/grafana-image-renderer/blob/master/src/app.ts#L12 should be updated to:

const chromeFolderPrefix = 'chromium'

Apart from that, it seems to work fine 👍

If we don't use Chrome in this PR, I think we should do it in a follow-up one but also look into the performance to ensure it isn't worse (it shouldn't but we should check). I would leave #432 open to keep track of that.

I somehow forgot that the whole point of this upgrade was to use the new Chrome 🙈 We could change it in this PR, but do you have any opinion on how to run Chrome in docker?

@AgnesToulet
Copy link
Contributor

It seems like installing Chrome on Alpine is not recommended so we should keep Chromium here and switch to chrome-stable in the Debian image. This is the Puppeteer Dockerfile: https://github.com/puppeteer/puppeteer/blob/main/docker/Dockerfile

If the performance is much better with Chrome for Testing than with chromium we should update the release pipeline to also publish the Debian Docker image.

@xlson I think you have a better knowledge around Docker than us, any opinion on the above?

@xlson
Copy link
Contributor

xlson commented Jul 25, 2023

@AgnesToulet I'm not following, what is it you want opinions on?

@AgnesToulet
Copy link
Contributor

@xlson I'd like your opinion on this:

It seems like installing Chrome on Alpine is not recommended so we should keep Chromium here and switch to chrome-stable in the Debian image. This is the Puppeteer Dockerfile: https://github.com/puppeteer/puppeteer/blob/main/docker/Dockerfile

If the performance is much better with Chrome for Testing than with chromium we should update the release pipeline to also publish the Debian Docker image.

The context is that Puppeteer (the library we use to open a browser and take a Grafana screenshot) is switching from Chromium to Chrome for Testing by default. We'd like to upgrade to do the same if this improves the performance and stability of the image renderer but we have trouble updating the Docker images (well mostly the Alpine one that is the one we currently publish to DockerHub).

@Clarity-89
Copy link
Contributor Author

If the performance is much better with Chrome for Testing than with chromium we should update the release pipeline to also publish the Debian Docker image.

There's also a request for Debian images: #442. Don't see why we shouldn't publish those.

@joanlopez
Copy link
Collaborator

If the performance is much better with Chrome for Testing than with chromium we should update the release pipeline to also publish the Debian Docker image.

There's also a request for Debian images: #442. Don't see why we shouldn't publish those.

I agree, it would be nice to publish both.

What I'm wondering now is: should we use a different Chrome flavour on each? If so, how much would that increase the maintainability cost?

cc/ @xlson @AgnesToulet

@xlson
Copy link
Contributor

xlson commented Jul 27, 2023

If the performance is better with Chrome I think it makes sense to switch to debian. I'm sceptical of publishing both alpine and debian though as that will increase our maintenance cost.

@Clarity-89
Copy link
Contributor Author

@xlson @AgnesToulet are there any risks with switching to Debian? If not I think we'd do it.

@Clarity-89
Copy link
Contributor Author

It looks like we already have a Debian Docker image, but we just don't publish it.

@xlson
Copy link
Contributor

xlson commented Aug 16, 2023

@xlson @AgnesToulet are there any risks with switching to Debian? If not I think we'd do it.

I don't really think so. We'd rather use alpine as it's designed for docker, but debian should work fine as well.

@Clarity-89 Clarity-89 changed the title Puppeteer: upgrade to v20 Puppeteer: upgrade to v21 Aug 17, 2023
dev.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

Overall looks good!! 👍🏻

@AgnesToulet
Copy link
Contributor

So I think this remains:

I tested this in plugin mode on Windows and there was one issue with chromium path.
The line https://github.com/grafana/grafana-image-renderer/blob/master/src/app.ts#L12 should be updated to:

const chromeFolderPrefix = 'chromium'

And we should be good to merge!

@Clarity-89
Copy link
Contributor Author

Clarity-89 commented Aug 18, 2023

So I think this remains:

I tested this in plugin mode on Windows and there was one issue with chromium path.
The line https://github.com/grafana/grafana-image-renderer/blob/master/src/app.ts#L12 should be updated to:
const chromeFolderPrefix = 'chromium'

And we should be good to merge!

Ah, that's why it wasn't working for me locally, fixed now!

Copy link
Contributor

@AgnesToulet AgnesToulet left a comment

Choose a reason for hiding this comment

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

I did a few more tests and everything works fine! Great work on this 👍

@Clarity-89 Clarity-89 merged commit 0b51487 into master Aug 19, 2023
4 checks passed
@Clarity-89 Clarity-89 deleted the puppeteer-upgrade branch August 19, 2023 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Bump puppeteer to switch to using "Chrome for Testing"
5 participants