[OPIK-7772] [BE] refactor: make the resolver the only way to name a trace mutation's table

[thiagohora]

Details

Stack 3/4 for OPIK-7772 — base #7952. Post-cutover traces is a Distributed table, which rejects mutations (code 36 / 48), so every trace mutation must target the traces_local shard once the wrap is live and traces while it is not. Routing was a two-branch <if(distributed_wrap)>traces_local<else>traces<endif> conditional repeated in each mutation template, plus one site that hand-rolled the same ternary in a StringBuilder — which made a correct new mutation a matter of remembering to copy the branch, and an incorrect one indistinguishable from a correct one at a glance. This funnels the decision into one method and guards it.

• TraceDAOImpl#tracesMutationTable() is now the single place the physical table name is decided. The mutation templates become topology-agnostic (DELETE FROM <traces_mutation_table>) and the resolver binds the resolved name.
• deleteForRetentionBounded appends the resolver's result instead of branching on the flag itself — it was the one site outside the resolver reading it.
• TraceMutationRoutingArchTest (ArchUnit) asserts the config flag is read in exactly one place and the routing decision made in exactly one place. These rules select the guarded method rather than its callers, so unlike TraceDeletionEventArchTest they deliberately omit allowEmptyShould — an empty selection would mean the method was renamed and the rule had silently stopped guarding.
• TraceMutationSqlRoutingTest covers what a call-graph rule cannot see: a mutation that hardcodes a table without consulting the flag at all. It reads the SQL constants reflectively and also scans the source's single-line string literals, which is precisely the form the previous StringBuilder site took.
• Reads and inserts are deliberately not routed through the resolver: they always go to traces, which is the Distributed wrapper post-cutover and the MergeTree before it, and is correct either way.
• No behaviour change intended — same SQL, same target table on both topologies.

Change checklist

• User facing
• Documentation update

Issues

• OPIK-7772

AI-WATERMARK

AI-WATERMARK: yes

• Tools: Claude Code
• Model(s): Claude Opus 5 (1M context)
• Scope: Authored the TraceDAO refactor (resolver, two table-name constants, template placeholders, the deleteForRetentionBounded call site) and both guard test classes. The ArchUnit formulation was corrected after the first attempt failed to compile — noCodeUnits() exposes no callMethod, so the rules use onlyBeCalled().byMethodsThat(...), which is also stricter.
• Human verification: All four guard rules were individually verified to fail on injected violations before being committed green (see Testing), and the existing behavioural suite for the wrapped topology passes unchanged. The author has not line-reviewed every Javadoc paragraph.

Testing

Environment: local, Docker 29.7.2 (linux/aarch64), Corretto 25.0.3, Maven 3.9.9, from apps/opik-backend.

cd apps/opik-backend
mvn test -Dtest=TracesDistributedWrapMutationTest                          # behavioural regression
mvn test -Dtest='TraceMutationRoutingArchTest,TraceMutationSqlRoutingTest'  # the new guards

Results: TracesDistributedWrapMutationTest 4/4 unchanged — this is the suite that drives the delete and retention paths against a real Distributed wrapper, so it is the load-bearing regression check for this refactor. New guards 4/4.

Scenarios validated:

• Regression on the wrapped topology — black-box create/list/delete through the public API plus the two retention DAO methods invoked directly, all against a real Distributed traces over traces_local. A delete that still hit the wrapper would surface as a 500.
• Each guard proven load-bearing — four violations injected one at a time, each confirmed to fail with a precise message:
  • a method calling TraceDAOImpl#tracesDistributedWrapEnabled() → routing rule fires, naming the method and line;
  • a method reading DatabaseAnalyticsDataModelConfig#tracesDistributedWrapEnabled() directly → flag rule fires;
  • a template changed to DELETE FROM traces → constants scan fires, naming the constant;
  • new StringBuilder("DELETE FROM traces_local WHERE (") → literal scan fires. This is the exact pre-refactor form, so the guard is confirmed to reject the code it replaced.
• All injected violations reverted and the suite re-run green before committing.

Not run: the full backend suite (CI runs it). No video — non-visual change.

Documentation

None in this PR. The runtime-routing rule is written up in stack 4/4's playbook, and TraceDAOImpl's Javadoc now points at the guard that enforces it.

[github-actions]

⏱️ pre-commit per-hook timing

Hook	Description	Result	Duration
☕ spotless — java backend	Format Java code	✅	3.91s
Total (1 ran)			3.91s

⏭️ 42 skipped (no matching files changed)

Hook	Description	Result
🐍 trim trailing whitespace — python sdk	Strip trailing whitespace	⏭️
🐍 fix end of files — python sdk	Ensure files end in a newline	⏭️
🐍 ruff — python sdk	Lint + autofix Python (ruff)	⏭️
🐍 ruff-format — python sdk	Format Python code (ruff)	⏭️
🐍 mypy — python sdk	Static type check	⏭️
🤖 trim trailing whitespace — optimizer	Strip trailing whitespace	⏭️
🤖 fix end of files — optimizer	Ensure files end in a newline	⏭️
🤖 check yaml — optimizer	Validate YAML syntax	⏭️
🤖 check json — optimizer	Validate JSON syntax	⏭️
🤖 check toml — optimizer	Validate TOML syntax	⏭️
🤖 check for added large files — optimizer	Block large files (>1MB)	⏭️
🔐 detect private key — optimizer	Block committed private keys	⏭️
🤖 check for merge conflicts — optimizer	Block merge-conflict markers	⏭️
🤖 check for case conflicts — optimizer	Block case-only name clashes	⏭️
🤖 pyupgrade — optimizer	Modernize Python syntax	⏭️
🤖 ruff — optimizer	Lint + autofix Python (ruff)	⏭️
🤖 ruff-format — optimizer	Format Python code (ruff)	⏭️
🤖 mypy — optimizer	Static type check	⏭️
📓 nbstripout — optimizer notebooks	Strip notebook output	⏭️
📝 markdownlint — optimizer	Lint Markdown	⏭️
🔤 codespell — optimizer	Fix common misspellings	⏭️
📊 radon cc — optimizer	Cyclomatic-complexity gate	⏭️
📊 radon raw — optimizer	Raw size metrics gate	⏭️
📊 xenon — optimizer	Fail on complexity thresholds	⏭️
📊 lizard — optimizer	Cyclomatic-complexity gate	⏭️
🧹 vulture — optimizer	Find dead code	⏭️
🛡️ trim trailing whitespace — guardrails	Strip trailing whitespace	⏭️
🛡️ fix end of files — guardrails	Ensure files end in a newline	⏭️
🛡️ ruff — guardrails	Lint + autofix Python (ruff)	⏭️
🛡️ ruff-format — guardrails	Format Python code (ruff)	⏭️
🛡️ mypy — guardrails	Static type check	⏭️
⚓ helm-docs	Regenerate Helm chart README	⏭️
block non-public FE plugins	Block non-public FE plugins	⏭️
🧪 pre-commit wrapper smoke tests	Self-test the wrapper scripts	⏭️
🧪 rebaseline script tests	Self-test the changelog re-baseline script	⏭️
🌐 eslint — frontend	Lint + autofix JS/TS	⏭️
🌐 typecheck — frontend	Whole-project tsc type check	⏭️
📘 eslint — typescript sdk	Lint + autofix JS/TS	⏭️
📘 typecheck — typescript sdk	Whole-project tsc type check	⏭️
⚙️ actionlint — github workflows	Lint GitHub Actions workflows	⏭️
🐳 hadolint — dockerfiles	Lint Dockerfiles	⏭️
🌈 zizmor — github workflows security	Security-scan GitHub Actions workflows	⏭️

[CometActions]

No test needed here.

Behaviour-preserving: <traces_mutation_table> resolves to exactly what the old <if(distributed_wrap)>traces_local<else>traces<endif> branch produced, and the bounded-retention StringBuilder becomes the same SQL via the standard getQueryItemPlaceHolder loop. The only user-facing mutation you touched, DELETE_BY_PROJECT_ID_TRACE_ID_PAIRS, is already exercised end-to-end by trace-explore/trace-delete.spec.ts, which deletes via the UI and via DELETE /v1/private/traces/delete and then asserts the rows are gone from the Logs table and that GET on each trace returns null — a mis-resolved or unset table name would fail both tests loudly. The retention paths aren't reachable on a fresh OSS install (scheduled job, age-based cutoffs) and traces_local only exists once the sharding wrap is on, so an e2e test couldn't reach the branch this PR is really about; TraceMutationRoutingArchTest and TraceMutationSqlRoutingTest are the right place for that and you've added them.

Run

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 21 Aug 19:17 UTC — nothing the verdict depends on changed.

[baz-reviewer]

Topology documentation contradicts assertion

MergeTree in the test name and documentation misstates the table topology, while the assertion requires ReplicatedReplacingMergeTree, so the test contract conflicts with the class description — should we rename the test and update the related comments/Javadoc to describe ReplicatedReplacingMergeTree consistently?

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/infrastructure/TracesUnwrappedMutationTest.java
around lines 227-232, rename `tracesIsAPlainMergeTreePreCutover` and update its comments
to consistently describe the asserted `ReplicatedReplacingMergeTree` topology. Also
correct the class-level documentation around lines 50-51 and any related wording so it
no longer calls `traces` a plain `MergeTree` and accurately distinguishes `traces` from
the absent `traces_local` table.

[github-actions]

Backend Tests - Integration Group 14

 31 files  + 3   31 suites  +3   5m 47s ⏱️ +5s
342 tests +11  340 ✅ +11  2 💤 ±0  0 ❌ ±0 
337 runs  +11  335 ✅ +11  2 💤 ±0  0 ❌ ±0 

Results for commit f084c37. ± Comparison against base commit 89832f9.

♻️ This comment has been updated with latest results.