Capture structured Error.cause data#148
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds support for 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/logfire-api/src/index.test.ts (1)
589-650: ⚡ Quick winAdd explicit tests for nested and circular cause chains.
These tests currently validate only a single-level
cause. Since this PR’s behavior includes recursive chains and circular-chain handling, please add at least one nested (2+ levels) and one circular-cause regression test in bothspanandreportErrorpaths to lock the contract.Also applies to: 743-792
🤖 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/logfire-api/src/index.test.ts` around lines 589 - 650, Add two new test cases for the span function path and two for the reportError function path to validate handling of complex cause chains. The first set should test nested cause chains where an error has a cause that itself has a cause (2+ levels deep), similar to the existing single-level cause test but with multiple levels of error nesting. The second set should test circular cause chains where an error's cause chain eventually references back to the original error or creates a loop, ensuring the implementation handles these edge cases without infinite recursion or stack overflow. Use the same structure as the existing test but construct errors with nested properties and circular references in the cause chain to verify both span and reportError paths properly record and serialize these complex scenarios.
🤖 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/logfire-api/src/index.test.ts`:
- Around line 589-650: Add two new test cases for the span function path and two
for the reportError function path to validate handling of complex cause chains.
The first set should test nested cause chains where an error has a cause that
itself has a cause (2+ levels deep), similar to the existing single-level cause
test but with multiple levels of error nesting. The second set should test
circular cause chains where an error's cause chain eventually references back to
the original error or creates a loop, ensuring the implementation handles these
edge cases without infinite recursion or stack overflow. Use the same structure
as the existing test but construct errors with nested properties and circular
references in the cause chain to verify both span and reportError paths properly
record and serialize these complex scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 38da35cf-6988-49f4-ba47-836f4e747ca7
📒 Files selected for processing (5)
.changeset/tame-errors-cause.mdexamples/express/.env.exampleexamples/express/app.tspackages/logfire-api/src/index.test.tspackages/logfire-api/src/index.ts
There was a problem hiding this comment.
Pull request overview
This PR enhances the Logfire JS SDK’s exception reporting to preserve Error.cause chains by (a) appending the cause chain into the recorded exception stacktrace and (b) emitting a structured exception.cause attribute using the existing JSON serialization + logfire.json_schema metadata, with an accompanying Express demo endpoint.
Changes:
- Extend exception recording to include
Error.causechains in stacktraces and add a structuredexception.causeattribute (forreportError()and span/instrument callback failures). - Add tests covering the new structured-cause behavior for
span()failures andreportError(). - Update the Express example with a
/structured-cause-errordemo route and align the example env var name toLOGFIRE_TOKEN(plus a changeset for a patch release).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/logfire-api/src/index.ts | Adds cause-chain stack formatting and structured exception.cause serialization for span/reportError exception paths. |
| packages/logfire-api/src/index.test.ts | Adds unit tests asserting cause-chain stacktrace output and JSON-schema-backed structured exception.cause. |
| examples/express/app.ts | Adds a demo error generator + /structured-cause-error route and attaches demo metadata to reportError(). |
| examples/express/.env.example | Updates the example token env var to LOGFIRE_TOKEN. |
| .changeset/tame-errors-cause.md | Declares a patch changeset for the new structured cause behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Error.causechains in recorded exception stacktraces forreportError()and span/instrument callback failuresexception.causeLogfire attribute that uses the existing JSON serialization +logfire.json_schemametadata pathcode,statusCode,details,operation, and nested causes/structured-cause-errorso the resulting shape can be inspected in the Logfire UIContext
A user reported that the JavaScript SDK currently omits
Error.cause, which is often where the actionable debugging context lives. Upstream OpenTelemetry JS currently maps exceptions toexception.type,exception.message, andexception.stacktrace, so passing a raw JSErrortorecordException()losescause.Relevant upstream context:
recordException should support chained exceptionsrecordException should support chained exceptions open-telemetry/opentelemetry-js#4227
recordException should support chained exceptions open-telemetry/semantic-conventions#941
exception.cause?.stackintoATTR_EXCEPTION_STACKTRACEAdd exception.cause?.stack to the
ATTR_EXCEPTION_STACKTRACEattribute open-telemetry/opentelemetry-js#6423exception.causewas called out as a follow-upfeat(sdk-logs,api-logs): support exceptions in Logger API open-telemetry/opentelemetry-js#6385
Span.recordException()implementation only readscode/name,message, andstackhttps://github.com/open-telemetry/opentelemetry-js/blob/main/packages/sdk-trace/src/Span.ts
Approach
This keeps the standard OTel attributes useful by appending the cause chain to
exception.stacktrace. That matches the upstream direction in open-telemetry/opentelemetry-js#6423 and is compatible with generic OTel consumers.For Logfire specifically, we also emit
exception.causeas a serialized JSON object using the SDK's existing object serialization mechanism. That gives the Logfire UI structured data withlogfire.json_schema, instead of flattening all cause information into an opaque stack string. The structured object is recursive, handles circular chains, and includes enumerable custom fields on caused errors.This avoids introducing a new dependency or waiting on OTel semantic-convention changes while still leaving the standard
exception.*surface intact.Validation
vp run logfire#test -- -t "cause chain"vp run logfire#typecheckvp run logfire#testvp fmt --check packages/logfire-api/src/index.ts packages/logfire-api/src/index.test.ts .changeset/tame-errors-cause.mdpnpm buildpnpm --dir examples/express exec tsc --noEmitLOGFIRE_SEND_TO_LOGFIRE=falseagainst/structured-cause-error