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 new concurrency mode: contextPerRenderKey #314

Merged
merged 26 commits into from
Feb 17, 2022

Conversation

ArturWierzbicki
Copy link
Contributor

@ArturWierzbicki ArturWierzbicki commented Jan 10, 2022

PR adds support for a new concurrency mode, contextPerRenderKey which improves the performance of the dashboard crawl process without negatively affecting the existing use cases.

With contextPerRenderKey mode enabled, image-renderer will reuse the same browser context (created via CDP - https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createBrowserContext) for all requests with the same domain and renderKey within a certain timeframe.

The dashboard crawler reuses the same renderKey for all renders within a single crawl thanks to the concept of RenderingSession. All other use cases create a unique renderKey for each request, causing contextPerRenderKey mode to behave exactly the same as the existing context mode

The main benefits are:

  • lower setup overhead for each render: new browserContext vs new page in existing browserContext
    • impact: ~~negligible
  • lower CPU/mem overhead: browserContext is a separate browser window
    • impact: ~~negligible
  • shared JS cache
    • impact: big! but depends on the network conditions

Shared JS cache is by far the biggest deal. Tests done in local env with 94 dashboards and 6 crawler threads - results are the average of 3 runs (low variance)

network context contextPerRenderKey
not throttled 60s 60s
throttled: max 4mbps, 100ms 95s 70s
throttled: max 1mbps, 100ms 190s 78s

to test:

  1. check out latest main grafana branch
  2. enable dashboardPreviews feature flag
  3. configure grafana to use http version of image-renderer via custom.ini config
[rendering]
server_url = http://localhost:8081/render
  1. go to server admin -> stats and license and click start
    • image

@ArturWierzbicki
Copy link
Contributor Author

renderer.mp4

Base automatically changed from scaled-thumb-result to master January 26, 2022 19:02
@ArturWierzbicki ArturWierzbicki changed the title WIP: Browser per domain concurrency Support new concurrency mode: contextPerRenderKey Feb 2, 2022
@ArturWierzbicki
Copy link
Contributor Author

puppeteer-cluster is in maintenance mode (thomasdondorf/puppeteer-cluster#428) and adding contextPerRenderKey required a lot of changes in the core of the library so I forked it. I have tested all of it extensively, but there is still a chance that I unintentionally broke something and that's a big risk.
Lets talk about how to safely deploy these changes if the overall approach looks good @AgnesToulet

@jongyllen
Copy link
Contributor

puppeteer-cluster is in maintenance mode (thomasdondorf/puppeteer-cluster#428) and adding contextPerRenderKey required a lot of changes in the core of the library so I forked it. I have tested all of it extensively, but there is still a chance that I unintentionally broke something and that's a big risk. Lets talk about how to safely deploy these changes if the overall approach looks good @AgnesToulet

Would you be prepared to help maintaining that fork?

@ArturWierzbicki
Copy link
Contributor Author

puppeteer-cluster is in maintenance mode (thomasdondorf/puppeteer-cluster#428) and adding contextPerRenderKey required a lot of changes in the core of the library so I forked it. I have tested all of it extensively, but there is still a chance that I unintentionally broke something and that's a big risk. Lets talk about how to safely deploy these changes if the overall approach looks good @AgnesToulet

Would you be prepared to help maintaining that fork?

@jongyllen yeah of course!

@ArturWierzbicki
Copy link
Contributor Author

@AgnesToulet as discussed, I have rollbacked mode changes in json configs and I made it so that poolpeteer is used only with contextPerRenderKey mode

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.

Looks pretty good! Left a few comments and concerns.

package.json Outdated Show resolved Hide resolved
maxConcurrency: this.clusteringConfig.maxConcurrency,
timeout: this.clusteringConfig.timeout * 1000,
workerCreationDelay: 1000,
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 this should be used only if needed as it could decrease performance in some cases.

src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@AgnesToulet
Copy link
Contributor

Also, as discussed, I think it would be nice to update the documentation that is managed here: https://github.com/grafana/grafana/blob/main/docs/sources/image-rendering/_index.md Feel free to reach out to me or @achatterjee-grafana if you have any doc-related question.

ArturWierzbicki and others added 5 commits February 15, 2022 20:53
Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>
Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>
Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>
@ArturWierzbicki
Copy link
Contributor Author

@AgnesToulet discussed the docs situation with @achatterjee-grafana and settled on adding draft docs right now and merging them into the main file once we release the previews. PR: https://github.com/grafana/grafana/pull/45490/files

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.

LGTM! Let's merge this 🎉

@ArturWierzbicki ArturWierzbicki merged commit 1574679 into master Feb 17, 2022
@ArturWierzbicki ArturWierzbicki deleted the browser-per-domain-concurrency branch February 17, 2022 14:18
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.

4 participants