From 2ddc5d5afd4cdb2e6a063180f47469a5298460fc Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Thu, 30 Apr 2026 16:28:42 -0400 Subject: [PATCH] =?UTF-8?q?fix(ui):=20remove=20intra-trace=20coalescing=20?= =?UTF-8?q?=E2=80=94=20preserve=20every=20stage=20for=20inspection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-Agent flagged that the trace-child coalesce was hiding legitimate repeated stage emissions: a request that retries an upstream call or emits the same stage twice for any reason would show only the first occurrence behind a `×N` badge, with subsequent attempts no longer selectable or inspectable. The original noise problem motivating intra-trace coalesce was the 149-stage `web-server:http` self-polling trace, but that's already hidden by default via the `hideDashboardInternals` filter. Users who opt in to see internals are deliberately debugging the dashboard and SHOULD see all 149 stages — collapsing them defeats the opt-in. Drop `coalesceChildren` entirely. Trace children render uncoalesced so retries, duplicated stages, and multi-attempt traces stay individually inspectable. Standalone-leaf coalesce stays — that's the actual user-facing dedup the PR was about. Tests: drop the 7 `coalesceChildren` cases; keep 16 covering `deriveStageHint` and `deriveTraceGroups`. Refs #1138, #1151 --- ui/src/components/logs/derive-trace-groups.ts | 58 +++---------------- ui/src/components/logs/logs-trace-row.tsx | 20 ++++--- .../logs-derive-trace-groups.test.ts | 48 --------------- 3 files changed, 22 insertions(+), 104 deletions(-) diff --git a/ui/src/components/logs/derive-trace-groups.ts b/ui/src/components/logs/derive-trace-groups.ts index 61ea9914..f0cfecef 100644 --- a/ui/src/components/logs/derive-trace-groups.ts +++ b/ui/src/components/logs/derive-trace-groups.ts @@ -18,11 +18,16 @@ export interface LeafItem { export type DerivedItem = LeafItem | TraceGroup; /** - * Tuple key for coalescing. Includes `message` so two adjacent logs that - * share event/module/level but report different content stay distinct + * Tuple key for coalescing standalone leaves. Includes `message` and + * `source` so two adjacent logs that share event/module/level but report + * different content (or come from a different service) stay distinct * (e.g. `User logged in: alice` and `User logged in: bob`). Excludes * `latencyMs` and `metadata` because those drift per request even on * truly redundant polls — including them would prevent any coalescing. + * + * NB: this only applies to *leaves* (entries without `requestId`). Trace + * children render uncoalesced so retries and duplicated-stage emissions + * stay individually inspectable. */ function coalesceKey(entry: LogsEntry): string { return [ @@ -35,52 +40,6 @@ function coalesceKey(entry: LogsEntry): string { ].join(' '); } -/** - * Coalesce identical consecutive children inside a single trace. - * - * Key tuple includes `message` so two children that share - * event/stage/level/module but report different content stay visible as - * separate rows. Includes `source` so adjacent rows from different - * services with otherwise-identical fields stay distinct (a request can - * fan out across multiple sources participating in the same trace). - * Excludes `latencyMs` — it drifts per request and would defeat the - * dedup on truly redundant polls. - * - * Exported so it can be unit-tested independently from the React row. - */ -export function coalesceChildren(children: LogsEntry[]): Array<{ head: LogsEntry; count: number }> { - if (children.length === 0) return []; - const out: Array<{ head: LogsEntry; count: number }> = []; - let head: LogsEntry | null = null; - let count = 0; - let key = ''; - const flush = () => { - if (head) out.push({ head, count }); - head = null; - count = 0; - }; - for (const child of children) { - const k = [ - child.event ?? '', - child.message ?? '', - child.stage ?? '', - child.level, - child.module ?? '', - child.source ?? '', - ].join(' '); - if (head && k === key) { - count += 1; - } else { - flush(); - head = child; - count = 1; - key = k; - } - } - flush(); - return out; -} - /** * For an entry without an explicit `stage` field, derive a short chip label * from the event name so the trace timeline still renders meaningful badges @@ -108,7 +67,8 @@ export function deriveStageHint(entry: LogsEntry): string | undefined { * Trace groups gather all children sharing a requestId regardless of * interleaving; they're sorted by `ts asc` before display, with the * group's positional `ts` set to the oldest child so it slots correctly - * in the reverse-chronological display sort below. + * in the reverse-chronological display sort below. Children are NOT + * coalesced — every stage is preserved for individual inspection. */ export function deriveTraceGroups(entries: LogsEntry[]): DerivedItem[] { const items: DerivedItem[] = []; diff --git a/ui/src/components/logs/logs-trace-row.tsx b/ui/src/components/logs/logs-trace-row.tsx index 4b8d70f9..a6f14bf9 100644 --- a/ui/src/components/logs/logs-trace-row.tsx +++ b/ui/src/components/logs/logs-trace-row.tsx @@ -2,7 +2,7 @@ import { memo } from 'react'; import { ChevronDown, ChevronRight, GitBranch } from 'lucide-react'; import type { LogsEntry, LogsLevel } from '@/lib/api-client'; import { cn } from '@/lib/utils'; -import { coalesceChildren, deriveStageHint } from './derive-trace-groups'; +import { deriveStageHint } from './derive-trace-groups'; import { LogLevelBadge } from './log-level-badge'; import { LogsRow } from './logs-row'; import { FOCUS_RING, MONO_NUMERIC, ROW_DENSITY, ROW_INTERACTIVE, type RowDensity } from './tokens'; @@ -110,18 +110,24 @@ function LogsTraceRowImpl({ {group.requestId.slice(-8)} + {/* Children render unconditionally — every stage is preserved so + retries, duplicated stage logs, and multi-attempt traces stay + individually inspectable. The user-facing noise problem this + PR addresses lives at the standalone-leaf level (dashboard + self-polling spam) where leaf-coalesce in deriveTraceGroups + handles it; opting in to see internals is a deliberate debug + choice, so collapsing trace stages there would hide intent. */} {isExpanded - ? coalesceChildren(group.children).map((run) => ( + ? group.children.map((child) => ( 1 ? run.count : undefined} + stageHint={deriveStageHint(child)} /> )) : null} diff --git a/ui/tests/unit/ui/components/logs-derive-trace-groups.test.ts b/ui/tests/unit/ui/components/logs-derive-trace-groups.test.ts index 03560ac5..41a726e9 100644 --- a/ui/tests/unit/ui/components/logs-derive-trace-groups.test.ts +++ b/ui/tests/unit/ui/components/logs-derive-trace-groups.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it } from 'vitest'; import type { LogsEntry } from '@/lib/api-client'; import { - coalesceChildren, deriveStageHint, deriveTraceGroups, type LeafItem, @@ -51,53 +50,6 @@ describe('deriveStageHint', () => { }); }); -describe('coalesceChildren', () => { - it('returns empty array for empty input', () => { - expect(coalesceChildren([])).toEqual([]); - }); - - it('passes through a single child as count=1', () => { - const e = entry({ id: '1', timestamp: 't', event: 'a.b' }); - expect(coalesceChildren([e])).toEqual([{ head: e, count: 1 }]); - }); - - it('coalesces two adjacent identical children', () => { - const a = entry({ id: '1', timestamp: 't1', event: 'a.b', message: 'm' }); - const b = entry({ id: '2', timestamp: 't2', event: 'a.b', message: 'm' }); - const result = coalesceChildren([a, b]); - expect(result).toHaveLength(1); - expect(result[0]?.count).toBe(2); - expect(result[0]?.head).toBe(a); - }); - - it('keeps children with different messages distinct (round-1 fix)', () => { - const a = entry({ id: '1', timestamp: 't1', event: 'auth.ok', message: 'alice' }); - const b = entry({ id: '2', timestamp: 't2', event: 'auth.ok', message: 'bob' }); - expect(coalesceChildren([a, b])).toHaveLength(2); - }); - - it('keeps children with different sources distinct (round-2 fix)', () => { - const a = entry({ id: '1', timestamp: 't1', source: 'svc-a', event: 'e' }); - const b = entry({ id: '2', timestamp: 't2', source: 'svc-b', event: 'e' }); - expect(coalesceChildren([a, b])).toHaveLength(2); - }); - - it('keeps children with different stages distinct', () => { - const a = entry({ id: '1', timestamp: 't1', stage: 'route', event: 'e' }); - const b = entry({ id: '2', timestamp: 't2', stage: 'dispatch', event: 'e' }); - expect(coalesceChildren([a, b])).toHaveLength(2); - }); - - it('only collapses adjacent duplicates — interleaving breaks the run', () => { - const a = entry({ id: '1', timestamp: 't1', event: 'a' }); - const b = entry({ id: '2', timestamp: 't2', event: 'b' }); - const c = entry({ id: '3', timestamp: 't3', event: 'a' }); - const result = coalesceChildren([a, b, c]); - // a, b, a — three distinct runs of count 1 each - expect(result.map((r) => r.count)).toEqual([1, 1, 1]); - }); -}); - describe('deriveTraceGroups', () => { it('returns empty array for empty input', () => { expect(deriveTraceGroups([])).toEqual([]);