add support for github.com/pingcap/errors stacktraces

[coder543]

PingCAP's errors package is a fork of pkg/errors that makes a few nice improvements, like being able to call errors.AddStack(err) several times at different levels of the call stack without overwriting any existing stacktrace, since it checks whether one is already associated with the error before capturing and storing a new one.

Their package is not as widely used as pkg/errors, but hopefully this patch is small enough to be merged in!

[coder543]

Actually, I thought this would work, but it looks like CaptureError is using pkgErrors.Cause(err) before calling the GetOrNewStacktrace function, so I'm going to have to investigate this a little further

[kamilogorek]

@coder543 we don't have a dependency on pkgErrors anymore – 238ebd8

[coder543]

That's interesting... I'm just confused as to how that works. Looking here, it's depending on runtime.Frame, but as we can see over here, pkg/errors actually just switched back to using uintptr instead of runtime.Frame for... unclear reasons. So, their StackTrace function shouldn't match the signature you defined in the interface anymore.

runtime.Frame does not appear to be convertible to a uintptr... it's a full struct. But... then right here you're statically casting a runtime.Frame to a uintptr.

Can you give me some background about what is going on with stackframes? I'm happy to see Raven using a more generic solution that should work for pkg/errors and pingcap/errors, but I haven't had a chance to test it, and reading the code has obviously raised a few questions. I'll definitely be planning to try this out on Monday.

[kamilogorek]

cc @mattrobenolt

[mattrobenolt]

@coder543 Yeah, so if this is the case, we need to support a few different interfaces and signatures here. Even if we used the signature directly out of pkg/errors, it's faulty and tied to specific versions.

So the problem with even vendoring, is if your application uses a different version, then our code still won't work since we're still using their interface from a wrong version which won't match the real type. We haven't really regressed in functionality at least.

I'd be open to just piling in a few different interfaces and type switching based on what we get to be fully flexible, and all of this can be done without vendoring in other libraries. We just need to use their interfaces.

[coder543]

I tested the new version of raven-go just now, and it doesn't seem to be noticing the pingcap/errors stacktraces, just FYI.

I also agree that defining those interfaces locally makes sense to provide compatibility against a broad range of versions!

[jayd3e]

Just came across this issue as well. Just to add my situation to the discussion, what I was doing was using runtime.CallersFrames to generate an array of type []runtime.Frame. I was under the impression that I would be able to return this from error.StackTrace(), it would satisfy the stackTracer interface, and the correct frames would be sent to Sentry. This was not the case. The call to uintptr in GetOrNewStacktrace was using the index of the runtime.Frame and not the pointer itself. I had to end up going back to v0.2.0 and returning a manually crafted errors.StackTrace to get this to work. Hope my situation helps #242 along.

[coder543]

@kamilogorek @mattrobenolt any update on this? Just curious! Having real stack traces in Sentry issues would be exceptionally helpful!

[kamilogorek]

@coder543 this feature is also covered in #242
However, right now I'm in a process of rolling out the beta version of https://github.com/getsentry/sentry-go/ (which also has it) and I don't have enough resources to take care of both SDKs at the same time. It be awesome if you could give it a try once it's released (beta should be available this week) and let me know what do you think.
If not, I'll try to get back to this PR once I release the new SDK.