clickhouse support for log_store - #4748
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (25)
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ClickHouse as a supported log store with config/schema wiring, connection and migration setup, ClickHouse-backed writes, query-layer support, tests, and dependency updates. ChangesClickHouse LogStore Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NewLogStore
participant newClickHouseLogStore
participant buildClickHouseDSN
participant GORM
participant triggerClickHouseMigrations
NewLogStore->>newClickHouseLogStore: ClickHouseConfig + retentionDays
newClickHouseLogStore->>buildClickHouseDSN: ClickHouseConfig
buildClickHouseDSN-->>newClickHouseLogStore: DSN
newClickHouseLogStore->>GORM: open connection
newClickHouseLogStore->>GORM: SELECT 1
newClickHouseLogStore->>triggerClickHouseMigrations: cluster + retentionDays
triggerClickHouseMigrations-->>newClickHouseLogStore: migration result
newClickHouseLogStore-->>NewLogStore: ClickHouseLogStore
sequenceDiagram
participant Caller
participant RDBLogStore
participant unixBucketExpr
participant ClickHouse
Caller->>RDBLogStore: Get*Histogram request
RDBLogStore->>unixBucketExpr: dialect + bucketSizeSeconds
unixBucketExpr-->>RDBLogStore: bucket SQL
RDBLogStore->>ClickHouse: aggregate query
ClickHouse-->>RDBLogStore: bucketed rows and quantiles
RDBLogStore-->>Caller: histogram result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
akshaydeo
marked this pull request as ready for review
June 28, 2026 08:55
Contributor
Confidence Score: 4/5This is close, but the config validation issue should be fixed before merging.
transports/config.schema.json Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/clickhouse.go`:
- Around line 121-129: The ClickHouse store initialization path returns on ping
or migration failures without closing the underlying pool, so update the
constructor that runs db.WithContext(ctx).Exec("SELECT 1") and clickhouseMigrate
to defer closing the sql.DB until the store is fully created. Keep the close
deferred while validating the connection and running migrations, then cancel or
skip the close only after successful construction and transfer of ownership to
the returned store. Use the existing ClickHouse init flow and logger/error
return paths to ensure every startup failure releases the pool before returning.
In `@framework/logstore/clickhousemigrate.go`:
- Around line 86-100: The `ver` column in `clickhousemigrate.go` should use a
higher-resolution timestamp than `now64()` so rapid reinserts don’t share the
same version and defeat `ReplacingMergeTree(ver)` ordering. Update the table DDL
construction in the migration code that appends the `ver` column and builds the
`CREATE TABLE` statement to use a more precise default for `ver`, keeping the
existing `ReplacingMergeTree` and `ReplicatedReplacingMergeTree` setup intact.
In `@framework/logstore/clickhousestore.go`:
- Around line 41-71: The update helpers currently allow modifying ClickHouse
dedup key columns, which can turn an update into a new logical row. Update
chApplyUpdateMap and chApplyStructUpdate to skip or reject writes to the
immutable key fields used by logs/mcp_tool_logs, specifically the ORDER BY
columns timestamp and id, and ensure any attempt to set those fields returns an
error instead of applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ecee9426-00e3-4417-8111-4438c2e63331
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumtests/cmd/e2eseed/go.sumis excluded by!**/*.sumtests/cmd/seed/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
framework/go.modframework/logstore/clickhouse.goframework/logstore/clickhousemigrate.goframework/logstore/clickhousestore.goframework/logstore/config.goframework/logstore/dialectsql.goframework/logstore/rdb.goframework/logstore/store.gotests/cmd/e2eseed/go.modtests/cmd/seed/go.mod
akshaydeo
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
from
June 28, 2026 18:37
ce56f61 to
b0c3092
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/clickhouse.go`:
- Around line 33-34: The ClickHouse config currently serializes DialTimeout as a
Go duration string, but logstore config duration fields must remain numeric
milliseconds. Update the ClickHouse config struct and any related
serialization/deserialization paths so dial_timeout is stored as milliseconds in
JSON, and only convert it to the ClickHouse driver’s duration string when
assembling the DSN in the ClickHouse connection builder. Use the existing
ClickHouse config symbols and DSN-building logic to keep the duration handling
consistent with the rest of framework/logstore.
- Around line 35-37: The Cluster field in clickhouse.go is interpolated into
ClickHouse DDL via the ON CLUSTER clause, so it must be validated or escaped
before use. Update the code path that builds the DDL to either reject invalid
cluster names up front or escape embedded backticks in config.Cluster before
passing it into the fmt.Sprintf/ON CLUSTER logic. Make sure the fix is applied
in the Cluster-related DDL construction around the ClickHouse logstore config
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 77faf937-57ce-4e13-b0f7-00d169555ae1
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumtests/cmd/e2eseed/go.sumis excluded by!**/*.sumtests/cmd/seed/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
framework/go.modframework/logstore/clickhouse.goframework/logstore/clickhousemigrate.goframework/logstore/clickhousestore.goframework/logstore/config.goframework/logstore/dialectsql.goframework/logstore/rdb.goframework/logstore/store.gotests/cmd/e2eseed/go.modtests/cmd/seed/go.mod
🚧 Files skipped from review as they are similar to previous changes (8)
- framework/logstore/config.go
- framework/logstore/dialectsql.go
- framework/logstore/store.go
- tests/cmd/e2eseed/go.mod
- framework/logstore/clickhousemigrate.go
- framework/go.mod
- tests/cmd/seed/go.mod
- framework/logstore/clickhousestore.go
akshaydeo
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
from
July 3, 2026 10:48
b0c3092 to
5d01e28
Compare
akshaydeo
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
3 times, most recently
from
July 3, 2026 12:00
ab5653d to
8e6c4b1
Compare
akshaydeo
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
2 times, most recently
from
July 3, 2026 12:45
82dbb2c to
0fbae96
Compare
8 tasks
danpiths
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
from
July 5, 2026 14:13
0fbae96 to
50f6d12
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
framework/logstore/clickhousestore_test.go (1)
568-599: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHistogram tests only check non-nil, not actual bucket values.
All four assertions (
GetHistogram,GetCostHistogram,GetTokenHistogram,GetModelRankings) only verify the result isn't nil. Since 4 entries are seeded with identical, known cost/token values, these tests would still pass even if the ClickHouse-specific bucket/quantile SQL (dialectsql.go) miscounted or mis-bucketed data.♻️ Example of stronger assertions
hist, err := store.GetHistogram(ctx, SearchFilters{}, 60) require.NoError(t, err) require.NotNil(t, hist) + // e.g. assert total count across buckets equals 4, or that a specific bucket has count 4. costHist, err := store.GetCostHistogram(ctx, SearchFilters{}, 60) require.NoError(t, err) require.NotNil(t, costHist) + // e.g. assert aggregated cost across buckets equals 4 * 0.25.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/clickhousestore_test.go` around lines 568 - 599, The histogram test in TestClickHouseHistograms only checks that results from GetHistogram, GetCostHistogram, GetTokenHistogram, and GetModelRankings are non-nil, so it would miss broken bucket/quantile aggregation. Strengthen the assertions by verifying the actual returned bucket counts and values against the four seeded log entries with known Status, Cost, TotalTokens, PromptTokens, and CompletionTokens, using the same SearchFilters and helper methods to confirm the ClickHouse SQL in dialectsql.go is producing the expected histogram and ranking data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/configs/withclickhouselogstorehttp/config.json`:
- Around line 27-29: The example provider key value in the config entry is too
похожe to a real OpenAI secret and should be replaced with an obviously dummy
placeholder or environment-based reference. Update the value used in the example
config item identified by the name field "openai-key-1" so it is clearly
non-secret and not copyable as a real credential, while keeping the example
structure intact.
In `@framework/logstore/clickhousemigrate.go`:
- Around line 175-184: `clickhouseReconcileColumns` is adding missing columns
without honoring the same override logic used by `clickhouseCreateTable`, so
reconciled tables can lose special defaults like `inc_number`’s Snowflake
generator. Update the column-add path in `clickhouseReconcileColumns` to apply
`chColumnOverrides` before generating the ClickHouse type, matching the behavior
already used during table creation. Keep the existing migration flow and ensure
the override is applied whenever a missing column is added.
---
Nitpick comments:
In `@framework/logstore/clickhousestore_test.go`:
- Around line 568-599: The histogram test in TestClickHouseHistograms only
checks that results from GetHistogram, GetCostHistogram, GetTokenHistogram, and
GetModelRankings are non-nil, so it would miss broken bucket/quantile
aggregation. Strengthen the assertions by verifying the actual returned bucket
counts and values against the four seeded log entries with known Status, Cost,
TotalTokens, PromptTokens, and CompletionTokens, using the same SearchFilters
and helper methods to confirm the ClickHouse SQL in dialectsql.go is producing
the expected histogram and ranking data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7f21bcf-bc24-4071-b400-08542f9d5ce1
⛔ Files ignored due to path filters (3)
framework/go.sumis excluded by!**/*.sumtests/cmd/e2eseed/go.sumis excluded by!**/*.sumtests/cmd/seed/go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
.gitignoreMakefilecore/schemas/async.gocore/schemas/bifrost.goexamples/configs/withclickhouselogstore/config.jsonexamples/configs/withclickhouselogstorehttp/config.jsonexamples/plugins/hello-world/.gitignoreframework/docker-compose.ymlframework/go.modframework/logstore/clickhouse.goframework/logstore/clickhousemigrate.goframework/logstore/clickhousestore.goframework/logstore/clickhousestore_test.goframework/logstore/config.goframework/logstore/dialectsql.goframework/logstore/rdb.goframework/logstore/store.goscripts/bifrost-migration-cli/.gitignoretests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/e2e/clis/.gitignoretests/e2e/clis/reports/.keeptests/semanticcache/.gitignoretransports/config.schema.jsonui/.gitignore
💤 Files with no reviewable changes (6)
- tests/semanticcache/.gitignore
- ui/.gitignore
- examples/plugins/hello-world/.gitignore
- scripts/bifrost-migration-cli/.gitignore
- tests/e2e/clis/.gitignore
- transports/config.schema.json
✅ Files skipped from review due to trivial changes (2)
- examples/configs/withclickhouselogstore/config.json
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (10)
- framework/logstore/dialectsql.go
- core/schemas/bifrost.go
- framework/logstore/config.go
- framework/logstore/store.go
- core/schemas/async.go
- tests/cmd/seed/go.mod
- Makefile
- framework/go.mod
- tests/cmd/e2eseed/go.mod
- framework/logstore/rdb.go
akshaydeo
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
from
July 5, 2026 21:54
3dbfd57 to
4e85377
Compare
akshaydeo
force-pushed
the
06-28-clickhouse_support_for_log_store
branch
from
July 5, 2026 22:24
4e85377 to
599ad8a
Compare
Contributor
Author
Merge activity
|
This was referenced Jul 14, 2026
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.
Summary
Adds ClickHouse as a supported log store backend, enabling high-throughput, append-only OLAP storage for Bifrost request logs, MCP tool logs, and async jobs. This provides a scalable alternative to SQLite and Postgres for analytics-heavy deployments.
Changes
ClickHouseLogStore: Implements theLogStoreinterface usingReplacingMergeTreetables with a connection-levelfinal = 1setting so reads transparently see the latest version of each row. Inserts are plain (noON CONFLICT); idempotency is handled by RMT dedup.UPDATE, mutations re-read the existing row, apply the patch, and re-insert with a higherver(defaulted tonow64(9)). Per-row shard locks prevent concurrent updaters on the same pod from silently dropping each other's patches.clickhousemigrate.goderives column definitions from GORM-parsed struct schemas and runs idempotentCREATE TABLE IF NOT EXISTS+ALTER TABLE ... ADD COLUMN IF NOT EXISTSmigrations. No migration ledger is needed since both DDL statements are inherently concurrency-safe. Cluster-mode DDL (ON CLUSTER) andReplicatedReplacingMergeTreeare supported via the optionalclusterconfig field.unixBucketExprhelper indialectsql.gothat returns the correct unix-bucket expression per dialect (SQLite, MySQL, Postgres, ClickHouse), eliminating repeated per-dialect switch blocks across all histogram queries. ClickHouse-specific JSON functions (isValidJSON,JSONExtractString) andquantile()aggregates replace Postgres/SQLite equivalents in filter, stats, and latency histogram paths.buildClickHouseDSNsupports native (port 9000/9440) and HTTP (port 8123/8443) protocols, TLS, credentials, custom dial timeout, and passesfinal=1andmutations_sync=1as connection-level settings.driver.Valueron custom string types:AsyncJobStatusandRequestTypenow implementdriver.Valuerso the clickhouse-go batch insert path can serialize them correctly.withclickhouselogstoreandwithclickhouselogstorehttpexample configs for native and HTTP protocol setups.clickhouseservice (native on host port 9001, HTTP on 8123) toframework/docker-compose.ymlfor local development and integration tests.config.schema.jsonupdated to includeclickhouseas a validlogs_store.typewith full property documentation.$(CURDIR)fix JUnit report paths whencdchanges the working directory..gitignoreconsolidation: Merged several scattered.gitignorefiles (UI, plugin, CLI, semantic cache) into the root.gitignore.clickhousestore_test.gocovers create, idempotent insert, batch insert, map/struct updates, dedup key protection, concurrent RMW correctness, bulk cost backfill, search/stats, delete, TTL-based batch delete, MCP tool logs, async jobs, and all histogram types. Tests skip automatically when ClickHouse is unavailable.Type of change
Affected areas
How to test
Start the framework Docker Compose stack (includes the new ClickHouse service):
cd framework docker compose up -d clickhouseRun the ClickHouse integration tests:
Run the full framework test suite:
To test end-to-end with the example config:
New
logs_storeconfig fields for ClickHouse:hostportdatabasedefaultusernamepasswordprotocolnative|httpnativesecurefalsedial_timeout10000clusterBreaking changes
Related issues
Security considerations
ClickHouse credentials are handled via
schemas.SecretVar(consistent with existing Postgres/SQLite config patterns). Theclusterfield is identifier-escaped before interpolation into DDL to prevent injection via config. No new secrets are introduced beyond what operators supply in their config.Checklist
docs/contributing/README.mdand followed the guidelines