Skip to content
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

[Feature Request]: Print last n frames. #11

Open
DevX86 opened this issue Dec 19, 2023 · 5 comments
Open

[Feature Request]: Print last n frames. #11

DevX86 opened this issue Dec 19, 2023 · 5 comments

Comments

@DevX86
Copy link

DevX86 commented Dec 19, 2023

Incredible project! Such a beautiful night and day difference.

Currently, we can control the amount of lines before and after the trace line in methods such as:

tracerr.SprintSourceColor(beforeLines, afterLines)

Is would be incredibly helpful if we could also specify the amount of stack frames printed. For example:

tracerr.SprintSourceColor(beforeLines, afterLines, stackFrameLimit)

Or some similar signature, getting everything every trace down to the runtime assembly is rather cool but in the majority of cases you only want the last 3-4 stack frames in any given trace for debugging. This will reduce output in terminal and show only the level of granularity we need.

Another potential attack vector which would potentially help performance:

Stack trace causes a performance overhead, depending on a stack trace depth

This could potentially be mitigated if an environmental variable such as STACK_FRAME_LIMIT was added to control the max stack length big-endian style. Only the last n stack frames would be tracked, and as a frame is added, the earliest frame is popped off. This would reduce the stack trace depth and potentially decrease performance overhead on deep traces.

@ztrue
Copy link
Owner

ztrue commented Dec 26, 2023

Hi @DevX86, thanks for the feedback and for pointing out this problem; it makes a lot of sense to me.
I was not quite sure about adding an extra parameter to functions because it can be a bit challenging to follow all those optional arguments, which is actually the issue with the initial design, not the proposal. Yet, I believe it will be beneficial for the reasons you described.
Regarding the package-level frame limit, I'd prefer going with a package-level variable, similar to DefaultCap. I think it will be consistent with the existing design and easy to use.
I'll try to address this in the near future.

@DevX86
Copy link
Author

DevX86 commented Dec 27, 2023

Awesome! Thank you @ztrue

fabien-marty added a commit to fabien-marty/tracerr that referenced this issue May 21, 2024
implemented in the way discussed in upstream issue:
ztrue#11
@fabien-marty
Copy link

👋 just implemented something about that in this commit

@ztrue interested in a pull-request?

@ztrue
Copy link
Owner

ztrue commented May 24, 2024

Sorry for not getting back to this issue in time. @fabien-marty, thank you for the link! Overall the change looks right and you are welcome to make a PR. There are some minor adjustments I would consider:

  • i can be reused from the loop itself, e.g. by replacing this line for _, frame := range frames { with for i, frame := range frames {, some <=, etc will be replaced with just < too.
  • I believe the appendedFrames counter is redundant, because DefaultIgnoreFirstFrames is a constant, and appendedFrames can be calculated at any point as i - DefaultIgnoreFirstFrames. If we are at frame 3 (starting with 0), and if we skip 2 frames, it means 1 frame was appended so far.

I know unit tests in this project are a bit of a mess, but this kind of change would require to have unit tests as well.
You can address these comments or create PR as is if you prefer (in this case I can make those adjustments when testing).

@fabien-marty
Copy link

fabien-marty commented May 27, 2024

@ztrue thanks for your review => I'm going to make a PR with requested changes

for unit tests, I totally agree with you but they don't run on my env because of hardcoded paths somewhere on your own :) maybe we should fix that... before (in another PR)

=== RUN   TestError
    error_test.go:181: cases[2].Error.StackTrace()[0].Path = "/Users/fab/src/tracerr/error_test.go"; want to has suffix "/src/github.com/ztrue/tracerr/error_test.go"
    [...]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants