Commit 766a2d7
[OPIK-7773] [BE] feat: fail readiness when the traces wrap flag disagrees with the DB topology (#7948)
* [OPIK-7773] [BE] feat: fail readiness when the traces wrap flag disagrees 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>
* [OPIK-7773] [BE] test: make the readiness suite's wrap idempotent; drop 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>
* [OPIK-7773] [BE] fix: keep the topology probe's SQL literal and hold 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>
* [OPIK-7773] [BE] fix: correct the runbook's restart attribution and the 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>
* [OPIK-7773] [BE] fix: correct the not-wrapped diagnosis and document 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>
* [OPIK-7773] [BE] docs: pin the topology probe's scope to the answering 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>
* [OPIK-7773] [BE] docs: disambiguate the per-replica check and the Helm 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>
* [OPIK-7773] [BE] docs: warn that an explicit Helm env entry overrides 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>
* [OPIK-7773] [BE] docs: revert duplicate Helm precedence callout
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>
* [OPIK-7773] [BE] docs: parameterise the database in the topology diagnostic
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>
* [OPIK-7773] [BE] test: admit the readiness probe as the wrap flag's second 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>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 82f6bc2 commit 766a2d7
20 files changed
Lines changed: 966 additions & 28 deletions
File tree
- apps
- opik-backend
- data-migrations/traces-local-v2-cutover
- src
- main/java/com/comet/opik/infrastructure
- db/healthchecks
- test
- java/com/comet/opik
- domain
- infrastructure
- db/healthchecks
- resources
- opik-documentation/documentation/fern/docs-v2/self-host
- deployment/helm_chart/opik
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
154 | 154 | | |
155 | 155 | | |
156 | 156 | | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
157 | 160 | | |
158 | 161 | | |
159 | 162 | | |
| |||
235 | 238 | | |
236 | 239 | | |
237 | 240 | | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
238 | 262 | | |
239 | 263 | | |
240 | 264 | | |
| |||
Lines changed: 28 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
212 | 212 | | |
213 | 213 | | |
214 | 214 | | |
215 | | - | |
216 | | - | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
217 | 219 | | |
218 | 220 | | |
219 | 221 | | |
| |||
227 | 229 | | |
228 | 230 | | |
229 | 231 | | |
230 | | - | |
| 232 | + | |
231 | 233 | | |
232 | 234 | | |
233 | 235 | | |
| |||
236 | 238 | | |
237 | 239 | | |
238 | 240 | | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
239 | 261 | | |
240 | | - | |
241 | | - | |
242 | | - | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
243 | 265 | | |
244 | 266 | | |
245 | 267 | | |
| |||
Lines changed: 6 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
50 | 50 | | |
51 | 51 | | |
52 | 52 | | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
53 | 59 | | |
54 | 60 | | |
55 | 61 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
0 commit comments