[HDX-4664] Support Replicated database + tables in the ClickStack OTel collector - #2761
[HDX-4664] Support Replicated database + tables in the ClickStack OTel collector#2761wrn14897 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 7f880f4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
| } | ||
|
|
||
| var tableCount uint64 | ||
| if exists { | ||
| tableCount, err = countDatabaseTables(ctx, db, database) |
There was a problem hiding this comment.
Greptile SummaryAdds opt-in support for Replicated ClickHouse databases and replicated table engines.
Confidence Score: 4/5The PR is not yet safe to merge because the database conversion can still drop tables created after its emptiness check. The current implementation reads the table count and later executes Files Needing Attention: packages/otel-collector/cmd/migrate/main.go
|
| Filename | Overview |
|---|---|
| packages/otel-collector/cmd/migrate/main.go | Adds database-engine detection, Replicated database conversion, and schema engine rewriting. |
| packages/otel-collector/cmd/migrate/main_test.go | Adds focused unit coverage for engine validation, conversion decisions, generated DDL, and schema rewriting. |
| smoke-tests/otel-collector/docker-compose.yaml | Adds a Keeper-enabled ClickHouse service and collector configuration for Replicated schema smoke tests. |
| smoke-tests/otel-collector/replicated-schema.bats | Verifies replicated database and table engines plus an ingestion round trip. |
Reviews (2): Last reviewed commit: "feat(otel-collector): support Replicated..." | Re-trigger Greptile
E2E Test Results✅ All tests passed • 275 passed • 1 skipped • 1108s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. The destructive path is guarded ( 🟡 P2 — recommended
🔵 P3 nitpicks (6)
Reviewers (7): correctness, adversarial, testing, maintainability, data-migrations, project-standards, reliability. Coverage caveat: Testing gaps:
|
…seed (HDX-4664) Adds ClickStack OTel collector support for the ClickHouse Replicated (DatabaseReplicated) database engine, so the collector's schema seed and clickhouse-operator v0.0.6 (enableDatabaseSync: true) agree on the default database engine and the ClickStack Helm chart can move to the Replicated engine. - New opt-in env var HYPERDX_OTEL_EXPORTER_CLICKHOUSE_DATABASE_ENGINE=Replicated. Before goose runs, the seed ensures the target database uses the Replicated engine: missing -> created with the operator's Keeper path convention; already Replicated -> no-op; non-Replicated + empty -> DROP DATABASE ... SYNC and recreate as Replicated (mirrors the operator's conversion, resolving the startup race from either side); non-Replicated + has tables -> never dropped, loud warning, seed continues (no data loss). - Auto-detected table-engine rewrite: whenever the target database uses the Replicated engine - whether created by the seed or by the operator - the processed schema is rewritten MergeTree -> ReplicatedMergeTree and SummingMergeTree -> ReplicatedSummingMergeTree so table data replicates across replicas. - Smoke tests: new ch-server-replicated (single-node ClickHouse with embedded Keeper + shard/replica macros) and otel-collector-replicated services, plus replicated-schema.bats asserting the Atomic->Replicated conversion, Replicated engines on all tables, and an ingest/query round-trip. - README documentation for the new env var and behavior.
10da7f2 to
7f880f4
Compare
|
Is the Greptile issue worth addressing? |
Summary
Adds ClickStack OTel collector support for the ClickHouse Replicated (DatabaseReplicated) database engine, so the collector's schema seed and clickhouse-operator v0.0.6 (
enableDatabaseSync: true) agree on thedefaultdatabase engine and the ClickStack Helm chart can move to the Replicated engine (follow-up inClickStack-helm-charts).Why: the seed tool always created the target database with the Atomic engine and plain
MergeTreetables. The operator convertsdefaultto Replicated — whichever side ran first broke the other: pre operator#255 the operator dropped the seeded tables (broke the full-stack integration test in ClickStack-helm-charts#240); post operator#255 the operator permanently refuses conversion when the collector seeds tables first, so the deployment never reaches the intended Replicated engine.What changed (all in the Go seed tool
packages/otel-collector/cmd/migrate/— the clickhouse exporter runs withcreate_schema: false, so the seed is the code path that creates the database and tables; seed SQL files are untouched since they're referenced by public ClickStack docs):HYPERDX_OTEL_EXPORTER_CLICKHOUSE_DATABASE_ENGINE=Replicated. Before goose runs, the seed ensures the target database uses the Replicated engine:ENGINE = Replicated('/clickhouse/databases/<name>', '{shard}', '{replica}')(operator's path convention)DROP DATABASE ... SYNC+ recreate as Replicated (exact mirror of the operator's conversion, resolving the startup race from either side)MergeTree→ReplicatedMergeTreeandSummingMergeTree→ReplicatedSummingMergeTree, so table data replicates across replicas (plain MergeTree in a Replicated database only replicates metadata). Backward-safe:CREATE TABLE IF NOT EXISTSno-ops on existing tables.ch-server-replicated(single-node ClickHouse with embedded Keeper +{shard}/{replica}macros via aconfig.doverlay) andotel-collector-replicatedservices, plusreplicated-schema.batsasserting the Atomic→Replicated conversion (fresh ClickHouse boots with an empty Atomicdefault), Replicated engines on all tables, and an ingest/query round-trip.@hyperdx/otel-collector.Notes / out of scope:
WithNoVersioning(no version table is created).TimeSeriesschema (ENABLE_PROMQL=true) is left untouched (not replication-aware).HYPERDX_OTEL_EXPORTER_CREATE_LEGACY_SCHEMA=true/ JSON mode) skips the seed tool and is unaffected.enableDatabaseSync+ operator end-to-end verification happens inClickStack-helm-chartsas a follow-up that depends on this.How to test locally
cd packages/otel-collector && go test ./cmd/migrate/— unit tests for the engine decision logic, DDL, and schema rewrite (including a pass over the realschema/seed/files).cd smoke-tests/otel-collector && bats replicated-schema.bats(requires docker, bats, clickhouse-client) — boots a Keeper-enabled single-node ClickHouse whosedefaultstarts as empty Atomic, and asserts the seed converts it to Replicated, creates all tables withReplicatedMergeTree/ReplicatedSummingMergeTreeengines, and that log ingestion round-trips.bats *.bats— 33/33 pass locally.docker compose up --build -d ch-server-replicated otel-collector-replicated, thenclickhouse-client --port=39000 --query="SELECT engine FROM system.databases WHERE name='default'"→Replicated, andclickhouse-client --port=39000 --query="SELECT name, engine FROM system.tables WHERE database='default'"→ allReplicated*(+MaterializedViewrollup MVs).References