fix(stacktrace): Correctly report package names and ignore subpackages

stacktrace.go

@@ -149,7 +149,7 @@ func NewFrame(f runtime.Frame) Frame {
 	abspath := f.File
 	filename := f.File
 	function := f.Function
-	var module string
+	var pkg string
 
 	if filename != "" {
 		filename = filepath.Base(filename)
@@ -162,14 +162,14 @@ func NewFrame(f runtime.Frame) Frame {
 	}
 
 	if function != "" {
-		module, function = deconstructFunctionName(function)
+		pkg, function = splitQualifiedFunctionName(function)
 	}
 
 	frame := Frame{
 		AbsPath:  abspath,
 		Filename: filename,
 		Lineno:   f.Line,
-		Module:   module,
+		Module:   pkg,
 		Function: function,
 	}
 
@@ -178,6 +178,15 @@ func NewFrame(f runtime.Frame) Frame {
 	return frame
 }
 
+// splitQualifiedFunctionName splits a package path-qualified function name into
+// package name and function name. Such qualified names are found in
+// runtime.Frame.Function values.
+func splitQualifiedFunctionName(name string) (pkg string, fun string) {
+	pkg = packageName(name)
+	fun = strings.TrimPrefix(name, pkg+".")
+	return
+}
+
 func extractFrames(pcs []uintptr) []Frame {
 	var frames []Frame
 	callersFrames := runtime.CallersFrames(pcs)
@@ -197,17 +206,24 @@ func extractFrames(pcs []uintptr) []Frame {
 	return frames
 }
 
+// filterFrames filters out stack frames that are not meant to be reported to
+// Sentry. Those are frames internal to the SDK or Go.
 func filterFrames(frames []Frame) []Frame {
+	if len(frames) == 0 {
+		return nil
+	}
+
 	filteredFrames := make([]Frame, 0, len(frames))
 
 	for _, frame := range frames {
-		// go runtime frames
+		// Skip Go internal frames.
 		if frame.Module == "runtime" || frame.Module == "testing" {
 			continue
 		}
-		// sentry internal frames
-		if frame.Module == "github.com/getsentry/sentry-go" ||
-			strings.HasPrefix(frame.Module, "github.com/getsentry/sentry-go.") {
+		// Skip Sentry internal frames, except for frames in _test packages (for
+		// testing).
+		if strings.HasPrefix(frame.Module, "github.com/getsentry/sentry-go") &&
+			!strings.HasSuffix(frame.Module, "_test") {
 			continue
 		}
 		filteredFrames = append(filteredFrames, frame)
@@ -226,21 +242,42 @@ func isInAppFrame(frame Frame) bool {
 	return true
 }
 
-// Transform `runtime/debug.*T·ptrmethod` into `{ module: runtime/debug, function: *T.ptrmethod }`
-func deconstructFunctionName(name string) (module string, function string) {
-	if idx := strings.LastIndex(name, "."); idx != -1 {
-		module = name[:idx]
-		function = name[idx+1:]
-	}
-	function = strings.Replace(function, "·", ".", -1)
-	return module, function
-}
-
 func callerFunctionName() string {
 	pcs := make([]uintptr, 1)
 	runtime.Callers(3, pcs)
 	callersFrames := runtime.CallersFrames(pcs)
 	callerFrame, _ := callersFrames.Next()
-	_, function := deconstructFunctionName(callerFrame.Function)
-	return function
+	return baseName(callerFrame.Function)
+}
+
+// packageName returns the package part of the symbol name, or the empty string
+// if there is none.
+// It replicates https://golang.org/pkg/debug/gosym/#Sym.PackageName, avoiding a
+// dependency on debug/gosym.
+func packageName(name string) string {
+	// A prefix of "type." and "go." is a compiler-generated symbol that doesn't belong to any package.
+	// See variable reservedimports in cmd/compile/internal/gc/subr.go
+	if strings.HasPrefix(name, "go.") || strings.HasPrefix(name, "type.") {
+		return ""
+	}
+
+	pathend := strings.LastIndex(name, "/")
+	if pathend < 0 {
+		pathend = 0
+	}
+
+	if i := strings.Index(name[pathend:], "."); i != -1 {
+		return name[:pathend+i]
+	}
+	return ""
+}
+
+// baseName returns the symbol name without the package or receiver name.
+// It replicates https://golang.org/pkg/debug/gosym/#Sym.BaseName, avoiding a
+// dependency on debug/gosym.
+func baseName(name string) string {
+	if i := strings.LastIndex(name, "."); i != -1 {
+		return name[i+1:]
+	}
+	return name
 }

stacktrace_test.go

@@ -1,8 +1,9 @@
 package sentry
 
 import (
-	"runtime"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func NewStacktraceForTest() *Stacktrace {
@@ -15,28 +16,146 @@ func (StacktraceTestHelper) NewStacktrace() *Stacktrace {
 	return NewStacktrace()
 }
 
-func TestFunctionName(t *testing.T) {
-	for _, test := range []struct {
-		skip int
-		pack string
-		name string
+func BenchmarkNewStacktrace(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		Trace()
+	}
+}
+
+//nolint: scopelint // false positive https://github.com/kyoh86/scopelint/issues/4
+func TestSplitQualifiedFunctionName(t *testing.T) {
+	tests := []struct {
+		in  string
+		pkg string
+		fun string
 	}{
-		{0, "sentry-go", "TestFunctionName"},
-		{1, "testing", "tRunner"},
-		{2, "runtime", "goexit"},
-		{100, "", ""},
-	} {
-		pc, _, _, _ := runtime.Caller(test.skip)
-		pack, name := deconstructFunctionName(runtime.FuncForPC(pc).Name())
-
-		// Go1.10 reports different paths than Modules aware versions (>=1.11)
-		assertStringContains(t, pack, test.pack)
-		assertEqual(t, name, test.name)
+		{"", "", ""},
+		{"runtime.Callers", "runtime", "Callers"},
+		{"main.main.func1", "main", "main.func1"},
+		{
+			"github.com/getsentry/sentry-go.Init",
+			"github.com/getsentry/sentry-go",
+			"Init",
+		},
+		{
+			"github.com/getsentry/sentry-go.(*Hub).Flush",
+			"github.com/getsentry/sentry-go",
+			"(*Hub).Flush",
+		},
+		{
+			"github.com/getsentry/sentry-go.Test.func2.1.1",
+			"github.com/getsentry/sentry-go",
+			"Test.func2.1.1",
+		},
+		{
+			"github.com/getsentry/confusing%2epkg%2ewith%2edots.Test.func1",
+			"github.com/getsentry/confusing%2epkg%2ewith%2edots",
+			"Test.func1",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.in, func(t *testing.T) {
+			pkg, fun := splitQualifiedFunctionName(tt.in)
+			if diff := cmp.Diff(tt.pkg, pkg); diff != "" {
+				t.Errorf("Package name mismatch (-want +got):\n%s", diff)
+			}
+			if diff := cmp.Diff(tt.fun, fun); diff != "" {
+				t.Errorf("Function name mismatch (-want +got):\n%s", diff)
+			}
+		})
 	}
 }
 
-func BenchmarkNewStacktrace(b *testing.B) {
-	for i := 0; i < b.N; i++ {
-		Trace()
+//nolint: scopelint // false positive https://github.com/kyoh86/scopelint/issues/4
+func TestFilterFrames(t *testing.T) {
+	tests := []struct {
+		in  []Frame
+		out []Frame
+	}{
+		// sanity check
+		{},
+		// filter out go internals and SDK internals; "sentry-go_test" is
+		// considered outside of the SDK and thus included (useful for testing)
+		{
+			in: []Frame{
+				{
+					Function: "goexit",
+					Module:   "runtime",
+					AbsPath:  "/goroot/src/runtime/asm_amd64.s",
+					InApp:    false,
+				},
+				{
+					Function: "tRunner",
+					Module:   "testing",
+					AbsPath:  "/goroot/src/testing/testing.go",
+					InApp:    false,
+				},
+				{
+					Function: "TestNewStacktrace.func1",
+					Module:   "github.com/getsentry/sentry-go_test",
+					AbsPath:  "/somewhere/sentry/sentry-go/stacktrace_external_test.go",
+					InApp:    true,
+				},
+				{
+					Function: "StacktraceTestHelper.NewStacktrace",
+					Module:   "github.com/getsentry/sentry-go",
+					AbsPath:  "/somewhere/sentry/sentry-go/stacktrace_test.go",
+					InApp:    true,
+				},
+				{
+					Function: "NewStacktrace",
+					Module:   "github.com/getsentry/sentry-go",
+					AbsPath:  "/somewhere/sentry/sentry-go/stacktrace.go",
+					InApp:    true,
+				},
+			},
+			out: []Frame{
+				{
+					Function: "TestNewStacktrace.func1",
+					Module:   "github.com/getsentry/sentry-go_test",
+					AbsPath:  "/somewhere/sentry/sentry-go/stacktrace_external_test.go",
+					InApp:    true,
+				},
+			},
+		},
+		// filter out integrations; SDK subpackages
+		{
+			in: []Frame{
+				{
+					Function: "Example.Integration",
+					Module:   "github.com/getsentry/sentry-go/http/integration",
+					AbsPath:  "/somewhere/sentry/sentry-go/http/integration/integration.go",
+					InApp:    true,
+				},
+				{
+					Function: "(*Handler).Handle",
+					Module:   "github.com/getsentry/sentry-go/http",
+					AbsPath:  "/somewhere/sentry/sentry-go/http/sentryhttp.go",
+					InApp:    true,
+				},
+				{
+					Function: "main",
+					Module:   "main",
+					AbsPath:  "/somewhere/example.com/pkg/main.go",
+					InApp:    true,
+				},
+			},
+			out: []Frame{
+				{
+					Function: "main",
+					Module:   "main",
+					AbsPath:  "/somewhere/example.com/pkg/main.go",
+					InApp:    true,
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run("", func(t *testing.T) {
+			got := filterFrames(tt.in)
+			if diff := cmp.Diff(tt.out, got); diff != "" {
+				t.Errorf("filterFrames() mismatch (-want +got):\n%s", diff)
+			}
+		})
 	}
 }