Skip to content

[HDX-4664] Support Replicated database + tables in the ClickStack OTel collector - #2761

Open
wrn14897 wants to merge 1 commit into
mainfrom
warren/hdx-4664
Open

[HDX-4664] Support Replicated database + tables in the ClickStack OTel collector#2761
wrn14897 wants to merge 1 commit into
mainfrom
warren/hdx-4664

Conversation

@wrn14897

Copy link
Copy Markdown
Member

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 the default database engine and the ClickStack Helm chart can move to the Replicated engine (follow-up in ClickStack-helm-charts).

Why: the seed tool always created the target database with the Atomic engine and plain MergeTree tables. The operator converts default to 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 with create_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):

  • 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 ENGINE = Replicated('/clickhouse/databases/<name>', '{shard}', '{replica}') (operator's path convention)
    • already Replicated → no-op
    • non-Replicated + emptyDROP DATABASE ... SYNC + recreate as Replicated (exact mirror of 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, independent of the env var — the processed schema is rewritten MergeTreeReplicatedMergeTree and SummingMergeTreeReplicatedSummingMergeTree, so table data replicates across replicas (plain MergeTree in a Replicated database only replicates metadata). Backward-safe: CREATE TABLE IF NOT EXISTS no-ops on existing tables.
  • Smoke tests: new ch-server-replicated (single-node ClickHouse with embedded Keeper + {shard}/{replica} macros via a config.d overlay) and otel-collector-replicated services, plus replicated-schema.bats asserting the Atomic→Replicated conversion (fresh ClickHouse boots with an empty Atomic default), Replicated engines on all tables, and an ingest/query round-trip.
  • README documentation for the new env var and behavior, plus a minor changeset for @hyperdx/otel-collector.

Notes / out of scope:

  • No goose tracking table concerns: the seed runs goose with WithNoVersioning (no version table is created).
  • The experimental PromQL TimeSeries schema (ENABLE_PROMQL=true) is left untouched (not replication-aware).
  • The legacy exporter-managed schema path (HYPERDX_OTEL_EXPORTER_CREATE_LEGACY_SCHEMA=true / JSON mode) skips the seed tool and is unaffected.
  • Helm chart change to re-enable enableDatabaseSync + operator end-to-end verification happens in ClickStack-helm-charts as a follow-up that depends on this.

How to test locally

  1. cd packages/otel-collector && go test ./cmd/migrate/ — unit tests for the engine decision logic, DDL, and schema rewrite (including a pass over the real schema/seed/ files).
  2. cd smoke-tests/otel-collector && bats replicated-schema.bats (requires docker, bats, clickhouse-client) — boots a Keeper-enabled single-node ClickHouse whose default starts as empty Atomic, and asserts the seed converts it to Replicated, creates all tables with ReplicatedMergeTree/ReplicatedSummingMergeTree engines, and that log ingestion round-trips.
  3. Full smoke suite regression: bats *.bats — 33/33 pass locally.
  4. Manual: docker compose up --build -d ch-server-replicated otel-collector-replicated, then
    clickhouse-client --port=39000 --query="SELECT engine FROM system.databases WHERE name='default'"Replicated, and
    clickhouse-client --port=39000 --query="SELECT name, engine FROM system.tables WHERE database='default'" → all Replicated* (+ MaterializedView rollup MVs).

References

@changeset-bot

changeset-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7f880f4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/otel-collector Minor
@hyperdx/api Minor
@hyperdx/app Minor

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

@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Preview Aug 7, 2026 12:34am
hyperdx-storybook Ignored Ignored Preview Aug 7, 2026 12:34am

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Jul 30, 2026
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches 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:

  • Background tasks or delivery pipeline substantially modified — 452 lines (bar: 30):
    • packages/otel-collector/cmd/migrate/main.go
    • packages/otel-collector/cmd/migrate/main_test.go

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 12
  • Production lines changed: 643
  • Critical-path lines changed: 452
  • Branch: warren/hdx-4664
  • Author: wrn14897

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

Comment on lines +501 to +505
}

var tableCount uint64
if exists {
tableCount, err = countDatabaseTables(ctx, db, database)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Non-atomic emptiness check drops tables

When another schema manager or collector replica creates a table after countDatabaseTables returns zero, the later DROP DATABASE SYNC cascades over that table, deleting it and any newly written data despite the non-empty-database safeguard.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds opt-in support for Replicated ClickHouse databases and replicated table engines.

  • Ensures a requested Replicated database exists before applying the schema seed.
  • Rewrites supported MergeTree engines to their replicated variants.
  • Adds unit tests, a Keeper-backed smoke-test environment, documentation, and a package changeset.

Confidence Score: 4/5

The 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 DROP DATABASE ... SYNC without locking or revalidation, so another collector, operator, or schema client can populate the database during that interval and have its tables and data removed.

Files Needing Attention: packages/otel-collector/cmd/migrate/main.go

Important Files Changed

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

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 275 passed • 1 skipped • 1108s

Status Count
✅ Passed 275
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

The destructive path is guarded (DROP DATABASE only fires when system.tables reports zero tables), the feature is opt-in, and the engine rewrite runs after the compat-swap and PromQL-removal steps so it covers whichever schema variant survives. The issues below are real but none breaks the single-collector happy path.

🟡 P2 — recommended

  • packages/otel-collector/cmd/migrate/main.go:761 — Tables that already exist as plain MergeTree inside a Replicated database are silently left non-replicated, because every seed file is CREATE TABLE IF NOT EXISTS and the rewrite only edits SQL text, yet line 762 unconditionally logs that engines were rewritten to Replicated variants.
    • Fix: After the seed completes, query system.tables for MergeTree-family engines in the target database that do not start with Replicated and emit a loud warning naming each one.
  • packages/otel-collector/cmd/migrate/main.go:521 — The engine/table-count snapshot read at lines 498–505 is acted on at line 521 with an unconditional DROP DATABASE ... SYNC (no IF EXISTS), and docker/otel-collector/entrypoint.sh:37 runs the seed in every collector container, so two containers cold-starting together can both decide to convert and the loser either drops the database the winner just created or errors into log.Fatalf.
    • Fix: Add IF EXISTS to the drop and re-read the engine and table count immediately before it under a Keeper-backed guard, or move the seed to a single leader-elected job.
    • reliability, orchestrator
  • packages/otel-collector/cmd/migrate/main.go:537mergeTreeEngineRe matches only MergeTree and SummingMergeTree, so any other MergeTree-family engine added to the seed later (ReplacingMergeTree, AggregatingMergeTree, CollapsingMergeTree) is silently left non-replicated inside a Replicated database.
    • Fix: Match the whole family with a pattern such as ^ENGINE = (\w*MergeTree)\b, skip names already prefixed with Replicated, and fail loudly on an engine the rewrite does not recognize.
  • packages/otel-collector/cmd/migrate/main_test.go:1191 — The guard test over the real seed directory asserts !mergeTreeEngineRe.Match(content) using the same regex that performed the rewrite, so it is self-satisfying and cannot detect an engine outside that regex's alternation.
    • Fix: Assert positively that every ENGINE = line in the processed seed either begins with Replicated or appears on an explicit allow-list containing TimeSeries.
  • packages/otel-collector/cmd/migrate/main.go:497ensureReplicatedDatabase and the engine re-query at line 711 have no retry and call log.Fatalf on any error, while the goose path at lines 341–369 gets five attempts with exponential backoff, so one transient Keeper or ClickHouse hiccup during CREATE DATABASE kills the container.
    • Fix: Route both calls through the same backoff helper used by runMigrationWithRetry before falling back to a fatal exit.
    • reliability, orchestrator
  • packages/otel-collector/cmd/migrate/main_test.go:1051dbActionKeep, the branch whose entire purpose is preventing data loss, is covered only as a pure function; no test exercises ensureReplicatedDatabase itself for any branch, so the drop/create sequencing and the refuse-to-drop early return are unverified.
    • Fix: Add a smoke case that points the replicated collector at a non-empty non-Replicated database and asserts the tables survive and the warning is logged.
🔵 P3 nitpicks (6)
  • packages/otel-collector/cmd/migrate/main.go:409isDefaultEngine returns true for "Atomic", so setting that value does not force the Atomic engine; if the database is already Replicated the tables are still rewritten to Replicated engines, while the validation error at lines 80–82 advertises Atomic as a supported choice.
    • Fix: Document that Atomic means "leave the engine as-is", or drop it from the accepted value set.
  • packages/otel-collector/cmd/migrate/main.go:459 — The Keeper path /clickhouse/databases/%s is hardcoded to match an external project's convention with no override, so a deployment using a different layout gets a mismatched znode path.
    • Fix: Allow the Keeper path prefix to be overridden by an environment variable, defaulting to the current value.
  • packages/otel-collector/cmd/migrate/main.go:686context.Background() carries no deadline, so the Keeper-dependent CREATE/DROP DATABASE calls are bounded only by the server-side max_execution_time rather than a client-controlled timeout.
    • Fix: Wrap the Replicated-engine calls in context.WithTimeout.
  • smoke-tests/otel-collector/README.md:42 — The "Signals covered" and test-structure sections were not updated for replicated-schema.bats or the new host ports 39000, 38123, and 54318 that the suite now requires.
    • Fix: Add the replicated suite and its ports to the README alongside the existing compat-schema entry.
  • smoke-tests/otel-collector/data/replicated-schema/engines/expected.snap:1 — The snapshot holds five lines for six queries because the second query is expected to return no rows, which makes an offset mismatch confusing to diagnose when it fails.
    • Fix: Note the intentionally empty result in a comment in assert_query.sql, or split the catch-all query into its own fixture.
  • smoke-tests/otel-collector/docker-compose.yaml:160ch-server-replicated pins clickhouse/clickhouse-server:26.5-alpine inline while the sibling services extends their image from docker-compose.ci.yml, so the replicated suite can drift from the version the rest of the suite tests.
    • Fix: Extend from the shared compose file and override only the config volume and ports.

Reviewers (7): correctness, adversarial, testing, maintainability, data-migrations, project-standards, reliability.

Coverage caveat: Bash, Grep, and Glob were unavailable in this environment (bwrap sandbox failure on every invocation), so scope was reconstructed by reading files directly rather than from git diff, and only the reliability reviewer returned before the output deadline — the remaining findings come from direct inspection of main.go, main_test.go, the seed SQL, the smoke-test fixtures, README.md, and entrypoint.sh. Two rollup seed files defining otel_logs_kv_rollup_15m and otel_traces_kv_rollup_15m could not be located without directory listing and were not read; the changeset file could not be verified.

Testing gaps:

  • No coverage of the compat schema (ClickHouse < 26.2) combined with the Replicated engine — the smoke suite exercises only the 26.5 path.
  • No coverage of ENABLE_PROMQL=true combined with a Replicated database.
  • No test simulates concurrent seed runs or an unavailable Keeper during CREATE DATABASE ... ENGINE = Replicated(...).
  • The smoke suite runs a single collector against a single-node server, so multi-replica startup ordering and crash recovery between the drop and the create are unexercised.

…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.
@jordan-simonovski

Copy link
Copy Markdown
Contributor

Is the Greptile issue worth addressing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants