[OPIK-6884] [BE] perf: full-scan audit Slice-1 remediations (R4, R6, R7, R8)#7404
Merged
thiagohora merged 8 commits intoJul 9, 2026
Merged
Conversation
…o workspace and drop dead span-cascade delete Closes Bucket-1 rows 17 (R6) and 19 (R8) of the OPIK-6883 full-scan audit: - ExperimentDAO.COMPUTE_EXPERIMENT_PROJECT_MAPPING: add an explicit 't.workspace_id = :workspace_id' predicate. The traces side previously carried tenancy only through the join key (ei.workspace_id = t.workspace_id), which does not let ClickHouse prune the traces table by its sorting-key prefix. The added predicate is provably equivalent given the join condition but restores primary-key pruning ahead of traces hot/cold tiering. - FeedbackScoreDAO: remove the dead DELETE_SPANS_CASCADE_FEEDBACK_SCORE query and its only user cascadeSpanDelete (zero callers). It also carried the degraded 'project_id dropped when null' filter, so removing it clears an audit finding outright.
…n (workspace_id, project_id) Closes Bucket-1 row 14 (R4) of the OPIK-6883 full-scan audit. The trace delete paths accept a nullable project_id (delete-by-id passes null; a batch may span projects), so the delete and its async span/feedback cascade dropped the project_id predicate and could no longer prune on the (workspace_id, project_id) sorting-key prefix - a workspace-wide scan once traces are tiered. TraceService.delete now, when no project is provided, resolves each trace's owning project and deletes per project group with a concrete project_id. The project flows through the existing TracesDeleted event to the span/feedback cascade unchanged, so no event-contract or downstream-service change is needed. Trace ids with no resolvable project (row already gone) fall back to a workspace-scoped delete to still clean up orphan child rows. The resolution itself is tiering-safe: getProjectIdsByTraceIdsBounded mirrors SELECT_PROJECT_ID_FROM_TRACE, bounding toMonday(id_at) by the id set's own min/max. id_at is MATERIALIZED as UUIDv7ToDateTime(toUUID(id)) (migration 000091), so the bound is exact (never drops a valid id) yet keeps the lookup on the ids' own weekly partitions instead of scanning all cold history. Adds a cross-project delete test covering the per-project grouping path.
…up service-start date Closes Bucket-1 row 17 (R7) of the OPIK-6883 full-scan audit. ESTIMATE_VELOCITY_FOR_RETENTION was bounded above (trace_id < cutoff) but had no lower bound, so it aggregated over all pre-cutoff history - an unbounded scan that pulls the whole cold tier once spans are tiered. The retention catch-up already treats the service-start date as its floor: the scouting fallback (scoutFirstDataCursor) scans month-by-month starting at the service-start cursor and never looks earlier. Only the primary estimate path was inconsistent, scanning back to the actual oldest span. Passing the service-start cursor as a lower bound (trace_id >= :lower_bound, mirroring DELETE_FOR_RETENTION) makes the two paths consistent and keeps the scan within [service_start, cutoff) instead of all history. Validated against real ClickHouse via RetentionPolicyServiceTest$CatchUpJob (bounded query executes and small-workspace catch-up still deletes old data).
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 40 skipped (no matching files changed)
|
Contributor
Contributor
…ping traces scan (R6) Verified on prod (ClickHouse 25.8.16 Altinity) with EXPLAIN indexes=1 on the full COMPUTE_EXPERIMENT_PROJECT_MAPPING for a large workspace: the analyzer already propagates the workspace_id constant across the join equality (ei.workspace_id = t.workspace_id AND ei.workspace_id = const) onto the traces primary key. The traces read prunes to the same 97,992 granules with or without an explicit t.workspace_id predicate; the added predicate only appears as a duplicated 'workspace_id in [...]' condition and changes nothing. The audit's R6 premise predates the 25.8 upgrade. Reverting the redundant predicate; R8 (dead DELETE_SPANS_CASCADE_FEEDBACK_SCORE removal) stays.
thiagohora
changed the title
…ace ids COMPUTE_EXPERIMENT_PROJECT_MAPPING joined experiment_items to traces with only a workspace_id bound on traces, so the ConcurrentHashJoin read the entire workspace trace slice into the hash table. Adding 'AND t.id IN (SELECT trace_id FROM experiment_items WHERE workspace_id = :workspace_id)' gives traces an 'id' primary-key condition (the set is implied by the join ei.trace_id = t.id, so the result is unchanged) and bounds the read to the referenced trace ids. This is the effective replacement for the reverted R6 (row 16): the workspace_id predicate the audit suggested is already applied by the CH 25.8 analyzer, whereas this bounds the actual cost - the unpruned trace-id scan. Validated by ExperimentProjectMigrationJobTest (mapping unchanged).
thiagohora
changed the title
…l-scan-audit-slice-1-remediations
…ss review Merged main brought the deletion-events bridge, whose tests exercise the same delete path R4 changed. Fixes after merge: - TraceDeletionEventTest: by-id / unscoped deletes now resolve the trace's owning project (R4), so the bridge captures the resolved project_id instead of null; update the two expectations accordingly. - TracesResourceTest (TraceComment cascade): the span was created with a random project, distinct from its trace. A span always shares its trace's project, so create it in the trace's project - otherwise the (now project-scoped) delete cascade can't reach it. Fixes the two ConditionTimeout errors. Review feedback (baz-reviewer): - TraceDAO: extract collectTraceIdToProjectId() shared by getProjectIdsByTraceIds and getProjectIdsByTraceIdsBounded (dedup the reactive pipeline). - TraceService: move the count to the end of the resolve-projects log message.
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Contributor
andrescrz
previously approved these changes
Jul 9, 2026
- Convert the SELECT_PROJECT_IDS_BY_TRACE_IDS_BOUNDED explanatory comment to javadoc. - Drop @nonnull on getProjectIdsByTraceIdsBounded(Set) — emptiness/null is already covered by the checkArgument(CollectionUtils.isNotEmpty(...)) guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
andrescrz
approved these changes
Jul 9, 2026
thiagohora
deleted the
thiagoh/OPIK-6884-full-scan-audit-slice-1-remediations
branch
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
2 participants
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 self-contained ("mechanical") Bucket-1 remediations from the OPIK-6883 full-scan query audit, hardening
traces/spansquery paths ahead of the planned weekly time-partitioning + hot/cold tiering. This is one increment of the rolling OPIK-6884 story; the structural items (R1/R2/R3) remain as separate follow-ups and are not required for Slice-1 close.traces/spansdeletes (TraceService,TraceDAO): the delete cascade accepted a nullableproject_id(delete-by-id passes null; a batch may span projects), so it dropped theproject_idpredicate and could no longer prune on the(workspace_id, project_id)sorting-key prefix.TraceService.deletenow resolves each trace's owning project and deletes per project group with a concreteproject_id, which flows through the existingTracesDeletedevent to the span/feedback cascade unchanged (no event-contract change). Resolution uses a new partition-bounded lookup (getProjectIdsByTraceIdsBounded) that boundstoMonday(id_at)by the id set's own min/max — exact becauseid_atisMATERIALIZED UUIDv7ToDateTime(toUUID(id))(migration 000091). ProdEXPLAINconfirms this prunes both the trace delete (58 granules) and, crucially, the span cascade (SELECT_SPAN_IDS_BY_TRACE_ID: 4,787 → 164 granules, ~29×, sinceproject_idis the spans' 2nd PK column).project_idon the delete path, so by-id / unscoped deletes now record the resolved project instead ofnull—TraceDeletionEventTest's two unscoped expectations were updated accordingly (needs a nod from the bridge owner). This relies on the invariant that a span always shares its trace's project; twoTracesResourceTestcascade tests that created a span in a different project than its trace were corrected to use the trace's project.SpanDAO.ESTIMATE_VELOCITY_FOR_RETENTION): added a lower bound (trace_id >= :lower_bound) set to the catch-up service-start cursor, so the estimate scans[service_start, cutoff)instead of all pre-cutoff history. Makes the primary estimate path consistent with the scouting fallback, which already floors at the service-start date.ExperimentDAO.COMPUTE_EXPERIMENT_PROJECT_MAPPING): the query joinedexperiment_itemstotraceswith only aworkspace_idbound ontraces, so theConcurrentHashJoinread the entire workspace trace slice into the hash table. AddedAND t.id IN (SELECT trace_id FROM experiment_items WHERE workspace_id = :workspace_id), which givestracesanidprimary-key condition (the set is implied by the joinei.trace_id = t.id, so the mapping result is unchanged) and bounds the read to the referenced trace ids. Verified on prod (ClickHouse 25.8.16 Altinity, large workspace) withEXPLAIN indexes=1/EXPLAIN ESTIMATE: thetracesread drops from ~28M rows / 97,997 granules (over the read cap) to ~1.25M rows / 1,896 granules (~52× fewer granules). Note: the audit's original R6 suggestion (an explicitt.workspace_idpredicate) is a no-op on CH 25.8 — the analyzer already propagates that constant across the join — so the real lever was bounding the trace-id scan, not the tenancy filter.FeedbackScoreDAO): removed the unreachableDELETE_SPANS_CASCADE_FEEDBACK_SCOREquery and its only (uncalled) user, which also carried the degraded null-project_idfilter.Note: the audit's original R5 ("short-circuit empty target-project set to an empty result") was found to be a correctness bug (would drop legitimate rows and break
projectDeletedfiltering) and has been retracted/reclassified to R10 in OPIK-6883.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Local, offline (
-o), backend Testcontainers (real MySQL/ClickHouse):mvn spotless:check— cleanmvn test -Dtest='TracesResourceTest$DeleteTrace,TracesResourceTest$DeleteTraces'— R4 single-trace, batch, and cascade; includes a newdeleteTracesAcrossProjectsWithoutProjectIdtest covering the cross-project grouping path (11 tests, pass)mvn test -Dtest=RetentionEstimationServiceTest— R7 unit path with updated mocks (6 tests, pass)mvn test -Dtest='RetentionPolicyServiceTest$CatchUpJob'— R7 bounded query against real ClickHouse; small-workspace catch-up still deletes old data (2 tests, pass)mvn test -Dtest=ExperimentProjectMigrationJobTest— R6: mapping result unchanged with the addedINpredicate (real ClickHouse, 1 test, pass)Merged
origin/main(24 commits) to run against the same code as CI; the deletion-events bridge tests + the twoTracesResourceTestcascade tests now pass locally alongside the delete suites.Prod validation (read-only
agentro, plan-onlyEXPLAIN— no execution/scan; CH 25.8.16, large workspace):COMPUTE_EXPERIMENT_PROJECT_MAPPING: thet.workspace_idpredicate the audit suggested is a no-op (analyzer already propagates it); thet.id IN (...)predicate bounds thetracesread from ~28M rows / 97,997 granules to ~1.25M rows / 1,896 granules (~52×).SELECT_SPAN_IDS_BY_TRACE_ID: resolvedproject_id→ 4,787 → 164 granules (~29×); traceDELETE_BY_ID→ 58 granules; the bounded project lookup prunes viaid INtoday and via thetoMonday(id_at)range once traces are partitioned.ESTIMATE_VELOCITY_FOR_RETENTION: the addedtrace_id >= :lower_boundis applied as atrace_idPK condition, bounding the scan to[service_start, cutoff).Not run: full repo regression suite (scoped to the affected areas). Structural items R1/R2/R3 are out of scope for this PR.
Documentation
No user-facing or SDK changes; internal query-path hardening only.