ztrue / tracerr

Golang errors with stack trace and source fragments.
MIT License
993 stars 41 forks source link

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

Open DevX86 opened 10 months ago

DevX86 commented 10 months ago

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 commented 9 months ago

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 commented 9 months ago

Awesome! Thank you @ztrue

fabien-marty commented 5 months ago

👋 just implemented something about that in this commit

@ztrue interested in a pull-request?

ztrue commented 4 months ago

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 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 commented 4 months ago

@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"
    [...]
ztrue commented 4 months ago

The path suffix github.com/ztrue/tracerr is the name of the package. Might be worth changing to just /tracerr/error_test.go separately from this change.

fabien-marty commented 4 months ago

just opened a specific issue for unit tests not passing out of the box: https://github.com/ztrue/tracerr/issues/12

DevX86 commented 3 months ago

This is awesome guys!