-
Couldn't load subscription status.
- Fork 485
chore(stacktrace): Use internal/stacktrace package where it possible #4062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kemal Akkoyun <[email protected]>
BenchmarksBenchmark execution time: 2025-10-22 16:06:51 Comparing candidate commit 8ba9e8b in PR branch Found 1 performance improvements and 3 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics. scenario:BenchmarkCaptureStackTrace/10-24
scenario:BenchmarkCaptureStackTrace/100-24
scenario:BenchmarkCaptureStackTrace/200-24
scenario:BenchmarkCaptureStackTrace/50-24
|
Signed-off-by: Kemal Akkoyun <[email protected]>
22e894f to
dfb0b1a
Compare
I will introduce subsequent PRs to improve this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| } | ||
| // Use SkipAndCaptureUnfiltered to capture all frames including internal DD frames. | ||
| // +4 to account for: runtime.Callers, iterator, SkipAndCaptureUnfiltered, and this WrapN function | ||
| stack := stacktrace.CaptureRaw(int(skip) + 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kakkoyun Are the comment and the actual value out of sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to update the comment, good catch. SkipAndCaptureUnfiltered doesn't exist anymore.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Use SkipAndCaptureUnfiltered to capture all frames including internal DD frames. | ||
| // +4 to account for: runtime.Callers, iterator, SkipAndCaptureUnfiltered, and this WrapN function | ||
| stack := stacktrace.CaptureRaw(int(skip) + 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honor skip parameter when wrapping errors
The new WrapN implementation always calls stacktrace.CaptureRaw(int(skip) + 2) before symbolication. Because the stack collection now has an extra wrapper (CaptureRaw) compared to the old direct runtime.Callers call, the caller’s requested skip is now off by one: skip=1 returns a stack that still starts at the first helper rather than the helper’s caller. This regresses behaviour for any code that relies on skip to remove wrapper functions (e.g. WrapN(err, 1) should report the frame that invoked the helper). The inline comment even says “+4 … and this WrapN function” but only +2 is applied, confirming the mismatch. To preserve existing semantics, the capture should skip WrapN itself as well (skip+3 or skip+4 as documented) so the first frame corresponds to the requested depth.
Useful? React with 👍 / 👎.
Signed-off-by: Kemal Akkoyun [email protected]
Consolidates stacktrace capture and formatting logic into the
internal/stacktracepackage, eliminating duplicate implementations across the codebase.No functional behavior changes are intended. This is purely a refactoring effort.
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!