fix(tracing): key root runs by user message id#74
Conversation
|
Warning Review limit reached
More reviews will be available in 34 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR adds run-level span tracking to session telemetry, centralizes trace-context resolution across session, tool, and LLM spans, updates plugin wiring to carry the new context maps, and extends tests and helpers for the revised parenting and lifecycle behavior. ChangesRun-aware tracing context
Sequence Diagram(s)sequenceDiagram
participant chatMessage as chat.message
participant handleRunStarted
participant handleSessionCreated
participant resolveSessionTraceContext
participant ctxRunSpans as ctx.runSpans
participant ctxSessionRunRoots as ctx.sessionRunRoots
participant handleSessionIdle
participant handleSessionError
chatMessage->>handleRunStarted: start run span when sessionSpan is missing
handleRunStarted->>ctxRunSpans: store run span and context
handleRunStarted->>ctxSessionRunRoots: record session root mapping
handleSessionCreated->>resolveSessionTraceContext: resolve parent context for the session span
resolveSessionTraceContext->>ctxRunSpans: read run span or run span context
resolveSessionTraceContext->>ctxSessionRunRoots: look up the run root
resolveSessionTraceContext-->>handleSessionCreated: parent context
handleSessionIdle->>ctxRunSpans: look up the run span and end it with OK
handleSessionError->>ctxRunSpans: look up the run span and end it with ERROR
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/handlers/spans.test.ts (1)
222-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the fallback span is actually root-parented.
This test name promises root fallback, but it only verifies that one span is created. Add parent assertions so regressions in
resolveSessionTraceContext()are caught.🧪 Proposed assertion
expect(() => handleSessionCreated(makeSessionCreated("ses_child", 1000, "ses_missing_parent"), ctx)).not.toThrow() expect(tracer.spans).toHaveLength(1) + expect(tracer.spans[0]!.parentSpan).toBeUndefined() + expect(tracer.spans[0]!.parentSpanContext).toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/handlers/spans.test.ts` around lines 222 - 226, The test around handleSessionCreated currently only checks that a span is created, but it does not verify that the fallback span is actually root-parented. Update this test to assert the created span has no parent / is rooted when resolveSessionTraceContext() cannot find the parent run, using the existing makeCtx(), handleSessionCreated(), and tracer.spans helpers to catch regressions in parent resolution.tests/helpers.ts (1)
84-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake spy span contexts stable and unique.
The new
parentSpanContextassertions are weakened because everySpySpan.spanContext()returns the same hard-coded span ID. A resolver using the wrong stored ended-span context can still pass these tests; allocate a stable unique context per spy span instead.🧪 Proposed test-helper refinement
function makeSpan( name: string, startTime?: number, parentSpan?: SpySpan, parentSpanContext?: SpanContext, + ownSpanContext?: SpanContext, ): SpySpan { + const context = ownSpanContext ?? { + traceId: "00000000000000000000000000000001", + spanId: "0000000000000001", + traceFlags: 1, + } const span: SpySpan = { name, @@ - spanContext() { return { traceId: "00000000000000000000000000000001", spanId: "0000000000000001", traceFlags: 1 } }, + spanContext() { return context }, @@ export function makeTracer(): SpyTracer { + let nextSpanID = 1 const tracer: SpyTracer = { spans: [], startSpan(name, options, ctx) { const parentFromCtx = ctx ? trace.getSpan(ctx) as SpySpan | undefined : undefined const parentSpanContext = ctx ? trace.getSpanContext(ctx) ?? undefined : undefined + const ownSpanContext: SpanContext = { + traceId: parentSpanContext?.traceId ?? "00000000000000000000000000000001", + spanId: (nextSpanID++).toString(16).padStart(16, "0"), + traceFlags: parentSpanContext?.traceFlags ?? 1, + ...(parentSpanContext?.traceState ? { traceState: parentSpanContext.traceState } : {}), + } const span = makeSpan( name, typeof options?.startTime === "number" ? options.startTime : undefined, parentFromCtx, parentSpanContext, + ownSpanContext, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers.ts` around lines 84 - 122, The SpySpan context handling in makeSpan is too static because spanContext() always returns the same hard-coded IDs, which weakens parentSpanContext assertions in tests. Update makeSpan and the SpyTracer path so each created spy span gets a stable, unique span context value that is preserved per span instance, then have spanContext() return that stored context instead of a shared constant. Keep the existing makeTracer/startSpan flow intact, but ensure parentSpanContext comparisons can distinguish the correct ended span from an incorrect one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/session.ts`:
- Around line 75-76: The bounded map writes in session handling are evicting
active spans without ending them, which can drop export/status and break parent
continuity. Update the session span storage flow around setBoundedMap in session
handling so active spans in ctx.runSpans and ctx.sessionSpans are not silently
deleted; either use a span-aware bounded helper that ends evicted spans before
removal, or limit bounded storage to retained span contexts like
ctx.runSpanContexts. Make the same fix wherever the same pattern appears in the
session lifecycle methods that handle idle/error completion.
In `@src/util.ts`:
- Around line 26-37: The trace context resolution in resolveSessionTraceContext
is vulnerable to re-parenting when a new run starts before late events from the
previous run are finished. Update the session/run context lifecycle in the
session handlers so handleSessionIdle also clears the stale runSpanContexts
entry for the session (or otherwise preserves the old run context until late
events are done), and add a test for the Run1 Start → Run2 Start → Late Child
(Run1) ordering to verify late events keep the original parent context instead
of attaching to the new run.
---
Nitpick comments:
In `@tests/handlers/spans.test.ts`:
- Around line 222-226: The test around handleSessionCreated currently only
checks that a span is created, but it does not verify that the fallback span is
actually root-parented. Update this test to assert the created span has no
parent / is rooted when resolveSessionTraceContext() cannot find the parent run,
using the existing makeCtx(), handleSessionCreated(), and tracer.spans helpers
to catch regressions in parent resolution.
In `@tests/helpers.ts`:
- Around line 84-122: The SpySpan context handling in makeSpan is too static
because spanContext() always returns the same hard-coded IDs, which weakens
parentSpanContext assertions in tests. Update makeSpan and the SpyTracer path so
each created spy span gets a stable, unique span context value that is preserved
per span instance, then have spanContext() return that stored context instead of
a shared constant. Keep the existing makeTracer/startSpan flow intact, but
ensure parentSpanContext comparisons can distinguish the correct ended span from
an incorrect one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d52e280-aa4d-4187-b4e8-670e9ebd066f
📒 Files selected for processing (7)
src/handlers/message.tssrc/handlers/session.tssrc/index.tssrc/types.tssrc/util.tstests/handlers/spans.test.tstests/helpers.ts
dialupdisaster
changed the title
1 participant
Summary
This PR replaces the earlier session-keyed trace-parenting approach with a run model keyed by the originating user message ID.
OpenCode does not expose a first-class
runID, but it does expose enough structure to derive one reliably:chat.messagecan provide the usermessageIDUserMessageobjects have stableidsAssistantMessagehasparentID, which points back to the originating user messagemessageIDThis PR uses that model so root traces stay tied to the correct user turn even when a single
sessionIDis reused across multiple runs.What changed
assistant.parentIDrun1 -> run2 -> late child from run1Why
The original trace fragmentation bug came from using
session.created/ live session spans as the trace root for work that is actually turn-scoped.A second user prompt in the same OpenCode session is a new run, not a continuation of the previous trace root.
Keying retained parent context by
sessionIDwas still unsafe because a new run could overwrite the old run context before late events from the previous run arrived. Keying by user message ID removes that collision.Notes
session.created; root traces start from user turnsValidation
bun run typecheckbun test