feat: lazy stack trace capture with deferred symbolization#19
Conversation
Replace eager per-frame runtime.Caller + runtime.FuncForPC with batch runtime.Callers for PC capture, deferring symbolization to first StackFrame() access via sync.Once. Performance (errors.New): 3500ns→540ns (6.5x), 1600B→392B (75% less) Deep stacks (32 frames): 40500ns→1060ns (38x), 17KB→392B (98% less) Changes: - Default stack depth reduced from 64 to 16 (configurable via SetMaxStackDepth) - captureStack: single runtime.Callers call instead of loop of runtime.Caller - StackFrame: lazy resolution via sync.Once + runtime.CallersFrames - All receivers changed to pointer (required by sync.Once, consistent) - SetMaxStackDepth: atomic.Int32 for concurrent safety - resolveFrames: pre-allocated slice, strings.TrimPrefix for base path - splitFuncName: takes string instead of uintptr (no runtime.FuncForPC)
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSwitches to two-phase lazy stack symbolization: capture program counters and basePath at error creation, resolve frames on first StackFrame() call with sync.Once, make stack depth and base path atomic and concurrency-safe, add benchmarks and tests, and update README anchors and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as User
participant Creator as Error Creator
participant Capture as Stack Capture
participant Resolver as Frame Resolver
participant Runtime as runtime
Caller->>Creator: call New / Wrap
Creator->>Capture: captureStack(skip) -> runtime.Callers(...) -> store PCs + basePath
Capture-->>Creator: return error (frames unresolved)
Caller->>Creator: call StackFrame() [first time]
Creator->>Resolver: resolveFrames() via sync.Once
Resolver->>Runtime: CallersFrames(pcs)
Runtime-->>Resolver: frames (Function, File, Line)
Resolver-->>Creator: cache []StackFrame
Creator-->>Caller: return resolved frames
Caller->>Creator: call StackFrame() [subsequent]
Creator-->>Caller: return cached frames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Updates the core errors package to reduce error creation overhead by capturing stack PCs in one batch and deferring symbolization until stack frames are actually requested, with accompanying documentation and benchmarks.
Changes:
- Replaced eager per-frame stack capture with
runtime.CallersPC capture and lazyStackFrame()resolution viasync.Once. - Made max stack depth configuration thread-safe with
atomic.Int32and reduced default captured depth to 16. - Regenerated package docs/READMEs and added benchmarks covering
New,Wrap, and deep-stack creation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
errors.go |
Implements lazy stack frame symbolization, batch PC capture, and atomic stack depth config. |
bench_test.go |
Adds benchmarks to measure the new stack capture/symbolization behavior. |
README.md |
Regenerated docs reflecting updated defaults and exported constants. |
notifier/README.md |
Regenerated notifier docs/links to reflect updated line references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
errors.go:244
- When wrapping an existing
ErrorExt, the code now reuses onlyCallers()and no longer reuses any already-resolved frames. If the wrapped error’s frames were previously materialized, callingStackFrame()on the new wrapper will re-symbolize the same PCs again. Consider carrying forward already-resolved frames (without forcing symbolization) to avoid repeated work in wrap chains where stack frames are accessed multiple times.
//if we have stack information reuse that
if e, ok := err.(ErrorExt); ok {
c := &customError{
Msg: msg + e.Error(),
cause: e.Cause(),
wrapped: err, // preserve full chain for errors.Is/errors.As
status: status,
shouldNotify: true,
}
c.stack = e.Callers()
if n, ok := e.(NotifyExt); ok {
c.shouldNotify = n.ShouldNotify()
}
return c
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
errors.go (1)
231-244:⚠️ Potential issue | 🟠 MajorPreserve third-party
ErrorExtframes when re-wrapping.This is only lossless for
*customError. A publicErrorExtcan legitimately precompute or sanitizeStackFrame(), and this wrapper now discards that data by keeping onlyCallers().💡 Preserve existing frames for non-
*customErrorimplementationsfunc (c *customError) StackFrame() []StackFrame { c.frameOnce.Do(func() { - if len(c.stack) > 0 { + if len(c.frame) == 0 && len(c.stack) > 0 { c.frame = resolveFrames(c.stack) } }) return c.frame } @@ if e, ok := err.(ErrorExt); ok { c := &customError{ Msg: msg + e.Error(), cause: e.Cause(), wrapped: err, // preserve full chain for errors.Is/errors.As status: status, shouldNotify: true, } - c.stack = e.Callers() + c.stack = append([]uintptr(nil), e.Callers()...) + if _, ok := err.(*customError); !ok { + c.frame = append([]StackFrame(nil), e.StackFrame()...) + } if n, ok := e.(NotifyExt); ok { c.shouldNotify = n.ShouldNotify() } return c }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errors.go` around lines 231 - 244, When wrapping an existing ErrorExt in customError, preserve any precomputed/sanitized frames from the original ErrorExt instead of unconditionally replacing them with Callers(); change the logic in the ErrorExt branch to: if the concrete type is *customError keep current behavior, otherwise if the ErrorExt implements a public frame accessor (e.g. an interface like StackFrame() []StackFrame or similar used by your project) copy that into c.stack; fall back to e.Callers() only if no such accessor exists, and still preserve cause, wrapped, status and shouldNotify as before (references: ErrorExt, customError, Callers(), shouldNotify).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@errors.go`:
- Around line 93-99: The stack trimming currently happens lazily in
customError.StackFrame() via resolveFrames(c.stack), which reads the global
basePath and causes instability/races when SetBaseFilePath changes; fix this by
capturing and freezing the basePath at error creation time (e.g., add a
basePathSnapshot or pre-trim frames when the stack is captured) and make
StackFrame() use that frozen value instead of reading global state; update the
logic around resolveFrames, customError.stack initialization, and any places
that create customError so the basePath is stored/used at capture time
(reference: customError.StackFrame, resolveFrames, and SetBaseFilePath).
- Around line 261-264: SetMaxStackDepth currently ignores out-of-range n and
leaves the previous value; change it to normalize/clamp the input into the valid
range [1,256] before storing so callers always see a deterministic result.
Inside SetMaxStackDepth, compute a clamped value (e.g., if n < 1 set to 1, if n
> 256 set to 256), then call atomicStackDepth.Store(int32(clamped)) and remove
the conditional that silently drops invalid inputs. Ensure you still use int32
conversion when storing.
---
Outside diff comments:
In `@errors.go`:
- Around line 231-244: When wrapping an existing ErrorExt in customError,
preserve any precomputed/sanitized frames from the original ErrorExt instead of
unconditionally replacing them with Callers(); change the logic in the ErrorExt
branch to: if the concrete type is *customError keep current behavior, otherwise
if the ErrorExt implements a public frame accessor (e.g. an interface like
StackFrame() []StackFrame or similar used by your project) copy that into
c.stack; fall back to e.Callers() only if no such accessor exists, and still
preserve cause, wrapped, status and shouldNotify as before (references:
ErrorExt, customError, Callers(), shouldNotify).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Snapshot basePath in captureStack so lazy resolution uses the value from error creation time, not access time (eliminates race) - resolveFrames takes basePath parameter instead of reading global - Document SetMaxStackDepth accepts [1, 256] - Add TestSetMaxStackDepth (valid, zero, negative, over-256) - Add TestStackFrameConsistency (repeated calls, Callers parity) - Update existing depth tests for new default of 16
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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)
errors.go (1)
233-246:⚠️ Potential issue | 🟠 MajorPreserve the inner stack-frame context when re-wrapping
ErrorExt.This branch only copies
e.Callers(). If the inner error was captured with a non-emptybasePath, the wrapper resolves the same PCs withc.basePath == "", soStackFrame()flips from trimmed paths back to absolute paths after a wrap. For non-*customErrorErrorExtimplementations, you can also lose already-materialized frames entirely. That shows up downstream innotifier/notifier.go:379, which serializesStackFrame()directly.💡 One way to keep wrapped output stable
if e, ok := err.(ErrorExt); ok { c := &customError{ Msg: msg + e.Error(), cause: e.Cause(), wrapped: err, // preserve full chain for errors.Is/errors.As status: status, shouldNotify: true, } - c.stack = e.Callers() + c.stack = append([]uintptr(nil), e.Callers()...) + if inner, ok := e.(*customError); ok { + c.basePath = inner.basePath + } else { + c.frame = append([]StackFrame(nil), e.StackFrame()...) + c.frameOnce.Do(func() {}) + } if n, ok := e.(NotifyExt); ok { c.shouldNotify = n.ShouldNotify() } return c }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errors.go` around lines 233 - 246, When re-wrapping an ErrorExt in the branch that builds a new customError, preserve the inner error's stack/frame context and any basePath used to trim frames: if the wrapped error (e) exposes basePath or already-materialized frames (e.g. is a *customError or implements accessors for basePath/frames), copy those into the new customError fields (e.g. c.basePath = e.basePath or via an accessor, and copy any cached/materialized stack frames rather than only calling e.Callers()), so StackFrame() continues to return the same trimmed/flattened frames after wrapping; keep wrapped: err as-is to preserve errors.Is/errors.As behavior and still update shouldNotify from NotifyExt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@errors.go`:
- Around line 135-154: resolveFrames may return more frames than intended
because runtime.CallersFrames can expand PCs into multiple inlined frames;
enforce the original depth cap by stopping after processing len(pcs) logical
frames. Modify resolveFrames (which currently iterates until frames.Next()
returns false) to count appended frames and break once the count reaches
len(pcs) (the number of PCs captured by captureStack), ensuring StackFrame slice
length never exceeds the original depth limit while still trimming file paths
and extracting function names via splitFuncName.
---
Outside diff comments:
In `@errors.go`:
- Around line 233-246: When re-wrapping an ErrorExt in the branch that builds a
new customError, preserve the inner error's stack/frame context and any basePath
used to trim frames: if the wrapped error (e) exposes basePath or
already-materialized frames (e.g. is a *customError or implements accessors for
basePath/frames), copy those into the new customError fields (e.g. c.basePath =
e.basePath or via an accessor, and copy any cached/materialized stack frames
rather than only calling e.Callers()), so StackFrame() continues to return the
same trimmed/flattened frames after wrapping; keep wrapped: err as-is to
preserve errors.Is/errors.As behavior and still update shouldNotify from
NotifyExt.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55e2f075-a48c-4904-9b0c-ccea2094be68
📒 Files selected for processing (3)
README.mderrors.goerrors_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
…t assertions - Copy basePath from wrapped ErrorExt so path trimming works on re-wrapped errors - Cap resolveFrames output at len(pcs) to handle inlining frame expansion - Test assertions use Callers() length (PCs) instead of StackFrame() length - Relax 1:1 PC-to-frame assumption in TestStackFrameConsistency
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
errors.go (1)
243-246: Freeze the wrapped PCs instead of aliasing them.Line 243 stores the slice returned by
e.Callers()directly. BecauseErrorExtis exported, another implementation can legally reuse or mutate that backing array, which would change this wrapper's lazyStackFrame()result after construction. Copy once here so the wrapper owns an immutable snapshot.📌 Small defensive copy
- c.stack = e.Callers() + callers := e.Callers() + c.stack = append([]uintptr(nil), callers...) if ce, ok := e.(*customError); ok { c.basePath = ce.basePath }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errors.go` around lines 243 - 246, The wrapper stores the slice returned by e.Callers() directly (assigned to c.stack), which lets external implementations mutate the backing array later; instead, make a defensive copy of the PCs when constructing the wrapper so the wrapper owns an immutable snapshot used by StackFrame(); update the assignment in the constructor/initializer that sets c.stack to copy the slice (e.g. allocate a new slice and copy elements from e.Callers()) while leaving the customError, basePath, and rest of the logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@errors.go`:
- Around line 22-23: The package-global basePath is written unsafely by
SetBaseFilePath and still read without synchronization when snapshotting (e.g.,
where StackFrame or the snapshot logic reads basePath around line ~131); update
the snapshotting/read sites to access basePath via a synchronized mechanism (use
the existing atomicStackDepth pattern or an atomic.Value / mutex) so
SetBaseFilePath stores the new string atomically and the snapshot/read path
loads it atomically before building the snapshot; reference and protect the
basePath symbol and ensure SetBaseFilePath and the snapshotting code use the
same synchronization primitive.
- Around line 83-89: Callers() exposes raw PCs and some consumers (Rollbar and
Sentry code paths that call runtime.CallersFrames() and the test
notifier_rollbar_airbrake_test) iterate frames without capping, risking
unbounded expansion; update those consumers to stop iterating after they've
yielded as many frames as were in the original pcs slice by adding an explicit
count check (e.g., track yieldedFrames and break when yieldedFrames >= len(pcs))
or reuse the existing resolveFrames() approach which already enforces maxFrames;
specifically modify the loops that call runtime.CallersFrames() in the notifier
Rollbar and Sentry handlers and in the notifier_rollbar_airbrake_test to check
len(frames) >= len(pcs) (or equivalent yieldedFrames >= len(pcs)) and break to
prevent frame growth.
---
Nitpick comments:
In `@errors.go`:
- Around line 243-246: The wrapper stores the slice returned by e.Callers()
directly (assigned to c.stack), which lets external implementations mutate the
backing array later; instead, make a defensive copy of the PCs when constructing
the wrapper so the wrapper owns an immutable snapshot used by StackFrame();
update the assignment in the constructor/initializer that sets c.stack to copy
the slice (e.g. allocate a new slice and copy elements from e.Callers()) while
leaving the customError, basePath, and rest of the logic unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6257793b-91b6-4c92-8abc-989aae82d7e6
📒 Files selected for processing (2)
errors.goerrors_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- errors_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Replaces eager per-frame stack capture with batch PC capture and deferred symbolization, reducing error creation cost by 6.5x on the hot path.
What changed
runtime.Callers()call replaces a loop ofruntime.Caller(i)+runtime.FuncForPC(pc)per frameStackFrame()resolves frames on first access viasync.Once, not at creation timeSetMaxStackDepth)SetMaxStackDepthusesatomic.Int32, safe for concurrent usecustomErrormethods use pointer receivers (required bysync.Once)Benchmark results (Apple M1 Pro)
errors.NewNew+StackFrame()Wrap(reuse stack)NewDeepStack(32 frames)Compatibility
Callers() []uintptr— unchanged, returns raw PCs as beforeStackFrame() []StackFrame— same output, now lazy. Sentry/Rollbar/Airbrake notifiers all work unchangedSetMaxStackDepth(n)— now thread-safe, default lowered to 16Test plan
make testpasses with-race(both errors and notifier packages)make buildcompiles cleanlybench_test.go)Summary by CodeRabbit
New Features
Bug Fixes
Changes
Documentation
Tests