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

proposal: tools for more readable stacktraces #25736

Open
kingishb opened this issue Jun 5, 2018 · 8 comments
Open

proposal: tools for more readable stacktraces #25736

kingishb opened this issue Jun 5, 2018 · 8 comments

Comments

@kingishb
Copy link

@kingishb kingishb commented Jun 5, 2018

As a Go beginner, I've benefitted a lot from using visual aides to assist reading dense panics and stacktraces. In particular, @maruel's panicparse has been helpful for reading panics while I work (even though the information is basically the same). Example from the project's README:
screenshot from 2018-06-05 11-01-43
screenshot from 2018-06-05 11-01-55

This style of tool/output seems like it could be useful in the Go standard distribution; as a command line tool in the family of debugging tools, as a toggleable flag somewhere to pretty-print panics, or maybe as inspiration for some kind of dense tab-delimited panic format.

@gopherbot gopherbot added this to the Proposal milestone Jun 5, 2018
@gopherbot gopherbot added the Proposal label Jun 5, 2018
@maruel
Copy link
Contributor

@maruel maruel commented Jun 5, 2018

Thanks for the reference. :) Some of the reason I didn't push this into the stdlib is that panicparse does a fair amount of processing to:

  • Read the source files and figure out the types on the stack*
  • Find similar goroutines and merge them into a common signature
  • Use colored outputs

All these cannot be safely done from a panic handler inside the panicking process, as the amount of processing must be kept to a minimum, which includes trying to even allocate memory.

  • This could probably be figured out by parsing the binary itself, but I never tried. It'd be much smarter.
@mvdan mvdan added the Go2 label Jun 5, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 25, 2018

Sounds like a good idea, but this can be done independently of any Go 2 effort, so removing the Go 2 label. This should start as a third-party or golang.org/x tool, and then we can see whether it makes sense in the Go standard library. Putting this proposal on hold until there is a tool available.

@ianlancetaylor ianlancetaylor changed the title proposal: Go 2: tools for more readable stacktraces proposal: tools for more readable stacktraces Sep 25, 2018
@ianlancetaylor ianlancetaylor added Proposal-Hold and removed Go2 labels Sep 25, 2018
@maruel
Copy link
Contributor

@maruel maruel commented Aug 31, 2020

An update.

I recently made a v2 of the parsing library. It's based on a relatively well designed state machine (in context.go) and has an API I took a fair amount of time to design; https://pkg.go.dev/github.com/maruel/panicparse/v2/stack

If Go folks think there's value in putting this in golang.org/x/tools, I'm fine with upstreaming, albeit the CLI tool uses a handful of external packages (for colored console text output) that would have to be redone; https://github.com/maruel/panicparse/blob/master/go.mod

@ianthehat
Copy link

@ianthehat ianthehat commented Sep 2, 2020

Note that x/tools does already have some of this functionality.
golang.org/x/tools/internal/stack
Primarily it was written to detect and produce useful reports on go routine leaks in tests (see golang.org/x/tools/internal/stack/stacktest)
I suspect there are some strong equivalences between that package and yours, although you have probably put far more thought and time into your design (and probably handle more stack forms). I would personally be happy to see that package improved and then made public.
I don't think we want to add a bunch of extra dependancies to x/tools, but there is already precedent for having individual binaries (in separate nested modules) with extra dependancies, so if they are just for the reprinting tools that could still work.

@maruel
Copy link
Contributor

@maruel maruel commented Sep 2, 2020

Panicparse is both a library and a tool. I'd like to discuss each separately.

When I did v2, I thought about going for the Scanner flow you chose to but decided against to keep the API surface smaller. The returned []byte is a bit archaic but is easy to manage in practice and I can accept any io.Reader, which I find nicer for the user. See the Stream example. I decided against Process() because I didn't want to force people on what to do with the discovered stacks. I'm surprised NewScanner is exported, since it's not used outside of the package. I guess it doesn't matter since it's all under internal/ anyway.

My stack package only depends on the stdlib, so it's safe to import if you see it desirable. The important point is that they are providing different functionalities, mine has no Diff equivalent, it's optimized for bucketing, not for deltas. Mine is fairly tested against corrupted inputs or mangling, like indentation, UTF-8 characters in path or symbols (it's fun!), or unexpected joins. It has a concept of "remoteness" for paths as I think the most practical use is to analyze stack traces coming from a server, and in this situation the paths do not necessary match. Mine provide source analysis; I find it useful in practice but it has its costs, hence it can be disabled. There's also htmlstack and webstack to provide visualization which I think people found useful.

The tool's only external dependencies are relative to colored output. panicparse uses colors to convey information in the CLI. Only github.com/mattn/go-colorable and github.com/mattn/go-isatty are needed. I think panicparse (as a CLI tool) would be less useful without the possibility to privode color coded information.

I don't think (?) there are precedent to do colored output in go tooling at all? That's a separate discussion as to whether this should be allowed. I'm not privy of any prior discussion of this on the team. Personally I think most external devs are used to tooling provided some level of colored output in an reasonable accessible manner for color blind people and that's something I'd welcome. But that should be tracked as a separate issue, and I would consider this a blocker to import pp as a CLI tool, but not the package stack.

Side note: The fact that github.com/maruel/panicparse is both a library and a tool is unfortunate. When I made v2 for the refactor of package "stack", I had to tell users to fetch the v2 of the tool. In hindsight, I think I should have put the library in a different repository than the tool; as the v2 didn't break the tool, only incrementally improved it. Lesson learned...

@ptman
Copy link

@ptman ptman commented Sep 2, 2020

I don't think (?) there are precedent to do colored output in go tooling at all

There's color in e.g. the HTML output by cover

@maruel
Copy link
Contributor

@maruel maruel commented Sep 2, 2020

@ptman I mean for TUI (text user interface) / CLI (command line interface) tools specifically. I found a few references:

  • cmd/pprof/readlineui.go uses cmd/vendor/golang.org/x/crypto/ssh/terminal/terminal.go
  • cmd/dist/test_linux.go
  • cmd/go/testdata/testterminal18153/terminal_test.go

@ianthehat can you vouch if this subtopic is worth discussing, and if so, I think it should be in a separate more focused issue.

@ianthehat
Copy link

@ianthehat ianthehat commented Sep 2, 2020

Let's discuss the library for now, and then the tool(s) if we manage to end up with a library we are happy with.
I think the next step is to think what the API of that package should be, and what its exact responsibilities are.
If you want we should try to iterate on the package currently in x/tools (it is internal, so we are free to change its API in incompatible ways for now). Once we think it is stable, we can propose moving it out of internal and making it a supported stable API. I don't think we need to ever move it to the standard library though.
Your parser handles far more cases and is probably more resilient, the diff stuff in x/tools is incredibly useful (for more than just leak tests), so I think it is clear we need to take some features from each, but from a cursory look I think it should be viable to design a common ground. At the very least we could share the parser (which is the most complex part anyway, and the bit most likely to need to change with go version) and layer the other parts on top.
We should probably move this discussion to a different medium though, issues are a terrible place to hash out an API design.

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

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.