[OPIK-7773] [BE] feat: fail readiness when the traces wrap flag disagrees with the DB topology - #7948
Merged
thiagohora merged 11 commits intoAug 26, 2026
Conversation
github-actions
Bot
added
documentation
Contributor
⏱️ pre-commit per-hook timing
⏭️ 36 skipped (no matching files changed)
|
Contributor
|
🌿 Preview your docs: https://opik-preview-01a03dd3-ae0e-770f-86e1-b79fff1b3d60.docs.buildwithfern.com/docs/opik No broken links found Unverified links (timeout / rate-limited / server error — not failing the check)• https://aistudio.google.com/apikey (401) 📌 Results for commit 99130a2 |
Collaborator
|
Already covered by a test in this PR. The probe ships with its own cover, at the endpoints that matter. HealthCheckIntegrationTest.TracesDistributedWrapEnabledWithoutTheWrap boots the app with tracesDistributedWrapEnabled=true over an unwrapped Not testable yet. The /is-alive/ping half is asserted nowhere. Because the check is critical, IsAliveResource — which filters on isCritical alone and ignores also touches Backend (Java API / internal), Deployment / Helm Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 26 Aug 11:31 UTC. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Contributor
Backend Tests - Integration Group 14333 tests 331 ✅ 5m 51s ⏱️ Results for commit 5643882. ♻️ This comment has been updated with latest results. |
thiagohora
force-pushed
the
thiagoh/OPIK-7773-fail-fast-traces-topology-assertion
branch
from
August 25, 2026 10:21
4dc98c5 to
cc90574
Compare
andrescrz
previously approved these changes
Aug 26, 2026
andrescrz
left a comment
andrescrz
left a comment
Member
There was a problem hiding this comment.
Just something to double check, but no blockers.
thiagohora
force-pushed
the
thiagoh/OPIK-7773-fail-fast-traces-topology-assertion
branch
from
August 26, 2026 10:31
33fabf6 to
f0cbf44
Compare
Contributor
Backend Tests - Unit Tests3 481 tests 3 479 ✅ 1m 51s ⏱️ Results for commit 7a9bcd4. ♻️ This comment has been updated with latest results. |
andrescrz
previously approved these changes
Aug 26, 2026
…rees with the DB Post-cutover `traces` is a Distributed table that cannot take mutations, so TraceDAO routes trace deletes at `traces_local` only while `databaseAnalyticsDataModel.tracesDistributedWrapEnabled` is on (OPIK-7455). If an install's flag does not match its database, every trace delete breaks — stale-`false` over a wrapped `traces` with code 36/48, stale-`true` over an unwrapped one with code 60 — and nothing says so until the first delete runs. The answer depends on each environment's Helm config plus its live database, so this cannot be a CI guard; it belongs at readiness. Adds ClickHouseTracesTopologyHealthCheck (`clickhouse-traces-topology`, critical/ready): one `system.tables` lookup covering both tables and both directions — flag on asserts `traces` is Distributed and `traces_local` exists, flag off asserts `traces` is a (Replicated)MergeTree — reporting the flag, the observed engine and the fix on a mismatch. An absent `traces` is unhealthy either way, since trace reads and writes cannot work at all. Unlike the sibling `clickhouse-cluster` / `clickhouse-cold-storage-disk` probes it is deliberately not toggle-gated: flag off over a MergeTree `traces` is the default on every install as shipped (OSS Docker, self-hosted, pre-cutover SaaS), so the assertion holds universally and only a genuine misconfiguration trips it. The flag stays the source of truth — the probe reports, it never re-routes, and no routing logic changed. Collects the db health checks into `infrastructure/db/healthchecks`: the new probe extends the package-private AbstractClickHouseHealthCheck, so it can only live alongside it, and the package now holds the family (both abstracts, the four ClickHouse probes, MysqlHealthyCheck) plus their tests instead of scattering them through `infrastructure/db`. Test coverage is three-layered. ClickHouseTracesTopologyHealthCheckTest asserts the exact message of every case, not merely that the probe went red — the message is all an operator sees. HealthCheckIntegrationTest covers the matching flag-off install (healthy, folded into the existing per-check and aggregate expectations) and the flag-on-without-the-wrap mismatch on the shared containers, since the probe only reads system.tables. ClickHouseTracesTopologyReadinessTest takes dedicated, non-reused ClickHouse and ZooKeeper containers — the wrap destructively renames the live `traces` — and walks the real transition: ready, apply the wrap block verbatim from 000003_exchange_and_wrap.sql, then not ready. Both mismatch directions assert `/health-check?name=all&type=ready` returns 503, the chart's actual `component.backend.readinessProbe` path, so the tests prove the pod really does leave rotation rather than merely that a row flipped. Documents the check where operators meet it: a self-host changelog entry and a troubleshooting section (mirrored into both docs trees) stating that the failure is intentional, giving the engine/flag table and the `system.tables` query to resolve it, and warning against removing the check instead of fixing the mismatch. The chart already wires the flag through values -> configmap -> env (OPIK-7455), so values.yaml gains only the note that the flag is asserted at readiness. The cutover runbook gains the operational consequence this introduces: the flag and the wrap cannot land simultaneously, so the unavoidable mismatch window is now a readiness window in either order — pods leave rotation rather than just failing deletes. It must therefore sit inside the maintenance window `--confirm-maintenance` already asserts for the wrap. The window is self-clearing, since the probe re-evaluates continuously and rotation returns once the two sides are back in step. The runbook's claim that no readiness endpoint exposes the flag's value is replaced by the endpoint that now does. Implements OPIK-7773. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…op the changelog entry The readiness suite assumed it starts on an unwrapped `traces`, which holds today (the wrap lives in data-migrations/, outside the migrations/ directory the analytics changelog includes) but breaks the day it lands as a regular migration: the container would arrive already Distributed, `CREATE TABLE traces_dist` would fail on the second pass, and asserting "healthy first" would fail on a correctly behaving probe. applyDistributedWrap() now returns early when `traces` is already Distributed, and the pre-wrap state is read rather than assumed: the healthy half of the transition runs only when there is something to transition from, while the mismatch half — the point of the test — always runs. probeWithTheFlagOnIsHealthyOverTheWrappedTopology wraps idempotently instead of asserting that a previous test left the topology in place, so it no longer depends on @order to pass; the ordering stays only so the transition test still gets the pristine pre-wrap state. Verified both ways by running the suite with the order reversed. Drops the self-host changelog entry: which release ships this is not decided yet, so the entry would state a version we cannot stand behind. The troubleshooting section carries the operator guidance in the meantime and does not link to the changelog, so nothing dangles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…traces_local to a mutable engine Two review findings, both in scope. SQL is never assembled with Java string operations (.agents/skills/opik-backend/SKILL.md:146 — "don't copy them and don't add new ones"), and both new queries did. TOPOLOGY_QUERY is now a literal text block instead of `.formatted(TRACES_TABLE, TRACES_LOCAL_TABLE)`; the constants stay as the single source for the map lookups and the messages, and the unit test stubs the exact query text, so the two cannot drift apart unnoticed. The readiness suite's wrap DDL likewise spells the database out rather than interpolating DATABASE_NAME — ClickHouse cannot bind an identifier inside a Distributed() engine argument, so the literal is held honest by an assertion in beforeAll instead. The `.formatted(...)` calls that remain build health check messages, which the rule explicitly still allows. The flag-on branch accepted `traces_local` on presence alone, so a same-named View, Log or nested Distributed table would report healthy while TraceDAO's DELETEs failed against it — the same outcome as an absent table, which the probe already rejects. It now holds that table to the MergeTree family, sharing the existing family-suffix predicate with the flag-off branch. This needs no extra query: the engine was already in the row the probe reads, so the assertion is free. Validating the Distributed engine_full's cluster/database/sharding-key arguments and the shard's full schema was left out — that is a topology audit, well past the ticket's "one cheap system.tables check", and it belongs with the CI DDL guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he readiness DTO builder Two more review findings, both in scope. The runbook's self-clearing note had the restart on the wrong path: it read "no extra restart needed on the wrap-first path, and only the already-planned restart on the toggle-first path", which is backwards. Both orderings spend exactly the one planned rolling restart the toggle already requires, and neither needs another; what differs is its position, and with it what closes the mismatch window — the wrap DDL on the toggle-first path (the restart is already done by then), the restart completing on the wrap-first path. Reworded to say that. HealthCheckResponse used a plain @builder, copied from the same record in HealthCheckIntegrationTest, where the convention predates the rule. Records and DTOs take @builder(toBuilder = true) (.agents/skills/opik-backend/SKILL.md:52, which lists bare @builder as the anti-pattern), so the new copy uses it; the older one is left alone as unrelated to this ticket. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the accepted engines Review feedback, and the first one was demonstrated by my own test fixture. **The not-wrapped message promised the wrong failure.** It ended "otherwise trace deletes fail with UNKNOWN_TABLE (60)", but that branch fires whenever the flag is on and `traces` is not Distributed — and `traces_local` may well still exist in that state. A rollback leaves exactly that shape: the wrapper dropped, the original `traces` promoted back, the old shard lingering. The routed delete then *succeeds against the stale shard* and the live rows are never touched, which is quieter than the error and worse. The check's own unit test seeds `traces_local` as present while asserting the UNKNOWN_TABLE wording, so the fixture disproved the message. Now states both outcomes and which one applies when, with the reasoning recorded on the constant so it does not get "simplified" back. **SharedMergeTree was accepted but undocumented.** The probe matches on the `MergeTree` suffix, so ClickHouse Cloud's SharedMergeTree family satisfies it, but config.yml, the Javadoc and the troubleshooting table all said "(Replicated)MergeTree" — leaving a valid Cloud install looking misconfigured. All three now describe the accepted family. **Documented that `critical: true` also gates /is-alive/ping.** IsAliveResource filters on isCritical alone and ignores `type`, so a readiness mismatch reports the server as down to SDKs and makes them buffer. That is the existing behaviour of every `ready` check here (`clickhouse`, `db`, `redis`, `mysql`), and it is intended for this one too — but it was an unstated consequence, so it is now stated where the flag is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g node Review follow-up: the probe reads the node-local system.tables, and nothing said so or said why. It stays node-local — clusterAllReplicas needs REMOTE + CLUSTER grants the app user is not guaranteed to hold (so a non-toggle-gated critical check would fail on installs as shipped), one unreachable replica would pull the whole fleet from rotation over a condition that breaks no delete, and on a shared catalog (ClickHouse Cloud / SharedMergeTree) node-local already is cluster-wide. The limit that follows is now stated instead of implied: a divergence confined to some replicas degrades into sporadic unhealthy reports rather than a fleet-wide outage, and the cluster-wide "did this ON CLUSTER DDL reach every replica" gate stays in exchange_and_wrap.sh / finalize.sh, where it is fail-loud and operator-run. The troubleshooting page gains the clusterAllReplicas form for an operator chasing an intermittent failure, with the fix being the lagging replica, not the flag. Comment and documentation only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m env precedence Two review catches on the troubleshooting page. The cluster-wide diagnostic said "the engine must be identical on all of them", which reads as if `traces` and `traces_local` must match each other — the exact opposite of the healthy post-wrap shape. Compare down each table instead: one table's engine must be the same on every host, and the two tables differing from each other is the wrap working. The resolution told operators to set the chart value, but the configmap derives ANALYTICS_DB_DATA_MODEL_TRACES_DISTRIBUTED_WRAP_ENABLED only when it is absent from component.backend.env, so an explicit entry there silently wins and the check keeps failing after the change. Say so, and give the configmap read that shows what the backend actually receives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the flag Review feedback, and a good catch for a troubleshooting page specifically. configmap-backend.yaml emits the derived ANALYTICS_DB_DATA_MODEL_TRACES_DISTRIBUTED_WRAP_ENABLED key only when `component.backend.env` does not already define it (`if not (hasKey $env $k)`). So an operator following this page, changing databaseAnalyticsDataModel.tracesDistributedWrapEnabled while an explicit env entry exists, sees no effect at all: the check keeps failing with the same message and nothing they change appears to help. That is precisely the state a troubleshooting page exists to get someone out of. Adds a callout with the precedence, the `printenv` command to see what the pod actually received, and the fix — remove the explicit key so the setting is the single source, or update it there instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The page already documented this, further down the same Resolution section. I added a second callout saying the same thing before checking, which is worse than having missed it. Reverted to the existing wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nostic The node-local query hardcoded `database = 'opik'` while the cluster-wide one right below it used `<database_name>`, so the page contradicted itself — and on any install with a custom ANALYTICS_DB_DATABASE_NAME the hardcoded form returns no rows, which reads as "the tables are missing" rather than "wrong database". Both queries now take the placeholder, with the substitution stated once, up front, where it covers both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…econd reader OPIK-7772's routing guard (#7953) landed on main after this branch last rebased and allows exactly one reader of tracesDistributedWrapEnabled — TraceDAOImpl's accessor. This probe is a second one, so the rebase turned it into a build failure. The guard is right to notice; the exemption is what was missing. Widened to an allowlist of two, still member-scoped so it is not a category hole: TraceDAOImpl#tracesDistributedWrapEnabled and the probe's constructor. What the rule protects is the routing decision — a second place that branches on the flag is a second place that can name the wrong table — and the probe names no table and issues no mutation. It reads the flag to assert it against the live `system.tables` engine and report a mismatch at readiness. byCodeUnitsThat rather than byMethodsThat because the flag is read at injection and a constructor is not a JavaMethod, so the method-only form could report the call but never admit it. Renamed the rule off "exactly_one_place", which two readers would have made a lie. Verified the teeth both ways: a third reader added as a method on a sibling health check in the same package, and again as a constructor on another, each fails the rule. Suites run: TraceMutationRoutingArchTest, TraceMutationSqlRoutingTest, Trace/SpanDeletionEventArchTest, the four healthcheck suites (55 tests, green). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thiagohora
force-pushed
the
thiagoh/OPIK-7773-fail-fast-traces-topology-assertion
branch
from
August 26, 2026 11:27
268a83f to
7a9bcd4
Compare
andrescrz
approved these changes
Aug 26, 2026
Comment on lines
+70
to
+72
| private static DescribedPredicate<JavaCodeUnit> only(Class<?> owner, String memberName) { | ||
| return DescribedPredicate.describe("%s.%s".formatted(owner.getSimpleName(), memberName), | ||
| codeUnit -> codeUnit.getOwner().isEquivalentTo(owner) && codeUnit.getName().equals(memberName)); |
Contributor
There was a problem hiding this comment.
Unauthorized overloads bypass routing guard
only matches code units by owner and getName() alone, so same-named constructor or method overloads can satisfy the exemptions and let an unauthorized flag reader or routing-table resolver pass the architecture guard — should we match parameter types for ClickHouseTracesTopologyHealthCheck, TraceDAOImpl#tracesDistributedWrapEnabled, and #tracesMutationTable, with wrong-signature regression coverage?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/domain/TraceMutationRoutingArchTest.java`
around lines 70-72, update the `only` predicate so it matches the owner, member name,
and exact parameter types rather than accepting every same-named constructor or method.
Use the intended `ClickHouseTracesTopologyHealthCheck` three-argument constructor and
the exact `TraceDAOImpl` accessor/resolver signatures, then add ArchUnit regression
fixtures or tests proving wrong-signature overloads are rejected.
thiagohora
deleted the
thiagoh/OPIK-7773-fail-fast-traces-topology-assertion
branch
August 26, 2026 11:49
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
3 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.
Details
Post-cutover
tracesis aDistributedtable that cannot take mutations, soTraceDAOroutes trace deletes attraces_localonly whiledatabaseAnalyticsDataModel.tracesDistributedWrapEnabledis on (OPIK_7455). When an install's flag disagrees with its database every trace delete breaks — code 36/48 one way, code 60 the other — and nothing says so until the first delete runs. This adds a readiness check that asserts the flag against the live topology at startup, so a misconfigured install is pulled from rotation instead of serving traffic it cannot mutate. It depends on each environment's Helm config plus its live database, which is why it can't be a CI guard.ClickHouseTracesTopologyHealthCheck(clickhouse-traces-topology,critical: true/type: ready) — onesystem.tableslookup covers both tables and both directions: flag on requirestracesto beDistributedandtraces_localto exist; flag off requirestracesto be a(Replicated)MergeTree. An absenttracesis unhealthy either way. On a mismatch the message names the flag, the engine actually observed, and the fix.clickhouse-cluster/clickhouse-cold-storage-diskprobes: flag off over aMergeTreetracesis the default on every install as shipped (OSS Docker, self-hosted, pre-cutover SaaS), so the assertion holds universally and only a genuine misconfiguration trips it.AbstractClickHouseHealthCheck, so it can only live beside it.infrastructure/db/healthchecksnow holds the family — both abstracts, the four ClickHouse probes,MysqlHealthyCheck— plus their tests, instead of scattering them throughinfrastructure/db.--confirm-maintenance, so the window belongs inside that maintenance window — which the cutover runbook now says explicitly. If the team would rather the cutover degrade than go dark, the lever iscritical: falseinconfig.yml: still reported on/health-check, no longer gating rotation. Worth an explicit call here.traces_local, nottraces_local_v2. The latter is the backfill shadow table — created empty in every install by migrations000101/000114,EXCHANGEd and then renamed totraces_pre_cutover_backupbefore the wrap (exchange_and_wrap.shhard-errors if it still exists at wrap time).traces_localis what the wrap'sRENAMEproduces and whatTraceDAO.java:1934/:5129actually mutate under the flag, which is precisely what this check exists to protect.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
AbstractClickHouseExistenceHealthCheck/TracesDistributedWrapMutationTestpatterns in this repo.Testing
Scenarios validated:
tracestopologyfalse(Replicated)MergeTreeHealthCheckIntegrationTest.DefaultConfig(realsystem.tablesquery)trueMergeTree(never wrapped)HealthCheckIntegrationTest.TracesDistributedWrapEnabledWithoutTheWrapfalseDistributed(cut over)ClickHouseTracesTopologyReadinessTesttrueDistributedovertraces_localClickHouseTracesTopologyReadinessTest/health-check?name=all&type=readyreturns503— the chart's actualcomponent.backend.readinessProbepath — so the tests prove the pod really leaves rotation, not just that a JSON row flipped.ClickHouseTracesTopologyReadinessTestuses dedicated, non-reused ClickHouse + ZooKeeper containers (the wrap destructively renames the livetraces) and applies the wrap block verbatim from000003_exchange_and_wrap.sql, walking the real transition: ready → wrap → not ready.ClickHouseTracesTopologyHealthCheckTest(13 cases, asserting exact messages since the message is all an operator sees):tracesabsent under either flag;Distributedtraceswithtraces_localmissing;MergeTree/Replicated/Sharedall accepted when the flag is off; a non-MergeTree engine rejected; query failure and interrupt paths cancel the in-flight query and restore the interrupt flag.TracesDistributedWrapMutationTest(OPIK_7455's suite, which boots with the flag on and the wrap applied) still passes with the new critical readiness check in place.26.3.16.16-alpine+ ZooKeeper3.9.4, plus the shared MySQL/Redis containers).Not run, with reason:
helm unittest— plugin not installed locally.helm templatealso unavailable (chart dependencies not vendored). Thevalues.yamlchange is comment-only, so nothing renders differently;tests/configmap_env_test.yamlis untouched and its assertions are unaffected.Documentation
self-host/troubleshooting.mdx— new "Backend Not Ready:clickhouse-traces-topology" section, mirrored into bothdocs/anddocs-v2/(these pages are kept in sync): the failure is intentional, here is the flag/engine table and thesystem.tablesquery to resolve it, and do not remove the check instead of fixing the mismatch.data-migrations/traces-local-v2-cutover/README.md— the readiness-window consequence described above; its claim that no readiness endpoint exposes the flag's value is replaced by the endpoint that now does.apps/opik-backend/config.yml,DatabaseAnalyticsDataModelConfigjavadoc,deployment/helm_chart/opik/values.yaml— the flag is now asserted at readiness. The chart already wires the flag through values → configmap → env (OPIK_7455), sovalues.yamlgains only that note; these keys don't use the# --prefixhelm-docsreads, so the chartREADME.mdneeds no regeneration.🤖 Generated with Claude Code