use runtime.Callers + runtime.CallersFrames

[toshok]

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]

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]

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]

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]

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]

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]

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]

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

👍 I can do that.

[toshok]

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:

1. for the 17 versions, they have // +build go1.7
2. 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.

[dcramer]

Was looking at getting this merged, but realizing that we likely also have a new behavior change we have to address in Go 1.11 with modules. Either that or I've forgotten all things about Go.

$ go test
--- FAIL: TestFunctionName (0.00s)
    stacktrace17_test.go:35: incorrect package; got _/Users/dcramer/Development/raven-go, want .
--- FAIL: TestStacktrace (0.00s)
    stacktrace17_test.go:63: incorrect Module: _/Users/dcramer/Development/raven-go
    stacktrace17_test.go:80: expected InApp to be true
FAIL

[dcramer]

(looks like not specific to 1.11, probably just specific to my local env)

[sjung-stripe]

@dcramer are you still interested in getting this landed? we have a rebased fork master...stripe:sjung/rebase-187 that integrates this PR with the already-merged #207

[dcramer]

@sjung-stripe yep let me see if I can't get the rebased patch merged today

[dcramer]

So I think our only concern here is this breaks Go 1.2 to 1.6. I don't know how the Go ecosystem works these days, or if there's some expectation of support. If there's statistics that show the low volume of usage I'm fine with it (and we'll note in README accordingly), but otherwise we might have to maintain compatibility with the older versions via a separate path of code.

[sjung-stripe]

go1.9 and older are all end of life https://golang.org/doc/devel/release.html#policy but you're the maintainer so i'm willing to defer to you on this subject. do you want to continue supporting 1.6 and older? i can adjust my patch if so, it's just a minor annoyance

[dcramer]

I'm fine with bumping the minimum version in the README. I can't find any obvious source of Go usage statistics, and I dont think I can reliably query that from sentry.io's dataset unfortunately.

[sjung-stripe]

👍 lemme bump that and open a new PR with it

[dcramer]

@sjung-stripe sounds good - mind just bumping to 1.7 since tests still pass there?

[dcramer]

I've also tagged v0.1.1 which includes this change