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

Make Finch pools lighter for self-hosting #2250

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Conversation

ukutaht
Copy link
Contributor

@ukutaht ukutaht commented Sep 20, 2022

Changes

Previous discussion: #2224

The current pool config eats a lot of memory for self-hosted installations. The pool count seems excessive so this is an attempt to make the app more lean by default.

If it turns out we need beefier pool counts for Plausible.Cloud now or in the future, we can add a config option but keep the lighter pools in the default configuration for easy self-hosting.

Tests

  • This PR does not require tests

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@bundlemon
Copy link

bundlemon bot commented Sep 20, 2022

BundleMon

Unchanged files (7)
Status Path Size Limits
static/css/app.css
515.19KB -
static/js/dashboard.js
295.63KB -
static/js/app.js
12.13KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.06KB -
tracker/js/plausible.js
748B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

defp maybe_add_sentry_pool(pool_config) do
case Application.get_env(:sentry, :dsn) do
dsn when is_binary(dsn) ->
Map.put(pool_config, dsn, size: 50)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only pool where we don't currently specify :http2

I'm not sure what the benefits of using it elsewhere is, and why we don't use http2 for Sentry. Didn't want to dive into this rabbit hole for now but something I noticed

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's https://forum.sentry.io/t/sentry-requests-using-http-1-1-instead-of-h2/13371

I am looking to learn what is the rationale for using http2 everywhere else? Is there an obvious benefit for our use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am looking to learn what is the rationale for using http2 everywhere else? Is there an obvious benefit for our use cases?

No idea

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we shouldn't use it until we find a good reason then? AFAIU http2 characteristics is the reason for this PR 😬

Copy link
Contributor

@ruslandoga ruslandoga Sep 21, 2022

Choose a reason for hiding this comment

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

@ukutaht @aerosol 👋

Sorry to butt in, but sentry.io is a docs/marketing host. Ingest hosts are different and support h2. I've been using h2 with sentry for some time (~6 months) with no issues.

Try https://sentry.io and then https://o1012425.ingest.sentry.io/ on https://tools.keycdn.com/http2-test


I am looking to learn what is the rationale for using http2 everywhere else

In my experience using h2 results in lower memory usage (single tcp connection vs 50 by default) and higher throughput. But I agree that it's a less tested approach to http clients in elixir and there are issues sometimes, so for stability http/1 is probably better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insights. I think it's best to aim for stability so I'll try removing http2 config and using normal http1 pools with size 50.

conn_opts: [transport_opts: [timeout: 15_000]]
]
}

sentry_dsn = Application.get_env(:sentry, :dsn)
base_config
|> maybe_add_sentry_pool()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all these pools are needed for self-hosting so I'm leaving them out here if the relevant integrations have not been configured

@ukutaht ukutaht marked this pull request as ready for review September 20, 2022 13:48
@ukutaht ukutaht requested a review from a team September 20, 2022 13:48
defp maybe_add_sentry_pool(pool_config) do
case Application.get_env(:sentry, :dsn) do
dsn when is_binary(dsn) ->
Map.put(pool_config, dsn, size: 50)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's https://forum.sentry.io/t/sentry-requests-using-http-1-1-instead-of-h2/13371

I am looking to learn what is the rationale for using http2 everywhere else? Is there an obvious benefit for our use cases?

config/runtime.exs Show resolved Hide resolved
Copy link
Contributor

@vinibrsl vinibrsl left a comment

Choose a reason for hiding this comment

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

Looks good!

@ukutaht ukutaht merged commit 3d54b88 into master Sep 21, 2022
@ukutaht ukutaht deleted the lighter-finch-pools branch September 21, 2022 09:51
@SaphireLattice
Copy link

Bumped my self-hosted version to the 1.5 RC with this, absolutely decimated the memory usage by almost a whole gig. Which is pretty huge when the entire VPS is only 2GB of RAM. Before that, with some minor services running (nginx, weechat), plausible with clickhouse ended up getting total system memory usage to around 1.7GB

@ruslandoga
Copy link
Contributor

ruslandoga commented Oct 31, 2022

@SaphireLattice 👋

Did you enable any new features like city-level geolocation? If so, you might want to disable them to get the memory usage to v1.4 level.

@SaphireLattice
Copy link

Oh, I meant that the 1.5 is using 1GB less memory than previous version! And huh, is that enabled by default?

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.

5 participants