Skip to content

Commit 3191a31

Browse files
thiagohoraclaude
andcommitted
[OPIK-7686] [DOCS] Document where the async-insert cutover bump lives
Records the decision from the #7675 review thread and closes the gap it left in the runbook: the cutover bump to async_insert_busy_timeout_max_ms is a temporary ANALYTICS_DB_ASYNC_INSERT_BUSY_TIMEOUT_MAX_MS entry on the deployed backend's own config, not a new Helm chart value. New "Where the buffer bump lives" section states the decision with its rationale, the key and value, and the revert step -- delete the key rather than set 250, since unset means "leave queryParameters alone" and restores the baked-in default in one edit. Prereq 6, sequence step 2, the manual-actions note and the final-cutover-window step previously said "config, see below" without ever saying what to set; they now point here. It also corrects the record: the reason first given on #7675 for keeping these out of the chart (empty values would not be inert) was wrong, since config.yml already ships the empty default everywhere. The exclusion stands on reversibility and scope instead. Two operational facts the runbook did not state: - The value reaches the backend through the container environment, which Kubernetes injects at container start only, so editing the ConfigMap does not reach a running pod. How the restart is triggered is deployment-specific and the chart ships no automation for it, so the runbook says to know which case applies before the window -- and, either way, to verify the ceiling is live on every instance rather than assume it. - The revert is delivered by a second restart while pods hold inserts parked for up to the widened ceiling, so the chosen ceiling must stay below terminationGracePeriodSeconds (the Kubernetes default 30s; the chart does not set it). Because traceColumnsNonNullable is another entry in the same backend config and ConfigMap, and "The final cutover window" step 1 asks for both, they should land together so the fleet restarts once. Kept deployment-agnostic on purpose. The bump is temporary, so it does not belong on an official Opik doc page, and Comet's own deployment paths are recorded internally rather than here; self-hosted instructions can follow if and when these knobs get a documented home. Also adds the explicit warning against bumping the value by editing ANALYTICS_DB_QUERY_PARAMETERS, and a Go/No-Go item for having the bump and its revert pre-written. No chart change: the chart-value option was not chosen, so the three ANALYTICS_DB_ASYNC_INSERT_* knobs stay out of values.yaml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 234e82a commit 3191a31

1 file changed

Lines changed: 96 additions & 11 deletions

File tree

  • apps/opik-backend/data-migrations/traces-local-v2-cutover

apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md

Lines changed: 96 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -73,14 +73,17 @@ new table before the EXCHANGE. The replay matches the **full key**, not `id` alo
7373
6. **Cutover buffer knob ready**`databaseAnalytics.asyncInsertBusyTimeoutMaxMs` (env
7474
`ANALYTICS_DB_ASYNC_INSERT_BUSY_TIMEOUT_MAX_MS`), unset by default so the buffer inherits the
7575
`async_insert_busy_timeout_max_ms=250` carried by `queryParameters`. Raise it to ~10000 for the cutover, then unset it
76-
again. The ceiling is a backend per-query setting applied on the backend's own ClickHouse client, so the migration
77-
scripts' direct `clickhouse-client` session **cannot read or verify it**. It is therefore **operator-asserted**:
78-
`exchange_and_wrap.sh` refuses the EXCHANGE without `--confirm-buffer-raised` (a fail-fast acknowledgment gate — it
79-
forces the operator to confirm the step, though it cannot prove the value took effect). Confirm it actually took
80-
effect on the prod-clone/staging load test (the Go/No-Go "Async-insert ceiling confirmed" item) before production.
81-
**Also confirm client/SDK insert timeouts
82-
exceed the widened buffer** (~10s) — with `wait_for_async_insert=1` a raised ceiling blocks each insert until it
83-
flushes, so a shorter client timeout would surface as ingestion errors during the window.
76+
again. **Where it is set, the exact value, the rollout and the revert step are in
77+
["Where the buffer bump lives"](#where-the-buffer-bump-lives-and-how-to-revert-it)** — it is a temporary env var on the
78+
deployment's own backend config, not a chart value (OPIK-7686). Have that config change written and reviewed *before*
79+
the window, so applying it is a merge, not an edit. The ceiling is a backend per-query setting applied on the backend's own
80+
ClickHouse client, so the migration scripts' direct `clickhouse-client` session **cannot read or verify it**. It is
81+
therefore **operator-asserted**: `exchange_and_wrap.sh` refuses the EXCHANGE without `--confirm-buffer-raised` (a
82+
fail-fast acknowledgment gate — it forces the operator to confirm the step, though it cannot prove the value took
83+
effect). Confirm it actually took effect on the prod-clone/staging load test (the Go/No-Go "Async-insert ceiling
84+
confirmed" item) before production. **Also confirm client/SDK insert timeouts exceed the widened buffer** (~10s) —
85+
with `wait_for_async_insert=1` a raised ceiling blocks each insert until it flushes, so a shorter client timeout would
86+
surface as ingestion errors during the window.
8487
7. **Schema-state flag wired, with a rollout plan**`databaseAnalyticsDataModel.traceColumnsNonNullable` (env
8588
`ANALYTICS_DB_DATA_MODEL_TRACE_COLUMNS_NON_NULLABLE`, default `false`). The successor's `end_time`/`ttft` are
8689
**non-nullable sentinel** columns, so the app must represent an absent value as the epoch/NaN sentinel — not `null`
@@ -158,7 +161,9 @@ new table before the EXCHANGE. The replay matches the **full key**, not `id` alo
158161
It executes the reference statement in
159162
[`000001_backfill_traces_local_v2.sql`](scripts/db-app-analytics/000001_backfill_traces_local_v2.sql) — the script
160163
reads that file and substitutes the window bounds, so the two never drift.
161-
2. **Raise the buffer ceiling** (config, see below), then **[`scripts/delta_replay.sh`](scripts/delta_replay.sh)**
164+
2. **Raise the buffer ceiling** (config — see
165+
["Where the buffer bump lives"](#where-the-buffer-bump-lives-and-how-to-revert-it) for the key, the value and the
166+
restart wait), then **[`scripts/delta_replay.sh`](scripts/delta_replay.sh)**
162167
(reference SQL [`000002_delta_and_deletion_replay.sql`](scripts/db-app-analytics/000002_delta_and_deletion_replay.sql))
163168
— delta-insert (anchored at `backfill_start`), then **deletion replay**. The replay runs with
164169
`lightweight_deletes_sync = 2`, so it returns only once the delete mutation has applied on **every** replica.
@@ -263,6 +268,77 @@ new table before the EXCHANGE. The replay matches the **full key**, not `id` alo
263268
delta one). This is normal — `ReplacingMergeTree` collapses them on merge / under `FINAL` / `LIMIT 1 BY id`, highest
264269
`last_updated_at` winning. Do not "fix" it.
265270

271+
### Where the buffer bump lives (and how to revert it)
272+
273+
**Decision (OPIK-7686): a temporary env var on the deployed backend's own configuration — not a chart value.** The three
274+
`ANALYTICS_DB_ASYNC_INSERT_*` knobs are deliberately absent from the chart's `values.yaml` (OPIK-6880, #7675).
275+
Rationale:
276+
277+
- **No chart change is needed.** `component.backend.env` is a free-form map rendered straight into the backend
278+
ConfigMap, so a deployment-level entry is already sufficient.
279+
- **Removal is a clean one-step rollback.** Unset means "leave `queryParameters` alone" (`DatabaseAnalyticsFactory`), so
280+
*deleting* the key restores the baked-in `async_insert_busy_timeout_max_ms=250`. There is no "set it back to 250" edit,
281+
and therefore no pinned value that can later drift from the `config.yml` default. For a time-boxed window that
282+
reversibility is the property worth optimising for.
283+
- **The value is deployment- and window-specific** — one environment, for the length of the cutover. Keeping it in that
284+
deployment's own config leaves it version-controlled and auditable without turning a temporary state into a permanent
285+
chart default that every install inherits.
286+
- **A chart value would save no work**: you edit the deployment config either way.
287+
288+
> For the record, the reason first given on #7675 for excluding these — that rendering them would send empty strings
289+
> where the backend expects an integer, so it "would not be inert" — was **wrong**. `config.yml` ships
290+
> `${ANALYTICS_DB_ASYNC_INSERT_*:-}` as the default for all three, so the empty case is the normal path in every
291+
> environment today: an empty substitution leaves a bare YAML scalar that parses to a null `Integer`, and `@Min(1)` does
292+
> not fire on null. Exposing them in the chart with empty defaults *would* be safe. The decision above rests on
293+
> reversibility and scope, not on safety.
294+
295+
> **Never bump it by editing `ANALYTICS_DB_QUERY_PARAMETERS`.** That means re-pasting the entire tuning string
296+
> (`compress`, `failover`, `async_insert`, `wait_for_async_insert`, the skip-index and shard settings, …), which risks
297+
> silently dropping one of the others and drifting from the `config.yml` default. The dedicated
298+
> `ANALYTICS_DB_ASYNC_INSERT_BUSY_TIMEOUT_MAX_MS` override exists precisely so the cutover states only the one value it
299+
> is changing.
300+
301+
**What to set, and how to revert.** One key, on the backend — a `component.backend.env` entry under Helm, a backend env
302+
var under compose:
303+
304+
```
305+
ANALYTICS_DB_ASYNC_INSERT_BUSY_TIMEOUT_MAX_MS=10000
306+
```
307+
308+
**Revert by deleting the key, not by setting `250`.** Only the ceiling changes: leave `…_MIN_MS` and `…_MAX_DATA_SIZE`
309+
unset, so the floor stays at the `async_insert_busy_timeout_min_ms=100` carried by `queryParameters`, and widening the
310+
ceiling alone is what parks the inserts.
311+
312+
**It takes effect only on a backend restart — so confirm the restart finished before continuing.** The backend receives
313+
this through the container environment (`envFrom.configMapRef` under Helm), which Kubernetes injects at container start
314+
only: editing the ConfigMap does not reach a running pod. **How that restart is triggered is deployment-specific** — the
315+
chart ships no automation for it, so some deployments run a ConfigMap watcher that rolls the workload on its own while
316+
others need an explicit `kubectl rollout restart deployment/opik-backend`. Know which one yours is *before* the window.
317+
Either way the operator's obligation is identical, because the ceiling has to be live on **every** instance before step
318+
2 — so verify rather than assume:
319+
320+
```bash
321+
kubectl rollout status deployment/opik-backend -n <namespace>
322+
kubectl get cm opik-backend -n <namespace> \
323+
-o jsonpath='{.data.ANALYTICS_DB_ASYNC_INSERT_BUSY_TIMEOUT_MAX_MS}{"\n"}'
324+
# and confirm no surviving pod predates the roll:
325+
kubectl get pods -n <namespace> -l component=opik-backend \
326+
-o custom-columns=NAME:.metadata.name,START:.status.startTime
327+
```
328+
329+
Three consequences to plan for:
330+
331+
- **One restart, not two, before the tail.** `traceColumnsNonNullable = true` (prereq 7) is another entry in the same
332+
backend config and the same ConfigMap — and step 1 of "The final cutover window" asks for both. Land them **together**
333+
so the fleet restarts once.
334+
- **Keep the chosen ceiling below the pod's termination grace period.** The revert is delivered by a *second* restart, and
335+
at that moment pods are holding inserts parked for up to the widened ceiling. The chart does not set
336+
`terminationGracePeriodSeconds`, so it is the Kubernetes default **30s** — comfortably above a ~10000ms ceiling, but a
337+
much larger ceiling would let `SIGKILL` land on parked inserts. Check the two numbers against each other before
338+
choosing a value.
339+
- **The restart itself costs ingestion capacity** (rolling-update `maxUnavailable`, plus any PodDisruptionBudget), so do
340+
it while there is slack — not between the final delta and the EXCHANGE.
341+
266342
### The final cutover window (the zero-loss invariant)
267343
268344
The buffer widening (prereq 6) is what makes the flip lossless, but the guarantee rests on a timing invariant worth
@@ -273,7 +349,9 @@ still land in the *old* `traces` in the gap between the last delta read and the
273349
run. The binding constraint is therefore **not** "replay < buffer window"; it is that the **gap between the final delta
274350
and the `EXCHANGE` completing must stay within the buffer hold**. So run the tail as tightly as possible:
275351
276-
1. Widen the buffer, and **roll out `traceColumnsNonNullable = true` to every backend instance** (see below).
352+
1. Widen the buffer, and **roll out `traceColumnsNonNullable = true` to every backend instance** (see below). Both are
353+
entries in the same backend config and the same ConfigMap, so land them **together** and let the single restart carry
354+
both — see ["Where the buffer bump lives"](#where-the-buffer-bump-lives-and-how-to-revert-it).
277355
2. Do the QA verify on an **earlier** pass (it can take minutes on a large table — do not let it be the last thing
278356
before the swap).
279357
3. Run a **final** `delta_replay.sh` as the last write-facing step.
@@ -728,7 +806,8 @@ server's major version, either way:
728806
`CLICKHOUSE_HOST`; for a ClickHouse on the host's own loopback, add `--network=host` via `CLICKHOUSE_CLIENT_DOCKER_OPTS`.
729807

730808
**The only manual actions are not SQL:** (1) raising/restoring the async-insert buffer ceiling
731-
(`databaseAnalytics.asyncInsertBusyTimeoutMaxMs`) around steps 2–3; (2) flipping
809+
(`databaseAnalytics.asyncInsertBusyTimeoutMaxMs`) around steps 2–3 — see
810+
["Where the buffer bump lives"](#where-the-buffer-bump-lives-and-how-to-revert-it); (2) flipping
732811
`databaseAnalyticsDataModel.traceColumnsNonNullable` to `true` in lockstep with the EXCHANGE (and back on rollback) —
733812
see "The final cutover window"; and (3) the go/no-go judgement between steps. All three are *backend config* / judgement changes (env + rolling
734813
restart, or a config push) that these DB-facing scripts cannot and should not make. They are deliberately operator-owned;
@@ -1079,6 +1158,12 @@ cheap (stage A); the bridge stays enabled so nothing is lost on a retry.
10791158
- [ ] **Async-insert ceiling confirmed** — raising `asyncInsertBusyTimeoutMaxMs` demonstrably widens the adaptive buffer
10801159
under load, not just the cap. `exchange_and_wrap.sh` enforces the acknowledgment via `--confirm-buffer-raised`, but
10811160
that is an assertion only — this checklist item is the actual "it took effect under load" verification.
1161+
- [ ] **The buffer-bump and revert changes are pre-written and reviewed** — the config entry raising
1162+
`ANALYTICS_DB_ASYNC_INSERT_BUSY_TIMEOUT_MAX_MS` (with `traceColumnsNonNullable = true` alongside it) and the revert
1163+
that *deletes* the key, both prepared before the window so each is a merge rather than an edit under pressure.
1164+
Confirm the chosen ceiling is below `terminationGracePeriodSeconds`, and that you know how a backend restart is
1165+
triggered on the target deployment and that it fits the schedule — see
1166+
["Where the buffer bump lives"](#where-the-buffer-bump-lives-and-how-to-revert-it).
10821167
- [ ] **Data Retention confirmed disabled** for the cutover window (`RETENTION_ENABLED=false`). Retention deletes bypass
10831168
the deletion bridge, so a sweep in the window would leak/resurrect across the swap; `exchange_and_wrap.sh` and
10841169
`rollback.sh` (stages B/C) enforce `--confirm-retention-paused`, but that is an assertion — this item is the real

0 commit comments

Comments
 (0)