go-stack / stack

Package stack implements utilities to capture, manipulate, and format call stacks.
MIT License
395 stars 33 forks source link

Allow access to the underlying uintptr and *runtime.Func #2

Closed teepark closed 8 years ago

teepark commented 8 years ago

Maybe make Call.fn and Call.pc exported (uppercase), or create access methods for them.

If another third-party package makes use of either of those (the stdlib representations of a stack frame), we currently don't have access if the stack was collected with this package.

My specific use-case is that I'm adding sentry error reporing to a project that already uses erroneous (error wrapper that carries a stack trace with it). I want to send the deeper, more accurate stack trace that erroneous collected to Sentry, but it's held as a stack.CallStack, meaning I can't get the uintptr that sentry wants.

ChrisHines commented 8 years ago

I would like to support your request, but I also want to be careful about how we do it because there is new functionality in this area coming in the next release of Go.

See https://github.com/golang/go/commit/ad03af66ebfb368fe0f87262092094e1793a9ef5.

I have not yet evaluated how the addition of these new APIs in the stdlib affects this package going forward, especially the addition of the runtime.Frame struct, which is logically equivalent to the stack.Call type. I would be happy for your input if you have suggestions.

teepark commented 8 years ago

Ah I was unaware of that. It looks great -- the corner cases are a pain when working with frames as uintptr (even being documented).

Still, fwiw implementing fmt.Formatter was a stroke of genius, I hope this package remains maintained.

There shouldn't be a need to change anything in this package though, is there? The backwards compatibility promise should protect the existing uintptr and Func, quirks and all.

ChrisHines commented 8 years ago

This package will remain maintained. My concern is that if we export Call.pc and Call.fn then we are stuck with the current implementation. But if the guts of type stack.Call are not exported then we could change the implementation to take advantage of the new APIs for Go 1.7+. For example maybe stack.Call changes to:

type Call struct {
    frame runtime.Frame
}

But I don't know if that is a good idea yet. Also consider that the new APIs are intended to support stack traces that traverse non-Go code and inlined functions, both of which will have a nil *runtime.Func. I foresee the implementation of this package getting reworked to use the new APIs and benefit from improved meta-data available from them.

Either way, I think exporting some access methods leaves more wiggle room than exporting the fields directly. I'll try to put some thought into it and create a PR for you to review sometime soon.

teepark commented 8 years ago

Thank you, and that's great to hear!

Frame still exports the uintptr and Func, so accessor methods should be doable even if this package changes post Go1.7.

It would be kind of a pain to maintain both though, and that would obviously be necessary to still support 1.6 and earlier.

ChrisHines commented 8 years ago

@teepark Would you be happy with only exposing the PC value for a Call? It looks like that is all you need.

Regarding maintenance, this package is pretty stable. I don't expect many code changes for Go versions <= 1.6. For Go versions >= 1.7 reworking the internals of this package to use the new frames APIs makes sense. Proper build tags should do the trick without impacting downstream clients.

teepark commented 8 years ago

@ChrisHines Yes that'd do it -- I still need the func to get file and line number, but I can get to all that via the pc uintptr.

Currently I'm re-parsing a formatted string from stack, so anything is welcome :)

ChrisHines commented 8 years ago

A Call.PC accessor method was added in the just released v1.5.0. The change was made in 77bcacc96980fce0f3df0fbecfae14b9dc626e8d.