From 0886a74dc2bf68f71f8acf58e9394c55086f7ce0 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Wed, 27 May 2020 13:16:26 +0200 Subject: [PATCH] fix: Set either Frame.Filename or Frame.AbsPath Despite the name "filename", the field is supposed to contain "The path to the source file relative to the project root directory." [1] Setting Frame.Filename to filepath.Base(FrameAbsPath) is a problem because it creates ambiguous paths and may affect the "Suspect Commits" feature. Depending on how the binary is built, the Go runtime knows either the absolute path or the relative path of source files. In the general case, it can be difficult to derive a correct "project-relative" path. Code built using Go Modules can be anywhere in the file system (not only under GOPATH). GOPATH can be multiple paths. There was an attempt to trim paths in raven-go [2], but the approach there relies on GOROOT and GOPATH as configured for "future builds", and does not reflect how the running binary has been built, thus not being a good general case solution. For now we set either Filename or AbsPath depending on what is known, and we leave figuring out a relative path from an absolute path for a later improvement. [1] https://develop.sentry.dev/sdk/event-payloads/stacktrace#frame-attributes [2] https://github.com/getsentry/raven-go/blob/5c24d5110e0e198d9ae16f1f3465366085001d92/stacktrace.go#L141 --- stacktrace.go | 42 +++++++++++++++++++++++++------------ stacktrace_external_test.go | 33 +++++++++++------------------ 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/stacktrace.go b/stacktrace.go index 037d5bb48..c41c98629 100644 --- a/stacktrace.go +++ b/stacktrace.go @@ -149,20 +149,36 @@ type Frame struct { // NewFrame assembles a stacktrace frame out of runtime.Frame. func NewFrame(f runtime.Frame) Frame { - abspath := f.File - filename := f.File - function := f.Function - var pkg string - - if filename != "" { - filename = filepath.Base(filename) - } else { - filename = unknown + var abspath, relpath string + // NOTE: f.File paths historically use forward slash as path separator even + // on Windows, though this is not yet documented, see + // https://golang.org/issues/3335. In any case, filepath.IsAbs can work with + // paths with either slash or backslash on Windows. + switch { + case f.File == "": + relpath = unknown + // Leave abspath as the empty string to be omitted when serializing + // event as JSON. + abspath = "" + case filepath.IsAbs(f.File): + abspath = f.File + // TODO: in the general case, it is not trivial to come up with a + // "project relative" path with the data we have in run time. + // We shall not use filepath.Base because it creates ambiguous paths and + // affects the "Suspect Commits" feature. + // For now, leave relpath empty to be omitted when serializing the event + // as JSON. Improve this later. + relpath = "" + default: + // f.File is a relative path. This may happen when the binary is built + // with the -trimpath flag. + relpath = f.File + // Omit abspath when serializing the event as JSON. + abspath = "" } - if abspath == "" { - abspath = unknown - } + function := f.Function + var pkg string if function != "" { pkg, function = splitQualifiedFunctionName(function) @@ -170,7 +186,7 @@ func NewFrame(f runtime.Frame) Frame { frame := Frame{ AbsPath: abspath, - Filename: filename, + Filename: relpath, Lineno: f.Line, Module: pkg, Function: function, diff --git a/stacktrace_external_test.go b/stacktrace_external_test.go index 788b86beb..0c994c3d6 100644 --- a/stacktrace_external_test.go +++ b/stacktrace_external_test.go @@ -60,8 +60,6 @@ func TestNewStacktrace(t *testing.T) { { Function: "f1", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 18, InApp: true, }, @@ -72,16 +70,12 @@ func TestNewStacktrace(t *testing.T) { { Function: "f2", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 22, InApp: true, }, { Function: "f1", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 18, InApp: true, }, @@ -97,8 +91,6 @@ func TestNewStacktrace(t *testing.T) { { Function: "f3", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 25, InApp: true, }, @@ -128,16 +120,12 @@ func TestExtractStacktrace(t *testing.T) { { Function: "RedPkgErrorsRanger", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 29, InApp: true, }, { Function: "BluePkgErrorsRanger", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 33, InApp: true, }, @@ -149,16 +137,12 @@ func TestExtractStacktrace(t *testing.T) { { Function: "RedPingcapErrorsRanger", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 37, InApp: true, }, { Function: "BluePingcapErrorsRanger", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 41, InApp: true, }, @@ -170,16 +154,12 @@ func TestExtractStacktrace(t *testing.T) { { Function: "RedGoErrorsRanger", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 45, InApp: true, }, { Function: "BlueGoErrorsRanger", Module: "github.com/getsentry/sentry-go_test", - Filename: "stacktrace_external_test.go", - AbsPath: "ignored", Lineno: 49, InApp: true, }, @@ -194,6 +174,17 @@ func TestExtractStacktrace(t *testing.T) { } got := sentry.ExtractStacktrace(err) compareStacktrace(t, got, tt.want) + // We ignore paths in compareStacktrace because they depend on the + // environment where tests are run. However, Frame.Filename should + // be a relative path and Frame.AbsPath should be an absolute path. + for _, frame := range got.Frames { + if !filepath.IsAbs(frame.AbsPath) { + t.Errorf("got %q, want absolute path", frame.AbsPath) + } + if filepath.IsAbs(frame.Filename) { + t.Errorf("got %q, want relative path", frame.Filename) + } + } }) } } @@ -226,6 +217,6 @@ func compareStacktrace(t *testing.T, got, want *sentry.Stacktrace) { func stacktraceDiff(x, y *sentry.Stacktrace) string { return cmp.Diff( x, y, - cmpopts.IgnoreFields(sentry.Frame{}, "AbsPath"), + cmpopts.IgnoreFields(sentry.Frame{}, "AbsPath", "Filename"), ) }