[OPIK-7686] [DOCS] Document where the async-insert cutover bump lives - #7916
Open
thiagohora wants to merge 1 commit into
Open
[OPIK-7686] [DOCS] Document where the async-insert cutover bump lives#7916thiagohora wants to merge 1 commit into
thiagohora wants to merge 1 commit into
Conversation
Contributor
⏱️ pre-commit per-hook timingNo linted files changed — nothing to run. ⏭️ 43 skipped (no matching files changed)
|
Collaborator
|
No test needed here. No product surface in this PR — only docs, CI, tests, lockfiles or other non-product files. 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 20 Aug 11:58 UTC. |
thiagohora
force-pushed
the
thiagoh/OPIK-7686-async-insert-bump-location
branch
from
August 19, 2026 15:55
c19f904 to
3191a31
Compare
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>
thiagohora
force-pushed
the
thiagoh/OPIK-7686-async-insert-bump-location
branch
from
August 20, 2026 11:57
3191a31 to
4ddc962
Compare
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
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
Closes the follow-up @andrescrz raised on #7675:
async_insert_busy_timeout_max_mshas to be increased during the traces cutover, and it was unsettled whether that belongs in the chart values or as a temporary env var on the deployed chart. Decision: the temporary env var — the option you leaned towards. No chart change, so the threeANALYTICS_DB_ASYNC_INSERT_*knobs stay out ofvalues.yaml.Docs-only, and scoped to the operator runbook rather than any published doc page: the bump is temporary, so it should not become official self-host documentation. One new section, "Where the buffer bump lives (and how to revert it)", plus cross-references from the four places that previously said "config, see below" without ever saying what to set.
The decision, and its rationale
component.backend.envis a free-form map rendered straight into the backend ConfigMap.queryParametersalone" (DatabaseAnalyticsFactory), so deleting the key restores the baked-inasync_insert_busy_timeout_max_ms=250. There is no "set it back to 250" edit and no pinned value that can later drift from theconfig.ymldefault.The section also corrects the record: the reason first given on #7675 for excluding these (that empty ConfigMap values "would not be inert") was wrong —
config.ymlalready ships${ANALYTICS_DB_ASYNC_INSERT_*:-}as the default, so the empty case is the normal path everywhere, and@Min(1)does not fire on a nullInteger. Exposing them with empty defaults would have been safe. The exclusion now rests on reversibility and scope, not safety. Happy to add all three with empty defaults instead if you'd rather have them discoverable in the chart.And the trap is written down explicitly: never bump it by editing
ANALYTICS_DB_QUERY_PARAMETERS— that means re-pasting the whole tuning string and risks silently dropping one of the other settings.Two operational facts the runbook did not state
envFrom.configMapRefunder Helm), injected at container start only — 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: some deployments run a ConfigMap watcher that rolls the workload, others need an explicitkubectl rollout restart. The runbook now 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 — with the commands to do so. Getting this wrong is silent: you would enter the delta and EXCHANGE with the old 250ms ceiling still live.terminationGracePeriodSeconds, so it is the Kubernetes default 30s — comfortably above ~10000ms, but a much larger ceiling would letSIGKILLland on parked inserts. Nothing previously connected those two numbers.A planning consequence falls out of (1):
traceColumnsNonNullable(prereq 7) is another entry in the same backend config and the same ConfigMap, and "The final cutover window" step 1 asks for both — so they should land together and ride a single restart. That note is now in both places.Scope note
The section is deliberately deployment-agnostic — it names the key, the value, the revert and the constraints, not any particular environment's file paths. Comet's own deployment specifics are recorded internally instead. If these knobs later earn a spot in the published self-host docs, the self-hosted instructions can follow then; a temporary cutover-only bump does not warrant one now.
Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Docs-only — no code, no chart, no SQL touched, so nothing to run. The claims were verified rather than tested:
envFrom.configMapRef: opik-backendconfirmed as the backend's config path in the chart, and read back off a live deployment.terminationGracePeriodSecondsconfirmed absent from the chart (hence the Kubernetes default 30s) and read back as30on a live deployment.podAnnotationsdefault — which is why the restart trigger is documented as deployment-specific rather than automatic.ANALYTICS_DB_ASYNC_INSERT_*keys and noANALYTICS_DB_QUERY_PARAMETERS, confirming deployments inherit theconfig.yml250ms default today.kubectl get pods -l component=opik-backendcommand quoted in the section was run as written and returns the expected rows.Documentation
This PR is the documentation, and only the operator runbook:
apps/opik-backend/data-migrations/traces-local-v2-cutover/README.md. No published doc page is touched, by design — see the scope note above.