feat(studio): correlate logs with traces and expand span detail - #369
Conversation
The Traces panel could show a request's waterfall and the Logs panel could
show the same request's log lines, but nothing connected them: `LogEvent`
carries a `traceId` to every sink, and the in-memory log buffer dropped it
on the way in. A span's attributes were likewise truncated into a
fixed-width cell, so the `{ orderId }` you opened the panel to read was
exactly what got cut, and `kind` / recorded span events never left the fold
at all.
- buffer `traceId` on a log entry and stamp it at the `ctx.log` and
dispatch-error push sites, so `getLogs` can be joined to `getTraces`
- carry `kind` and `events` through `foldTraces` onto `TraceSpan`
- expand a span row into a detail block: ids, kind, timing, the full
attribute bag, the error, and `addEvent`/`recordException` events
- show the dispatch's log lines under its expanded waterfall, read from the
same live log ring rather than a new correlated RPC
- draw an elapsed-time ruler over the bars, add an "Errors only" filter, and
match the search box against span names and span ids
- link a log row to its trace, reusing the metrics exemplar hand-off
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
…ents The backend-free mock is what `main.mock.tsx` renders for design work, and it answered `getLogs` with entries carrying no `traceId` and `getTraces` with spans carrying no `kind` or events — so the trace/log correlation, the span detail's kind row, and the events list were all invisible in mock mode. The log fixtures already lined up with the trace fixtures by function path, so this only stamps the matching trace ids and adds a handled retry plus a recorded exception. One log line is deliberately left untraced, standing in for a container-lifecycle entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR propagates trace IDs through logs and request logs, preserves span events and kinds, and expands Studio trace inspection, filtering, log correlation, and navigation from Logs views. ChangesTrace observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LogsPanel
participant UseOpenTrace
participant TracesPanel
participant LogRing
LogsPanel->>UseOpenTrace: store shard and traceId
UseOpenTrace->>TracesPanel: navigate to /traces
TracesPanel->>LogRing: query correlated logs
LogRing-->>TracesPanel: return entries indexed by traceId
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Thank you for following the naming conventions! 🙏 |
|
Thank you for confirming the Contributor License Agreement! 🙏 |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #369 +/- ##
==========================================
+ Coverage 87.09% 87.10% +0.01%
==========================================
Files 1172 1177 +5
Lines 63383 63582 +199
Branches 15447 15306 -141
==========================================
+ Hits 55202 55384 +182
- Misses 7654 7676 +22
+ Partials 527 522 -5
🚀 New features to boost your workflow:
|
Merging this PR will degrade performance by 3.17%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | in-batch: single IN(...) query + id->doc re-projection |
799.4 µs | 948.9 µs | -15.76% |
| ⚡ | count, no attributes |
67.1 µs | 60.3 µs | +11.31% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing feat/studio-trace-log-correlation (cf943c2) with alpha (1762097)2
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
alpha(2dc73bf) during the generation of this report, so 1762097 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
…est log The in-memory log buffer now carries a trace id, but the durable request log — the one that survives hibernation and records EVERY dispatch — did not, so the Requests view had no way back to a waterfall and a SIEM receiving the Logpush event could not join it to the collector's spans for the same request. - add a `trace_id` column via a guarded ALTER, mirroring `error_fingerprint`; each ALTER gets its own try so a shard that already has one column still gains the other - stamp it from the shared per-request anchor, so the row lands in the same trace as that dispatch's spans and log lines - emit it on the Logpush console event — the join key on the far side - never redact it: a trace id is an opaque identifier, and masking it destroys the only thing it is for - link a request row to its trace, extracting the cell both log views now share Note the deliberate retention asymmetry, documented at the module, the type, the component, and the docs page: this log is durable while the span ring it points at is in-memory, so a link on a row older than the current DO instance resolves in an external collector but not against the local waterfall. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/do/src/shard-do.ts (1)
6412-6455: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the trace-id race in
recordRequestLog.
recordRequestLogreadsthis.currentRequestTrace?.traceIdat line 6449. Both call sites (line 4467, the "ok" path, and line 4526, the "error" path) invoke it AFTERawait this.handleRpc(...)resolves or throws. A Durable Object can interleave a different dispatch's synchronous code during that await. When that happens,this.currentRequestTraceno longer holds THIS dispatch's anchor — it holds whatever dispatch last set the shared field.The surrounding code already solves this exact problem for the same dispatch:
dispatchTraceis captured as a local at line 4322 specifically because "the finally below runs after the handler's awaits, by which point an interleaved dispatch may have re-set the shared field." The error-paththis.logs.push({ ..., traceId: dispatchTrace.traceId })at line 4535 correctly uses that local.recordRequestLogdoes not — it re-reads the shared field instead of receiving the anchor by value.Under concurrent dispatch on the same shard instance, a request-log row (and its mirrored Logpush event, per
emitRequestLogEvent) can be persisted with another request'straceId. This directly undermines the log↔trace correlation this PR adds, and does so silently.As per path instructions for
packages/**/src/**/*.ts("Verify error handling" and "adheres to best practices associated with nodejs"), thread the trace anchor through by value instead of reading the shared field.🐛 Proposed fix: thread the trace anchor by value into `recordRequestLog`
private recordRequestLog( functionPath: string, args: Record<string, unknown>, durationMs: number, outcome: "error" | "ok", tablesWritten: string[], errorMessage?: string, + trace?: { traceId: string }, ): void { const config = this.requestLogConfig(); @@ tablesWritten, // Read from the shared per-request anchor rather than re-resolved, so // the row lands in the SAME trace as this dispatch's `ctx.trace` spans // and its `ctx.log` lines. `undefined` outside a dispatch-scoped call. - traceId: this.currentRequestTrace?.traceId, + traceId: trace?.traceId, ts: Date.now(), userId: this.getCurrentUserId(), };And pass
dispatchTraceat both call sites:- this.recordRequestLog(payload.functionPath, payload.args ?? {}, durationMs, "ok", tablesWritten); + this.recordRequestLog(payload.functionPath, payload.args ?? {}, durationMs, "ok", tablesWritten, dispatchTrace);- this.recordRequestLog(payload.functionPath, payload.args ?? {}, durationMs, "error", [...(this.pendingChangedTables ?? [])], message); + this.recordRequestLog(payload.functionPath, payload.args ?? {}, durationMs, "error", [...(this.pendingChangedTables ?? [])], message, dispatchTrace);🤖 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 `@packages/do/src/shard-do.ts` around lines 6412 - 6455, Fix the trace-id race by adding a trace-anchor parameter to recordRequestLog and using that value when populating entry.traceId instead of reading this.currentRequestTrace. Pass the dispatchTrace local from both the success and error call sites after handleRpc completes, preserving the same anchor for persisted request logs and emitted events.Source: Path instructions
🤖 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 `@packages/studio/src/features/traces/traces-panel.tsx`:
- Around line 119-122: Update the tick label rendering in the ticks.map block so
the terminal 100% label is aligned to the left of its gridline while preserving
the existing 100% ruler position and alignment for other ticks. Apply the
conditional positioning through the span’s styling or classes using
tick.percent.
---
Outside diff comments:
In `@packages/do/src/shard-do.ts`:
- Around line 6412-6455: Fix the trace-id race by adding a trace-anchor
parameter to recordRequestLog and using that value when populating entry.traceId
instead of reading this.currentRequestTrace. Pass the dispatchTrace local from
both the success and error call sites after handleRpc completes, preserving the
same anchor for persisted request logs and emitted events.
🪄 Autofix
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 Plus
Run ID: fe2c5a11-8387-48e2-89c2-56094b115783
⛔ Files ignored due to path filters (9)
api-snapshots/observability.api.mdis excluded by none and included by noneapi-snapshots/studio.api.mdis excluded by none and included by nonepackages/do/__tests__/shard-do.admin.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/observability/__tests__/request-log.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/observability/__tests__/span-buffer.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/app/studio.test.tsxis excluded by!**/__tests__/**and included bypackages/**packages/studio/__tests__/features/logs/logs-panel.test.tsxis excluded by!**/__tests__/**and included bypackages/**packages/studio/__tests__/features/traces/trace-geometry.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/features/traces/traces-panel.test.tsxis excluded by!**/__tests__/**and included bypackages/**
📒 Files selected for processing (12)
apps/docs/src/content/docs/concepts/observability.mdxapps/studio/src/mock/dev-client.tspackages/do/src/shard-do.tspackages/observability/src/log-buffer.tspackages/observability/src/request-log.tspackages/observability/src/span-buffer.tspackages/studio/src/features/logs/logs-panel.tsxpackages/studio/src/features/traces/span-detail.tsxpackages/studio/src/features/traces/trace-geometry.tspackages/studio/src/features/traces/traces-panel.tsxpackages/studio/src/lib/admin.tspackages/studio/src/locales/en.ts
| {ticks.map((tick) => ( | ||
| <span className="absolute inset-y-0 border-l border-border/70 pl-1" key={tick.percent} style={{ left: `${String(tick.percent)}%` }}> | ||
| {tick.label} | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the terminal ruler label inside the timeline track.
Line 120 positions the 100% tick at left: 100%. Its label starts after the track and can overlap the duration and detail columns. Keep the gridline at 100%, but align its label to the left of that edge.
Proposed fix
<span className="absolute inset-y-0 border-l border-border/70 pl-1" key={tick.percent} style={{ left: `${String(tick.percent)}%` }}>
- {tick.label}
+ <span className={tick.percent === 100 ? "inline-block -translate-x-full" : undefined}>{tick.label}</span>
</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {ticks.map((tick) => ( | |
| <span className="absolute inset-y-0 border-l border-border/70 pl-1" key={tick.percent} style={{ left: `${String(tick.percent)}%` }}> | |
| {tick.label} | |
| </span> | |
| {ticks.map((tick) => ( | |
| <span className="absolute inset-y-0 border-l border-border/70 pl-1" key={tick.percent} style={{ left: `${String(tick.percent)}%` }}> | |
| <span className={tick.percent === 100 ? "inline-block -translate-x-full" : undefined}>{tick.label}</span> | |
| </span> |
🤖 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 `@packages/studio/src/features/traces/traces-panel.tsx` around lines 119 - 122,
Update the tick label rendering in the ticks.map block so the terminal 100%
label is aligned to the left of its gridline while preserving the existing 100%
ruler position and alignment for other ticks. Apply the conditional positioning
through the span’s styling or classes using tick.percent.
Formatting only, no behaviour change. The bigint-queryability test landed on alpha with a paren group prettier removes, which fails the repo-wide `Lint (prettier)` job on every branch that merges alpha afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
`recordRequestLog` runs after the handler's awaits, and it read the shared `currentRequestTrace` field. A Durable Object interleaves dispatches at await points, so a concurrent request re-sets that field — or clears it in its `finally` — before the first one records its row. Two concurrent `/rpc` calls could file request A's durable row, and the Logpush event carrying it, under request B's trace. Silently: the Requests view's Trace link would open the wrong waterfall and nothing would report a mismatch. The anchor is now threaded by value, matching the `dispatchTrace` capture the same function already uses for the root span — whose comment describes this exact hazard three lines above the code that ignored it. Regression test drives the real race: park a dispatch inside its handler, run a sibling with a different trace to completion, then release. It fails against the previous code with `undefined`, the symptom of the sibling's `finally`. Review fixes, same pass: - gate the Traces panel's correlated-log read on an expanded row, so visiting the page no longer opens a second live subscription streaming the whole log ring for a section that starts collapsed - restore the waterfall row's accessible name: as a button it concatenated every cell, including the bar's own label, into a run-on string - put the ruler and the span rows on one shared grid, so gridlines cannot silently drift from the bars they annotate - key correlated log rows by position; identical messages in the same millisecond are ordinary and collided on a content-derived key - shift the terminal ruler label back over its gridline instead of past the track - loop the guarded ALTERs, making per-column isolation structural rather than a comment defending two copy-pasted blocks - share one LogLine between both log views (they had already drifted on the level column's width and on timestamp formatting) and one useOpenTrace hook between the Logs and Metrics drill-downs - move the waterfall into its own module; let a span row own its expansion state instead of prop-drilling a Set from the panel - replace the hand-rolled log bucketing with Map.groupBy, dropping a useMemo React Compiler flagged as redundant - split the ruler test, which asserted absence against a prior render's DOM, and make the redaction test prove redaction actually ran on the row - collapse the retention-asymmetry note from five copies to one Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/studio/src/hooks/use-open-trace.ts (1)
21-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider clearing the hand-off if navigation fails.
fireAndForgetdiscards a rejectednavigatepromise. If navigation fails, the pending trace filter stays in session storage. The next manual visit to Traces then applies a filter the user did not request — the exact failure the doc comment at Lines 12-15 describes. Clear the stored filter on the rejection path.♻️ Proposed change
import { fireAndForget } from "../lib/internal"; -import { writePendingTraceFilter } from "../lib/trace-handoff"; +import { clearPendingTraceFilter, writePendingTraceFilter } from "../lib/trace-handoff"; @@ return (traceId: string): void => { writePendingTraceFilter({ shardKey, traceId }); - fireAndForget(navigate({ to: "/traces" })); + fireAndForget( + navigate({ to: "/traces" }).catch((error: unknown) => { + // Navigation failed, so no panel will consume the hand-off. + clearPendingTraceFilter(); + throw error; + }), + ); };As per path instructions: "Verify error handling".
🤖 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 `@packages/studio/src/hooks/use-open-trace.ts` around lines 21 - 24, Update the callback returned by the trace-opening hook to handle rejected navigation from navigate instead of discarding it through fireAndForget. On rejection, clear the pending trace filter written by writePendingTraceFilter, while preserving the existing successful navigation behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In `@packages/studio/src/hooks/use-open-trace.ts`:
- Around line 21-24: Update the callback returned by the trace-opening hook to
handle rejected navigation from navigate instead of discarding it through
fireAndForget. On rejection, clear the pending trace filter written by
writePendingTraceFilter, while preserving the existing successful navigation
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc9c12a0-2283-4110-9d54-e4cd4fcbc9b3
⛔ Files ignored due to path filters (5)
packages/do/__tests__/shard-do.sampling.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/observability/__tests__/request-log.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/shard-engine/__tests__/ctx-db.bigint-bytes.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/features/logs/logs-panel.test.tsxis excluded by!**/__tests__/**and included bypackages/**packages/studio/__tests__/features/traces/traces-panel.test.tsxis excluded by!**/__tests__/**and included bypackages/**
📒 Files selected for processing (11)
packages/do/src/shard-do.tspackages/observability/src/log-buffer.tspackages/observability/src/request-log.tspackages/studio/src/features/logs/log-line.tsxpackages/studio/src/features/logs/logs-panel.tsxpackages/studio/src/features/reports/metrics-panel.tsxpackages/studio/src/features/traces/trace-waterfall.tsxpackages/studio/src/features/traces/traces-panel.tsxpackages/studio/src/hooks/use-open-trace.tspackages/studio/src/lib/admin.tspackages/studio/src/locales/en.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/studio/src/locales/en.ts
- packages/observability/src/log-buffer.ts
- packages/studio/src/lib/admin.ts
- packages/studio/src/features/logs/logs-panel.tsx
- packages/do/src/shard-do.ts
- packages/observability/src/request-log.ts
The TTL sweep and the shape fan-out's alarm callers were recorded with no trace, on the claim that no trace was ambient. That was wrong: `withTriggerTrace` publishes an anchor for the whole alarm, so those lines were attributable all along — and the alarm is, per its own docstring, where a silent failure hides longest because no client is waiting on a response. Threading it by value rather than reading a field, because `recordShapeError` has callers on both the alarm and socket-frame paths: a field read there would let a socket frame interleaving at an await point file its own failure under the alarm's trace, which is the bug this branch already fixed once. - `withTriggerTrace` publishes its anchor on a dedicated `currentTriggerTrace`, needed only because `runner.handleAlarm()` sits between it and the handler - `handleAlarmCloudflare` captures that into a local synchronously at entry, before its first await, and passes it down through `pollTier`, `pollTtlSweeps` → `deleteExpiredTtlRow`, and `pollGlobalShapes` → `pollSocketGlobalShapes` - `recordShapeError` takes the anchor as an optional parameter, so the socket-frame callers stay untraced, which is correct for them The test drives two alarm ticks and asserts each contained failure carries its own 32-hex id; it fails without the threading. `recordExternalSourceError` is left alone — it is called from codegen-emitted code, so threading it is a cross-package change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/do/src/shard-do.ts (2)
4638-4665: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCarry the alarm anchor into deferred subscription refreshes.
handleAlarmCloudflarecallsflushChangedTables()after the traced poll work.flushChangedTablescan submitdrainSubscriptionRefreshes()torunner.background(...)and return before that work completes.The
withTriggerTracecleanup then restorescurrentTriggerTraceand clearscurrentRequestTrace. A laterrecordSubscriptionRefreshErrorcall usesrecordUserLogwithout an explicit anchor. The error is then trace-less or can inherit a concurrent RPC trace.Thread the captured
TraceAnchorthrough the deferred refresh path and pass it torecordSubscriptionRefreshErrorandrecordUserLog. Do not read ambient request fields from the background task.This follows the PR objective that alarm-path log errors retain the trigger trace.
Also applies to: 4763-4780
🤖 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 `@packages/do/src/shard-do.ts` around lines 4638 - 4665, Thread the captured TraceAnchor from handleAlarmCloudflare through flushChangedTables and any deferred drainSubscriptionRefreshes background task. Pass it explicitly to recordSubscriptionRefreshError and recordUserLog, ensuring the background callback never reads ambient currentTriggerTrace or request fields and alarm-path errors retain the originating trigger trace.
8567-8588: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate
traceto contained alarm error records.
pollGlobalShapes(trace)reaches the outer captures, but the cap path still logs withouttrace:refreshGlobalShape(...)callswithinGlobalShapeBound(...)without an anchor, sorecordShapeError(...)dropstraceId. Add atrace?: TraceAnchorparameter here and pass it frompollSocketGlobalShapes.
recordExternalSourceError(table, error)always callsthis.recordShapeError(...)without an anchor. The codegen-generatedpollExternalSources()uses this in the alarm tier, so per-table source failures can lose their alarm trace identity. Addtrace?: TraceAnchorhere and update the generated callers to pass the alarm trace down.🤖 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 `@packages/do/src/shard-do.ts` around lines 8567 - 8588, Propagate the optional TraceAnchor through the global-shape alarm error paths: update pollSocketGlobalShapes and refreshGlobalShape to accept and forward trace into withinGlobalShapeBound, ensuring recordShapeError receives it. Also update recordExternalSourceError to accept trace and pass it to recordShapeError, then update generated pollExternalSources callers to forward the alarm trace for each table failure.
🤖 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.
Outside diff comments:
In `@packages/do/src/shard-do.ts`:
- Around line 4638-4665: Thread the captured TraceAnchor from
handleAlarmCloudflare through flushChangedTables and any deferred
drainSubscriptionRefreshes background task. Pass it explicitly to
recordSubscriptionRefreshError and recordUserLog, ensuring the background
callback never reads ambient currentTriggerTrace or request fields and
alarm-path errors retain the originating trigger trace.
- Around line 8567-8588: Propagate the optional TraceAnchor through the
global-shape alarm error paths: update pollSocketGlobalShapes and
refreshGlobalShape to accept and forward trace into withinGlobalShapeBound,
ensuring recordShapeError receives it. Also update recordExternalSourceError to
accept trace and pass it to recordShapeError, then update generated
pollExternalSources callers to forward the alarm trace for each table failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3147f59f-5ea0-43b9-a821-33e647afa2e8
⛔ Files ignored due to path filters (2)
api-snapshots/do.api.mdis excluded by none and included by nonepackages/do/__tests__/shard-do.admin.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**
📒 Files selected for processing (1)
packages/do/src/shard-do.ts
…ching into a private field Closes the last uncorrelated alarm-path log site. The generated `pollExternalSources` override now takes the alarm's trace and forwards it to all three places it records a contained failure, so a `.source()` table's ingest error is attributable to the tick that produced it. While threading it, found that the emitted poll loop writes `this.logs.push(...)` — and `logs` is PRIVATE on `ShardDO`, so that statement cannot compile in the generated subclass. Any project declaring a `.source()` table has been emitting a shard that fails `tsc`. It went unnoticed because no fixture or example declares one and the emitted string is only ever asserted against as text, never compiled. Routed through a new protected `recordExternalSourceWarning` seam instead, which keeps the ring encapsulated and gives the line the same correlation as its sibling. The trace crosses the package boundary as `TraceRefLike`, a structural projection exported from `@lunora/do` — `@lunora/do` deliberately does not re-export `@lunora/observability`, and a generated app should not take on that dependency to name a parameter. Verified the emitted shape compiles from OUTSIDE the base class (a throwaway subclass mirroring exactly what the emitter writes) rather than trusting the substring assertions that missed the private access. Two new emit tests pin it: one asserts all three sites forward the trace, one asserts the output never contains `this.logs`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
…ds on Closes the gap that let `this.logs.push(...)` — a private access — ship inside the generated poll loop. The emitter produces TypeScript as a string and every test asserts on substrings of it; nothing compiles it, and golden fixtures cannot, since `tsconfig.json` excludes them (generated output only type-checks inside a whole app). Two halves, each useless alone: - `emitted-shard-contract.ts` is a real subclass of `ShardDO` exercising every base member the emitted sourced shard touches. It is never executed — it just has to compile, so `lint:types` fails in the package that owns the emitter the moment one of those members changes visibility or signature. - A test asserting the emitter still restricts itself to that set, computed as the delta between a sourced and a non-sourced shard's `this.*` usage, so it needs no hand-maintained allowlist of unrelated members. Verified both directions: adding `this.logs.push(...)` back to the contract reproduces the original `TS2341`, and making the emitter reach for an uncovered private member (`pendingChangedTables`) fails the delta test. Both reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
The gate that stops the Traces page opening a second live subscription on every visit had no test, so removing it would have broken nothing. Asserts `getLogs` is untouched while the list is collapsed and read once a trace opens; fails against the ungated version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
2 participants
Why
The Studio could render a request's waterfall in Traces and the same
request's log lines in Logs, and nothing connected them.
LogEventhasalways carried a
traceIdto every sink — the in-memory log buffer dropped iton the way in, and the durable request log never had a column for it — so the
join was impossible on the client no matter how you squinted at timestamps.
Two smaller versions of the same problem:
SpanEvent.kindand.eventswererecorded by
ctx.traceand then discarded byfoldTraces, so aspan.recordExceptionreached the buffer and vanished before anything couldrender it; and a span's attributes were truncated into a fixed-width cell, which
is enough to know an attribute exists and not enough to read it — the
{ orderId }you opened the panel for is exactly what got cut.What changed
Buffers / fold
LogEntrygainstraceId, stamped at thectx.logand dispatch-error pushsites.
foldTracescarrieskindandeventsontoTraceSpan.Alarm-path log sites are correlated too. An earlier revision of this
description claimed the three remaining
logs.pushsites "genuinely have noambient trace". That holds only for the lifecycle-hook failure, reached from
webSocketMessage/webSocketClose— deliberately not wrapped inwithTriggerTrace. The TTL sweep and the shape fan-out's alarm callers do rununder an anchor that
withTriggerTracepublishes, so they are now stamped:withTriggerTracepublishes its anchor on a dedicatedcurrentTriggerTracefield — needed only because
runner.handleAlarm()sits between it and thehandler, so the anchor cannot simply be an argument.
handleAlarmCloudflarecaptures that field into a local synchronously atentry, before its first
await, then passes it by value topollTier,pollTtlSweeps→deleteExpiredTtlRow, andpollGlobalShapes→pollSocketGlobalShapes.recordShapeErrortakes it as an optionalparameter, so the socket-frame callers stay untraced.
That discipline is the point:
recordShapeErrorhas callers on both paths, soreading a field there would let a socket frame interleaving at an await point
file its own failure under the alarm's trace — the same class of bug as the one
above. Its regression test drives two alarm ticks and asserts each failure
carries a distinct 32-hex id; it fails without the threading.
The external-source path is threaded too, which is a cross-package change: the
generated
pollExternalSourcesoverride now takes the alarm's trace andforwards it to all three sites where it records a contained ingest failure. The
anchor crosses the boundary as
TraceRefLike, a structural projection exportedfrom
@lunora/do— that package deliberately does not re-export@lunora/observability, and a generated app should not take on that dependencyjust to name a parameter.
The gap that let it happen is closed too. Nothing in CI compiled emitted
output, and golden fixtures cannot do it —
packages/codegen/tsconfig.jsonexcludes them, because generated code only type-checks inside a whole app. So
the guard is two halves, each useless alone:
packages/codegen/__tests__/emitted-shard-contract.ts— a realShardDOsubclass exercising every base member the emitted sourced shard touches. Never
executed; it only has to compile, so
lint:typesfails in the package thatowns the emitter the moment one of those members changes visibility or
signature.
the delta between a sourced and a non-sourced shard's
this.*usage — sothere is no hand-maintained allowlist to rot.
Verified in both directions: putting
this.logs.push(...)back into thecontract reproduces the original
TS2341, and pointing the emitter at anuncovered private member fails the delta test.
Durable request log
trace_idcolumn added via a guardedALTER, mirroringerror_fingerprint.Each
ALTERgets its owntry: one shared block would let the firstcolumn's duplicate-column error skip the second add, leaving a shard that has
error_fingerprintbut never gainstrace_id.as that dispatch's spans and log lines.
SIEM correlate a request to the collector's spans.
destroys the only thing it is for. Covered by a test.
Traces panel
full attribute bag, the error, and any
addEvent/recordExceptionevents.same live
getLogsring the Logs panel uses — a client-side join of two readsalready pushed over this socket, not a new correlated RPC.
matches span names and span ids as well as the trace's own identifiers.
Logs panel
TraceLinkCell.Mock
apps/studio's backend-free dev client seeds matching trace ids, a handledretry, and a recorded exception, plus one row deliberately left trace-less to
stand in for a pre-column row.
The retention asymmetry (read this before reviewing the Requests link)
The request log is durable and bounded by row count; the
ctx.tracespanring it points at is in-memory and resets on hibernation. So a Trace link on
a row older than the current DO instance lands on an empty Traces panel — the
trace has aged out locally, and the panel's ordinary empty state is the correct
outcome rather than an error.
Recording the id is still worth it: it is what joins the row to whatever
collector
otlpSinkships to (where a deployed app's traces actually live) andto the Logpush event. This is stated at the module docstring, the
RequestLogEntrytype on both sides, the component, and the docs page, so nobody re-derives it
from a surprising empty panel.
Prior art
Modelled on Cloudflare's Local Explorer observability tab
(blog,
docs),
whose trace view is a list → waterfall → per-span attributes → correlated
console logs. Deliberately skipped from that UI: collapsible subtrees, a
resizable name column, per-kind icons, and a clear button — none of them earn
their complexity until someone hits a trace deep enough to need collapsing.
Not in scope
Logs correlate at trace level, not per span:
LogEvent.spanIdis thedispatch root span and is constant across a dispatch, so carrying it would imply
a precision it doesn't have. It is deliberately not stored.
Review pass
A two-reviewer audit (bugs/security + maintainability) ran over the branch. It
found one real correctness bug, now fixed with a regression test:
recordRequestLogread the sharedcurrentRequestTraceafter the handler'sawaits. A DO interleaves dispatches at await points, so a concurrent request
re-sets that field — or clears it in its
finally— before the first onerecords its row. Two concurrent
/rpccalls could file request A's durable row,and the Logpush event carrying it, under request B's trace, silently. The anchor
is now threaded by value, matching the
dispatchTracecapture the same functionalready used for the root span, whose comment describes this exact hazard three
lines above the code that ignored it. The new test parks a dispatch inside its
handler, runs a sibling to completion, then releases; it fails against the
previous code with
undefined.Also fixed from that pass: the correlated-log read is now gated on an expanded
row (it was opening a second live subscription on every page visit for a
collapsed section); the waterfall row's accessible name is explicit again
(as a
<button>it concatenated every cell into a run-on string); the ruler andspan rows share one grid so gridlines can't drift from the bars; correlated log
rows are keyed by position (identical messages in one millisecond collided); the
guarded
ALTERs are a loop;LogLineanduseOpenTraceare shared rather thanduplicated; the waterfall moved to its own module; and two weak tests were
repaired — one asserted absence against a prior render's DOM, the other's stated
rationale was factually wrong.
Verification
lint:eslint,lint:types,lint:package-json, and repo-widelint:prettierclean for
@lunora/studio,@lunora/do,@lunora/observability, andapps/studio. Suites: studio 1027, do 525, codegen 1070, observability 230 —all passing.
api:checkgreen; the snapshot changes are additive (optionaltraceparameters, the newrecordExternalSourceWarningseam, and theTraceRefLikeprojection).The migration path has its own test: it builds the pre-column schema
verbatim, inserts a row, then asserts the guarded
ALTERadds the column,the new row round-trips its id, and the pre-existing row survives with none —
the path a fresh
CREATEnever exercises.The
TraceSpankey drift guards in bothpackages/doandpackages/studiocaught the new keys on the first type-check, which is what they exist for.
Rebased onto the branch's merge of current
alpha; lockfile untouched. Onecommit here is unrelated housekeeping: a redundant paren group that landed on
alphavia #365 was failing the repo-wide prettier job on every branch thatmerged alpha afterwards.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KBeSX2o4sTCPjVDRDWkVQG
Summary by CodeRabbit