-
[OPIK-6884] [BE] perf: full-scan audit Slice-1 remediations (R4, R6, R7, R8) (#7404)
frostbyte_neo 发布于
2026-07-09 11:23:49 +00:00 - [OPIK-6884] [BE] perf: scope experiment project-mapping traces scan to 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.
- perf(traces): group deletes by project so the delete cascade prunes on (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.
- perf(retention): bound the velocity-estimate scan below by the catch-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).- revert(experiments): drop no-op workspace_id predicate on project-mapping 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.- perf(experiments): prune project-mapping traces scan by referenced trace 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).
- fix(traces): align delete cascade with deletion-events bridge + address 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.
- [OPIK-6884] address review nits: javadoc + drop redundant @NonNull
- 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
Co-authored-by: Claude Opus 4.8 (1M context) noreply@anthropic.com
下载附件