feat(stacktrace): conditional stack capture (WrapIf/HereIf) (#40) - #52
Conversation
Add HereIf/HereIfDepth, WrapIf/WrapIfDepth, ClassifyIf/ClassifyIfDepth, and HasTrace so layered wrapping skips duplicate stack traces when the cause chain already carries one. Includes early-exit trace presence detection, examples, tests, and CHANGELOG entry. Closes #40
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesConditional stack capture
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WrapIf
participant HasTrace
participant ErrorChain
Caller->>WrapIf: provide cause and message
WrapIf->>HasTrace: inspect cause
HasTrace->>ErrorChain: walk wrapped or joined errors
ErrorChain-->>HasTrace: trace frames or no trace
alt no trace present
WrapIf-->>Caller: wrapped error with captured trace
else trace present
WrapIf-->>Caller: wrapped error without duplicate trace
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
stacktrace/stacktrace_test.go (1)
195-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a top-frame identity check to catch the suspected skip-depth issue.
This test only bounds frame count; it wouldn't detect if
WrapIfDepth's captured trace starts insidewrapIfDepth/captureTraceUnlessPresentrather than at the real caller (see the skip-depth concern raised onstacktrace.golines 201-206/477-520). Consider assertingframes[0].Functionmatches the test function, mirroringExampleHere'sTop frame: ...ExampleHerecheck instacktrace/example_test.go.🤖 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 `@stacktrace/stacktrace_test.go` around lines 195 - 206, Strengthen TestWrapIfDepthCapturesWhenAbsent by asserting the first extracted frame identifies TestWrapIfDepthCapturesWhenAbsent. Add the check after confirming frames are non-nil, preserving the existing maximum-frame-count assertion and failing with the observed function name when the captured trace starts inside WrapIfDepth or captureTraceUnlessPresent.
🤖 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 `@stacktrace/stacktrace_test.go`:
- Around line 195-206: Strengthen TestWrapIfDepthCapturesWhenAbsent by asserting
the first extracted frame identifies TestWrapIfDepthCapturesWhenAbsent. Add the
check after confirming frames are non-nil, preserving the existing
maximum-frame-count assertion and failing with the observed function name when
the captured trace starts inside WrapIfDepth or captureTraceUnlessPresent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd2fd452-d5d5-4797-9ded-8d10aa465f28
📒 Files selected for processing (4)
CHANGELOG.mdstacktrace/example_test.gostacktrace/stacktrace.gostacktrace/stacktrace_test.go
WrapIf/ClassifyIf captured traces starting inside internal helpers instead of the real caller. Use skip=4 to match Wrap()'s effective caller depth. Add top-frame identity checks to *If depth tests so a wrong skip depth is caught immediately.
Description
Adds conditional stack capture APIs to the
stacktracepackage. When an error chain already contains a trace, these helpers skip redundant capture while still wrapping or classifying the error.Type of Change
Related Issues
Fixes #40
Changes Made
HasTrace(err) boolfor efficient trace presence checksHereIf/HereIfDepth— no-opClassifiedwhen a trace already existsWrapIf/WrapIfDepth— wrap without redundant stack captureClassifyIf/ClassifyIfDepth— classify without redundant stack capturecollectFromso emptyTracerframes do not count as having a traceExampleWrapIf,ExampleHereIf)Testing
Test Coverage
go test ./...)go test -race ./...)Manual Testing
go test -run Example)golangci-lint run ./...— 0 issuesDocumentation
Code Quality
golangci-lint run)go vetpassesgo fmthas been runBreaking Changes
Does this PR introduce breaking changes?
Performance Impact
HasTraceuses early-exit traversal (presenceOnly) to avoid collecting full traces when only checking presence.*Ifvariants skipruntime.Callerswhen a trace is already present.Additional Notes
Closes #40
Summary by CodeRabbit
New Features
Documentation