Skip to content

chore(observability): tear down the #1280 Tempo eval sandbox - #2073

Merged
mashkovd merged 3 commits into
mainfrom
chore/1280-teardown-eval-sandbox
Oct 22, 2026
Merged

mashkovd merged 3 commits into
mainfrom
chore/1280-teardown-eval-sandbox

Conversation

@mashkovd

Copy link
Copy Markdown
Member

Tears down the #1280 observability-eval sandbox in one commit, per runbook §9 with Tempo selected.

  • otelCollector.eval.enabled: false, the tempo candidate enabled: false, teardownAfter: "". validate-eval-teardown-date.py would otherwise turn main red on 2026-10-24.
  • The producers' export is off: otel.enabled: false on the three mctl-agents-worker* services and on admins-mctl-agent. Nothing exports into a Collector with no trace backend, as both ADR producer amendments require.
  • vm-rules/tempo-eval-alerts.yaml stays. It is silent without the sandbox, and the adoption PR renames it.
  • tests/test_otel_collector_backends_render.py checks the closed default: no otlp/eval- exporter, traces go to [debug] only, no tempo-eval datasource, no observability-eval Namespace or Application, and no Loki derived field. The Tempo wiring checks now render with an eval-on overlay. Mutation check: re-enabling eval in the committed default makes 6 checks fail.

The R2 bucket tempo-traces, its token and Vault secret/platform/r2-tempo are kept, because the permanent Tempo reuses them. After the Argo CD prune comes the manual step: delete the "Tempo (eval)" Grafana datasource, then verify with runbook §10.

Ordering. This PR is option A of the adoption plan (#2072 draft): tracing is off until adoption lands. Merge it after the verdict PR #2071 and by 2026-10-24, unless the owner picks option B (adoption first).

Refs #1280

🤖 Generated with Claude Code

The #1280 soak closed on 2026-10-22 with the verdict ADOPT TEMPO /
mctl-native only. The sandbox was declared disposable, and
validate-eval-teardown-date.py turns main red after 2026-10-24, so it
goes now in the one commit runbook section 9 prescribes, instead of
being promoted in place.

Because Tempo was selected, the TempoEval alerts and the R2 bucket stay
(step 9.4 and the bucket note). The permanent deployment is a separate,
owner-approved change. The real producers' export is switched off as both
ADR producer amendments require, so nothing keeps exporting into a
Collector that has no trace backend.

The render test now asserts the closed state (no eval exporter,
datasource, namespace, Application or Loki derived field). The Tempo
wiring checks still run against an explicit eval-on overlay.

Refs #1280
@claude

claude Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 4m 5s —— View job


Review complete ✅

  • Gather context (PR description, changed files)
  • Review diff via git diff 9c5a124f1c96ccf6de89de10485d3d2be64fb5cc...9a9575015cb953c5956fca6dfa94b9493598deff (scoped to this commit's own 6 files — the raw two-dot diff against the stated base SHA also showed unrelated drift from other merged PRs, e.g. agents-state/mctl-alice/..., agent-worker-preview, mctl-alice, mctl-telegram-preview tag bumps; confirmed via git show --stat 9a9575015 that none of those are part of this commit, so they're excluded from review)
  • Check Helm values correctness (bootstrap/mctl-agent.yaml, bootstrap/values.yaml)
  • Check worker services' otel.enabled: false changes
  • Check test file changes
  • Post findings / verdict

Traced the full gating chain for otelCollector.eval.enabled: false + candidates[0].enabled: false:

  • eval-namespace.yaml, eval-candidates.yaml (Application), otel-collector.yaml's fan-out block, and tempo-datasource.yaml are all wrapped in {{- if .Values.otelCollector.eval.enabled }} — namespace, NetworkPolicies, ResourceQuota/LimitRange, the ArgoCD otel-eval-tempo Application, the collector's otlp/eval-tempo exporter, and the "Tempo (eval)" Grafana datasource ConfigMap all correctly stop rendering.
  • loki-datasource.yaml's derivedFields block is correctly gated on the same eval.enabled && candidate.enabled combination, so the dangling tempo-eval datasource UID reference also disappears.
  • teardownAfter: "" with eval.enabled: false is accepted by scripts/validate-eval-teardown-date.py (check_values: "sandbox closed, teardownAfter is free to be anything, including """), so this avoids the 2026-10-24 CI-red deadline as intended.
  • The three worker services' otel.enabled: false is consumed by base-service._helpers.tpl's base-service.env, which gates OTEL_EXPORTER_OTLP_ENDPOINT/OTEL_RESOURCE_ATTRIBUTES on that exact flag — no dangling env pointing at a torn-down Collector fan-out. MCTL_TRACE_WORKFLOW_TYPES staying set is just app config, independent of the OTEL SDK env vars, and pre-exists this PR.
  • The Application's existing finalizers: [resources-finalizer.argocd.argoproj.io] on otel-eval-tempo means ArgoCD's prune will cascade-delete the Tempo workload itself once the Application manifest disappears, consistent with the PR body's "After the Argo CD prune comes the manual step."
  • vm-rules/tempo-eval-alerts.yaml is untouched and still present, matching the stated intent to keep it.
  • Test file: the new shipped_docs/_loki_ds_raw checks correctly assert the closed default, and the "Tempo round as it ran" block correctly reconstructs the eval-on scenario via tempo_candidate (pulled from the real committed candidate list, enabled overridden to True) rather than hand-duplicating the spec — good, avoids drift between the real values and the test fixture.

P3 (nice to have, does not block merge): the module docstring (lines ~39-44) and the T1 section comment (lines ~109-112) still describe the old "sandbox OPEN, ships one candidate" committed-default behavior and now contradict the code a few hundred lines below, which asserts the opposite (closed default). Both predate this diff and sit outside the changed hunk, so no inline comment was filed, but worth a follow-up touch-up for a reader skimming the top of the file. No functional impact — the test logic itself is correct.

No P1/P2 findings. Verdict submitted: APPROVE.
· Branch

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit 9a95750.

No significant issues found.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No P1/P2 findings (1 P3: stale module docstring/T1 comment describing the old sandbox-open committed default, outside the diff hunk, no functional impact). Gating chain traced end-to-end (namespace, Application, collector exporter, Tempo + Loki datasources, teardown-date validator, worker otel env) and all correctly closed/off. Good to merge.

@github-actions

Copy link
Copy Markdown
Contributor

Skipping — this commit (52539a1) already has a Claude review verdict. Push a new commit, or check the run log if you expected a fresh pass and the previous run actually failed.

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit 52539a1.

No significant issues found.

@github-actions

Copy link
Copy Markdown
Contributor

Skipping — this commit (d4f1c87) already has a Claude review verdict. Push a new commit, or check the run log if you expected a fresh pass and the previous run actually failed.

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit d4f1c87.

No significant issues found.

@mashkovd
mashkovd merged commit d4add90 into main Oct 22, 2026
23 checks passed
@mashkovd
mashkovd deleted the chore/1280-teardown-eval-sandbox branch October 22, 2026 18:47
mashkovd added a commit that referenced this pull request Oct 22, 2026
The #1280 verdict (ADR 0001) adopted Tempo, mctl-native only, as a
separate deployment rather than the sandbox promoted in place. The owner
approved the plan on 2026-10-22 (Option A, teardown first; #2073 merged).

Tempo 2.10.8 (chart 2.4.0) runs in its own `tracing` namespace, reuses the
tempo-traces R2 bucket, keeps 336h and is read-bounded (api_timeout,
querier and frontend concurrency) after the soak's unranged read pinned the
pod. root-app renders only the Namespace and the Application; all
namespaced objects come from the Application's manifestsPath, which avoids
the project/namespace sync deadlock of gitops#1925.

The collector exports to it over otlp/backend; Grafana gets datasource
"Tempo" (uid tempo) and deletes the read-only "Tempo (eval)" leftover via
provisioning, since the API refuses. Only the DevLoop workers and
admins-mctl-agent export.

The emptyDir WAL is an owner-accepted standing risk. The unexplained
partial trace-by-id read (5,092 of 6,930 spans) is a documented limitation:
no completeness claims or counts from Tempo until explained
(docs/runbooks/tempo.md).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant