use runtime.Callers + runtime.CallersFrames

toshok commented 6 years ago

Go's runtime package includes this comment about not using PCs directly:

// To translate these PCs into symbolic information such as function
// names and line numbers, use CallersFrames. CallersFrames accounts
// for inlined functions and adjusts the return program counters into
// call program counters. Iterating over the returned slice of PCs
// directly is discouraged, as is using FuncForPC on any of the
// returned PCs, since these cannot account for inlining or return
// program counter adjustment.

Convert NewStacktrace over to using runtime.Callers and runtime.CallersFrames instead of looping over PCs. Fixes #186

toshok commented 6 years ago

there's also a similar "loop over PCs calling runtime.FuncForPC" block in GetOrNewStacktrace which required changing as well. I set fName to "unknown" as well, but maybe it should be ""?

toshok commented 6 years ago

looks like runtime.CallersFrames wasn't added until go 1.7.

any guidance on what you think should happen here to fix older builds? I'm guessing splitting things up until < 1.7 and >= 1.7 files with build flags?

mattrobenolt commented 6 years ago

I personally don't know how to do that, but I'm open to the idea. I'm also open to the idea of just dropping < 1.7 support, not sure if that's even relevant anymore.

akshayjshah commented 6 years ago

Dropping support for 1.7 is completely reasonable, since the Go team doesn't even support it. Going forward, it probably makes sense to match the Go team's support policy.

dataBaseError commented 6 years ago

These fixes work for me Originally I received: Discarded invalid value for parameter 'exception' With a payload that contained:

"stacktrace":{
      "frames":[
         {
            "in_app":false
         },
         ...
    ]
}

With the PR the frames were correctly produced and used by sentry.

mattrobenolt commented 6 years ago

Alright, following up here:

It's been a bit, but I think this is a good change that I'd like to get landed. But we have some conflicts against master. Do you want to get this rebased against master, and I'll get this merged?

Not sure if we need to mention the version change, or just let it happen and see it people complain. Since we don't really have a versioned API or anything.

toshok commented 6 years ago

Do you want to get this rebased against master, and I'll get this merged?

👍 I can do that.

toshok commented 6 years ago

final commit moves the new versions of stacktrace.go/stacktrace_test.go to stacktrace17.go and stacktrace17_test.go, and keeps the original stacktrace.go/stacktrace_test.go from master.

all four files gain build flags:

for the 17 versions, they have // +build go1.7
for the non-17 versions, they have // +build !go1.7

go1.7 means >= 1.7. 1.7 was the version that apparently introduced the api.

I mostly just wanted a green build - this is one way to keep things working across all versions.

I can remove the commit if you'd rather drop support for go < 1.7.