chore: improve ClickHouse read connection pool settings / behavior - #3871
Merged
amokan merged 9 commits intoAug 26, 2026
Merged
Conversation
…CH config UX (wasnt used for that anyhow); adds `read_pool_size` config option to CH backends with a default of 50; adds optional `labeled_read_pool_size` for non-default labeled read clusters
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Ziinc
approved these changes
Aug 26, 2026
Comment on lines
+33
to
+34
| @read_pool_size 50 | ||
| @labeled_read_pool_size 32 |
Contributor
There was a problem hiding this comment.
Suggested change
| @read_pool_size 50 | |
| @labeled_read_pool_size 32 | |
| @default_read_pool_size 50 | |
| @default_labeled_read_pool_size 32 |
| "ClickHouse slow connection checkout: waited #{pool_ms}ms for a pool connection", | ||
| backend_id: backend_id | ||
| backend_id: backend_id, | ||
| read_cluster: label |
Contributor
There was a problem hiding this comment.
would be good to nest this under clickhouse.read_cluster
| "ClickHouse slow connection checkout: waited #{pool_ms}ms for a pool connection", | ||
| backend_id: backend_id | ||
| backend_id: backend_id, | ||
| read_cluster: label |
Contributor
There was a problem hiding this comment.
Suggested change
| read_cluster: label | |
| clickhouse: %{read_cluster: label} |
Contributor
Author
There was a problem hiding this comment.
So this gets a little funky on tests (and consistency) due to the dev/test env using the standard Logger which effectively drops the key because it returns nil for any value that does not have a String.Chars impl (like a map).
So what I did as a compromise is prefix these with clickhouse_ - so rather than read_cluster: label -> clickhouse_read_cluster: label.
| @@ -15,7 +15,7 @@ defmodule LogflareWeb.Live.DisplayHelpers do | |||
| """ | |||
| def sanitize_backend_config(config) when is_map(config) do | |||
Contributor
There was a problem hiding this comment.
for future, probably want to let the Adaptor handle this declaration based on their config keys
Addresses PR review feedback on `connection_manager.ex`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses PR review feedback. Telemetry metadata stays flat since `lib/telemetry.ex` tags on top-level `:read_cluster`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gger calls in the same adaptor consistent with the same prefix
…onnection-pool-behavior
Ziinc
approved these changes
Aug 26, 2026
amokan
deleted the
adammokan/o11y-2410-review-clickhouse-read-connection-pool-behavior
branch
August 26, 2026 20:13
amokan
added a commit
that referenced
this pull request
Aug 26, 2026
…nd-config-options-to-support-a Resolves conflicts with #3871 (ClickHouse read connection pool changes): - validate_config: keep `validate_query_user_pass/1`, adopt main's `read_pool_size`/`labeled_read_pool_size` validations in place of the removed `pool_size`. - check_read_grant: adopt main's `clickhouse_`-prefixed logger metadata and rename our `query_user` key to `clickhouse_query_user` to match. - open_api_schemas: keep `query_user`/`query_password`, take main's read pool fields. - connection_manager_test / backends_live_test: both sides added independent tests at the same location; kept all of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It became obvious when configuring labeled read clusters that the connection pool situation was not flexible enough to support us in production.
This PR makes some sane adjustments to that area:
@ch_idle_intervalfrom 5s -> 3s to reduce churnpool_sizeconfig removed from ClickHouse backends as it was only used for queries and was not labeled appropriately in the UXread_pool_sizeconfig (default of 50) option. This is the single source of truth for read pool sizing for every scenario other than labeled read poolslabeled_read_pool_sizeconfig (default 32) for labeled read clusters. The exception is that the default labeled read cluster will always use theread_pool_sizevalue.:pool_exhaustederror handlingread_clusterwith the relevant label for a value[:logflare, :clickhouse, :read_pool, :checkout]telemetry eventpool_size: 3fromconfig.exsAlso adds a version bump to 1.50.7