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

Support running in parallel when snapshotting a static directory #330

Open
mitar opened this issue May 11, 2021 · 9 comments
Open

Support running in parallel when snapshotting a static directory #330

mitar opened this issue May 11, 2021 · 9 comments
Labels
✨ enhancement New feature or request

Comments

@mitar
Copy link

mitar commented May 11, 2021

So I have a static website with not too crazy amount of pages, but one run with running npx percy snapshot ./public still takes 35 minutes. I was wondering if the CLI tool could add some support for parallel execution over a static directory, where I could specify the number of parallel instances and the ID of this instance, something like:

npx percy snapshot --parallel-nonce <unique> --parallel-index 1 --parallel-total 5 ./public

If those arguments are provided, the CLI tool would index all files in the directory, but then in a deterministic way take only 1/5 of the files to snapshot.

@wwilsman
Copy link
Contributor

Having shard options like that would be a great addition! I'll see if we can slot it in with some upcoming work on that command.

@wwilsman
Copy link
Contributor

Regarding the 35 minutes though, that does seem like a while for this command. Looking at your logs, I see that the time includes downloading Chromium which happens the first time asset discovery is launched. You can speed up your CI a bit by having Percy download the browser on install and then caching dependencies. Or if you already have a Chromium browser available in your environment, you can skip Percy's browser download entirely.

I also remember from your other issue with the errors that you have a 1000ms network idle timeout. This will add 1 second to each page's loading time while it waits for the network to idle before taking the snapshot. Over all 1000+ snapshots I can see how that could contribute a significant amount of time. We generally advise keeping that timeout as low as possible and if you exceed 500ms, we would recommend addressing the page slowness rather than continuing to wait longer and longer.

@mitar
Copy link
Author

mitar commented May 18, 2021

You can speed up your CI a bit by having Percy download the browser on install and then caching dependencies. Or if you already have a Chromium browser available in your environment, you can skip Percy's browser download entirely.

Yea, ideally Percy would just offer a Docker image with this included: #328

It seems I will have to roll out one myself, but I didn't yet have time. In any case, from what I observed (looking at logs live), downloading Chromium is pretty fast in fact.

We generally advise keeping that timeout as low as possible and if you exceed 500ms, we would recommend addressing the page slowness rather than continuing to wait longer and longer.

I increased that as a workaround for iframe issues. Also, I reported to user support also many 422 errors which is also why I tried to increase that timeout: https://gitlab.com/mitar/mitarpage/-/jobs/1260988066

@wwilsman
Copy link
Contributor

ideally Percy would just offer a Docker image with this included

There already exists plenty of docker images with browsers included and since Percy is typically a project dependency, there is little value in maintaining our own docker image for only setting an environment variable. Environment variables are usually configurable via your CI config, and any caching strategies will be largely dependent on the project.

I increased that as a workaround for iframe issues. Also, I reported to user support also many 422 errors which is also why I tried to increase that timeout

Now that the iframe issues are hopefully fixed, you should be safe to lower or remove those timeouts again. The 422 errors come from our API, so should have nothing to do with asset discovery. If you get 422 errors again, you can try setting the --verbose flag or the env var, PERCY_DEBUG=*. That will print detailed logs and maybe give us clues as to why the API is responding with 422.

@mitar
Copy link
Author

mitar commented May 18, 2021

There already exists plenty of docker images with browsers included and since Percy is typically a project dependency, there is little value in maintaining our own docker image for only setting an environment variable. Environment variables are usually configurable via your CI config, and any caching strategies will be largely dependent on the project.

For static pages, the Docker image would be most useful: it would include node, latest Percy and chrome. That is all. It took me quite some time to figure out this list of packages.

That will print detailed logs and maybe give us clues as to why the API is responding with 422.

Good idea.

@wwilsman
Copy link
Contributor

For your Docker image, I personally like CircleCI's docker images. My go to is typically circleci/node:lts-browsers which comes with a few other browsers too (useful for cross browser testing). I also found this open source image on DockerHub which is just Node and Chromium.

@wwilsman
Copy link
Contributor

Hey @mitar! We just noticed a bug actually with our 422 errors where details aren't included in the response. So the debug logs likely wouldn't help. The likely cause is that the build has already failed while new snapshots are continuing to attempt to upload. I've opened #344 as well so we can stop handling snapshots when a build fails.

@mitar
Copy link
Author

mitar commented May 20, 2021

I made a docker image (registry.gitlab.com/mitar/mitarpage/percy/hash:27c5359f564bf64e6bed2973e348bc99c9e8176cb0f3fedb9ab600d9ea5dccdc).

@mitar
Copy link
Author

mitar commented Jun 4, 2021

I use Chromium from a Docker image but the whole run still takes 23 minutes now to make snapshots of all 1000+ pages. Running that in parallel would save time.

@Robdel12 Robdel12 added the ✨ enhancement New feature or request label Jun 24, 2021
samarsault pushed a commit that referenced this issue Mar 3, 2023
Bumps [eslint](https://github.com/eslint/eslint) from 7.25.0 to 7.26.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md)
- [Commits](eslint/eslint@v7.25.0...v7.26.0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants