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

@@ -149,28 +149,44 @@ 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)
 	}
 
 	frame := Frame{
 		AbsPath:  abspath,
-		Filename: filename,
+		Filename: relpath,
 		Lineno:   f.Line,
 		Module:   pkg,
 		Function: function,

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"),
 	)
 }