Repository: getsentry/sentry-go

Issue:
Support for stack frames from xerrors
## Summary

This is related to the xerrors package: https://godoc.org/golang.org/x/xerrors

For high level context, we are still using this in favor of Go 1.13 stdlib because of the stack trace collection that when logged helps developers understand where the errors are coming from.

An issue was raised before in https://github.com/getsentry/sentry-go/issues/137. While I see that the unwrapping an xerror to its original error is working, I do not see stack traces from xerrors working in Sentry.

<!-- A clear and concise description of what the bug is. -->

## Steps To Reproduce
<!--
Good options:
* Inline source code snippet
* Link to Go Playground https://play.golang.org/
* Gist https://gist.github.com
* Link to a public project
-->

```go
func foo() {
	sentry.CaptureException(x1())
}

func x1() error {
	return xerrors.Errorf("%w", x2())
}

func x2() error {
	return xerrors.New("x2msg")
}
```

## Expected Behavior

If you print the extended format of the xerror you see a stack trace like:

```
x2msg:
  grax/x/log.x1
    foo/x/log/error_test.go:84
- x2msg:
  grax/x/log.x2
    foo/x/log/error_test.go:88
```

But if I look at Sentry UI I do not see anything about x2 or x1 in the call stack. See the screenshot below.

## Screenshots

<img width="593" alt="Screen Shot 2020-06-05 at 2 23 03 PM" src="https://user-images.githubusercontent.com/147410/83923918-1a54e580-a738-11ea-9e00-087bf1950b78.png">

## Sentry.io Event
<!-- If applicable, add a link to an event in sentry.io to help explain your problem. -->

## Environment
<!-- Please complete the following information. -->

### SDK
 - `sentry-go` version: `github.com/getsentry/sentry-go v0.6.1`
 - Go version: `go version go1.14.3 darwin/amd64`
 - Using Go Modules? yes

### Sentry
 - Using hosted Sentry in sentry.io? yes

## Additional context

Digging into the `sentry-go` code I see support for getting stack frames from interfaces packages like https://github.com/pkg/errors use. But I do not see anything related to the xerrors interface.

https://github.com/getsentry/sentry-go/blob/master/stacktrace.go#L71

I will attempt to patch this, but wanted to open an issue first to discuss.

Hints from issue comments:
@nzoschke: I haven’t found a good way to get the actual frame data in since xerrors doesn’t offer a public interface to get the data out. 

For now I am copying the expanded xerrors formatting with stack trace into the sentry exception message.  @rhcarvalho: @nzoschke thanks for filing the issue. I confirm I've implemented unwrapping and overlooked the stack trace extraction at the time, my bad.

I'm working on revamping the stack trace extraction and was able to extract the stack from an `xerrors` error using reflection:

```go
field := reflect.ValueOf(err).Elem().FieldByName("frame") // type Frame struct{ frames [3]uintptr }
field = field.FieldByName("frames")
pc := make([]uintptr, field.Len())
for i := 0; i < field.Len(); i++ {
	pc[i] = uintptr(field.Index(i).Uint())
}
```

One thing to notice is that unlike `github.com/go-errors/errors` (max depth 50 frames) and `github.com/pkg/errors` (max depth 32 frames), `golang.org/x/xerrors` retains at most 3 (physical) frames (there can be more logical frames due to inlining).

If all goes well this week I should be able to prepare a patch and submit a PR.

Solution patch:
diff --git a/hub.go b/hub.go
index 60e80b5..fcd9376 100644
--- a/hub.go
+++ b/hub.go
@@ -86,6 +86,9 @@ func CurrentHub() *Hub {
 
 // LastEventID returns an ID of last captured event for the current Hub.
 func (hub *Hub) LastEventID() EventID {
+	hub.mu.RLock()
+	defer hub.mu.RUnlock()
+
 	return hub.lastEventID
 }
 
@@ -222,6 +225,9 @@ func (hub *Hub) CaptureEvent(event *Event) *EventID {
 		return nil
 	}
 	eventID := client.CaptureEvent(event, nil, scope)
+
+	hub.mu.Lock()
+	defer hub.mu.Unlock()
 	if eventID != nil {
 		hub.lastEventID = *eventID
 	} else {
@@ -239,6 +245,9 @@ func (hub *Hub) CaptureMessage(message string) *EventID {
 		return nil
 	}
 	eventID := client.CaptureMessage(message, nil, scope)
+
+	hub.mu.Lock()
+	defer hub.mu.Unlock()
 	if eventID != nil {
 		hub.lastEventID = *eventID
 	} else {
@@ -256,6 +265,9 @@ func (hub *Hub) CaptureException(exception error) *EventID {
 		return nil
 	}
 	eventID := client.CaptureException(exception, &EventHint{OriginalException: exception}, scope)
+
+	hub.mu.Lock()
+	defer hub.mu.Unlock()
 	if eventID != nil {
 		hub.lastEventID = *eventID
 	} else {
diff --git a/integrations.go b/integrations.go
index eb11aa9..1c2f5d6 100644
--- a/integrations.go
+++ b/integrations.go
@@ -70,27 +70,49 @@ func (ei *environmentIntegration) SetupOnce(client *Client) {
 }
 
 func (ei *environmentIntegration) processor(event *Event, hint *EventHint) *Event {
+	// Initialize maps as necessary.
 	if event.Contexts == nil {
 		event.Contexts = make(map[string]interface{})
 	}
-
-	event.Contexts["device"] = map[string]interface{}{
-		"arch":    runtime.GOARCH,
-		"num_cpu": runtime.NumCPU(),
+	for _, name := range []string{"device", "os", "runtime"} {
+		if event.Contexts[name] == nil {
+			event.Contexts[name] = make(map[string]interface{})
+		}
 	}
 
-	event.Contexts["os"] = map[string]interface{}{
-		"name": runtime.GOOS,
+	// Set contextual information preserving existing data. For each context, if
+	// the existing value is not of type map[string]interface{}, then no
+	// additional information is added.
+	if deviceContext, ok := event.Contexts["device"].(map[string]interface{}); ok {
+		if _, ok := deviceContext["arch"]; !ok {
+			deviceContext["arch"] = runtime.GOARCH
+		}
+		if _, ok := deviceContext["num_cpu"]; !ok {
+			deviceContext["num_cpu"] = runtime.NumCPU()
+		}
 	}
-
-	event.Contexts["runtime"] = map[string]interface{}{
-		"name":           "go",
-		"version":        runtime.Version(),
-		"go_numroutines": runtime.NumGoroutine(),
-		"go_maxprocs":    runtime.GOMAXPROCS(0),
-		"go_numcgocalls": runtime.NumCgoCall(),
+	if osContext, ok := event.Contexts["os"].(map[string]interface{}); ok {
+		if _, ok := osContext["name"]; !ok {
+			osContext["name"] = runtime.GOOS
+		}
+	}
+	if runtimeContext, ok := event.Contexts["runtime"].(map[string]interface{}); ok {
+		if _, ok := runtimeContext["name"]; !ok {
+			runtimeContext["name"] = "go"
+		}
+		if _, ok := runtimeContext["version"]; !ok {
+			runtimeContext["version"] = runtime.Version()
+		}
+		if _, ok := runtimeContext["go_numroutines"]; !ok {
+			runtimeContext["go_numroutines"] = runtime.NumGoroutine()
+		}
+		if _, ok := runtimeContext["go_maxprocs"]; !ok {
+			runtimeContext["go_maxprocs"] = runtime.GOMAXPROCS(0)
+		}
+		if _, ok := runtimeContext["go_numcgocalls"]; !ok {
+			runtimeContext["go_numcgocalls"] = runtime.NumCgoCall()
+		}
 	}
-
 	return event
 }
 
diff --git a/stacktrace.go b/stacktrace.go
index c41c986..1c3320f 100644
--- a/stacktrace.go
+++ b/stacktrace.go
@@ -50,11 +50,13 @@ func NewStacktrace() *Stacktrace {
 func ExtractStacktrace(err error) *Stacktrace {
 	method := extractReflectedStacktraceMethod(err)
 
-	if !method.IsValid() {
-		return nil
-	}
+	var pcs []uintptr
 
-	pcs := extractPcs(method)
+	if method.IsValid() {
+		pcs = extractPcs(method)
+	} else {
+		pcs = extractXErrorsPC(err)
+	}
 
 	if len(pcs) == 0 {
 		return nil
@@ -129,6 +131,32 @@ func extractPcs(method reflect.Value) []uintptr {
 	return pcs
 }
 
+// extractXErrorsPC extracts program counters from error values compatible with
+// the error types from golang.org/x/xerrors.
+//
+// It returns nil if err is not compatible with errors from that package or if
+// no program counters are stored in err.
+func extractXErrorsPC(err error) []uintptr {
+	// This implementation uses the reflect package to avoid a hard dependency
+	// on third-party packages.
+
+	// We don't know if err matches the expected type. For simplicity, instead
+	// of trying to account for all possible ways things can go wrong, some
+	// assumptions are made and if they are violated the code will panic. We
+	// recover from any panic and ignore it, returning nil.
+	//nolint: errcheck
+	defer func() { recover() }()
+
+	field := reflect.ValueOf(err).Elem().FieldByName("frame") // type Frame struct{ frames [3]uintptr }
+	field = field.FieldByName("frames")
+	field = field.Slice(1, field.Len()) // drop first pc pointing to xerrors.New
+	pc := make([]uintptr, field.Len())
+	for i := 0; i < field.Len(); i++ {
+		pc[i] = uintptr(field.Index(i).Uint())
+	}
+	return pc
+}
+
 // Frame represents a function call and it's metadata. Frames are associated
 // with a Stacktrace.
 type Frame struct {



Test patch:
diff --git a/integrations_test.go b/integrations_test.go
index 4f130a7..83d2b4a 100644
--- a/integrations_test.go
+++ b/integrations_test.go
@@ -1,6 +1,7 @@
 package sentry
 
 import (
+	"encoding/json"
 	"path/filepath"
 	"regexp"
 	"runtime/debug"
@@ -342,3 +343,48 @@ func TestExtractModules(t *testing.T) {
 		})
 	}
 }
+
+func TestEnvironmentIntegrationDoesNotOverrideExistingContexts(t *testing.T) {
+	transport := &TransportMock{}
+	client, err := NewClient(ClientOptions{
+		Transport: transport,
+		Integrations: func([]Integration) []Integration {
+			return []Integration{new(environmentIntegration)}
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	scope := NewScope()
+
+	scope.contexts["device"] = map[string]interface{}{
+		"foo": "bar",
+	}
+	scope.contexts["os"] = map[string]interface{}{
+		"name": "test",
+	}
+	scope.contexts["custom"] = "value"
+	hub := NewHub(client, scope)
+	hub.CaptureMessage("test event")
+
+	events := transport.Events()
+	if len(events) != 1 {
+		b, err := json.MarshalIndent(events, "", "  ")
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Fatalf("events = %s\ngot %d events, want 1", b, len(events))
+	}
+
+	contexts := events[0].Contexts
+
+	if contexts["device"].(map[string]interface{})["foo"] != "bar" {
+		t.Errorf(`contexts["device"] = %#v, want contexts["device"]["foo"] == "bar"`, contexts["device"])
+	}
+	if contexts["os"].(map[string]interface{})["name"] != "test" {
+		t.Errorf(`contexts["os"] = %#v, want contexts["os"]["name"] == "test"`, contexts["os"])
+	}
+	if contexts["custom"] != "value" {
+		t.Errorf(`contexts["custom"] = %#v, want "value"`, contexts["custom"])
+	}
+}
diff --git a/stacktrace_external_test.go b/stacktrace_external_test.go
index 0c994c3..8cd7f55 100644
--- a/stacktrace_external_test.go
+++ b/stacktrace_external_test.go
@@ -165,6 +165,72 @@ func TestExtractStacktrace(t *testing.T) {
 				},
 			},
 		}},
+		// FIXME: The tests below are commented out to avoid introducing a
+		// dependency on golang.org/x/xerrors. We should enable them when we
+		// move tests with external dependencies to a separate module.
+		// See https://github.com/getsentry/sentry-go/issues/238.
+		//
+		// https://golang.org/x/xerrors
+		// "xerrors.errorString": {
+		// 	func() error {
+		// 		err := xerrors.New("xerror")
+		// 		errType := reflect.TypeOf(err).String()
+		// 		if errType != "*xerrors.errorString" {
+		// 			panic("unexpected error type: " + errType)
+		// 		}
+		// 		return err
+		// 	},
+		// 	&sentry.Stacktrace{
+		// 		Frames: []sentry.Frame{
+		// 			{
+		// 				Function: "TestExtractStacktrace.func1",
+		// 				Module:   "github.com/getsentry/sentry-go_test",
+		// 				Lineno:   178,
+		// 				InApp:    true,
+		// 			},
+		// 		},
+		// 	},
+		// },
+		// "xerrors.wrapError": {
+		// 	func() error {
+		// 		err := xerrors.Errorf("new error: %w", xerrors.New("xerror"))
+		// 		errType := reflect.TypeOf(err).String()
+		// 		if errType != "*xerrors.wrapError" {
+		// 			panic("unexpected error type: " + errType)
+		// 		}
+		// 		return err
+		// 	},
+		// 	&sentry.Stacktrace{
+		// 		Frames: []sentry.Frame{
+		// 			{
+		// 				Function: "TestExtractStacktrace.func2",
+		// 				Module:   "github.com/getsentry/sentry-go_test",
+		// 				Lineno:   198,
+		// 				InApp:    true,
+		// 			},
+		// 		},
+		// 	},
+		// },
+		// "xerrors.noWrapError": {
+		// 	func() error {
+		// 		err := xerrors.Errorf("new error: %w", "not an xerror")
+		// 		errType := reflect.TypeOf(err).String()
+		// 		if errType != "*xerrors.noWrapError" {
+		// 			panic("unexpected error type: " + errType)
+		// 		}
+		// 		return err
+		// 	},
+		// 	&sentry.Stacktrace{
+		// 		Frames: []sentry.Frame{
+		// 			{
+		// 				Function: "TestExtractStacktrace.func3",
+		// 				Module:   "github.com/getsentry/sentry-go_test",
+		// 				Lineno:   218,
+		// 				InApp:    true,
+		// 			},
+		// 		},
+		// 	},
+		// },
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -192,6 +258,10 @@ func TestExtractStacktrace(t *testing.T) {
 func compareStacktrace(t *testing.T, got, want *sentry.Stacktrace) {
 	t.Helper()
 
+	if got == nil {
+		t.Fatal("got nil stack trace")
+	}
+
 	if len(got.Frames) == 0 {
 		t.Fatal("got no frames")
 	}
diff --git a/stacktrace_test.go b/stacktrace_test.go
index c707ec8..33bed05 100644
--- a/stacktrace_test.go
+++ b/stacktrace_test.go
@@ -1,6 +1,7 @@
 package sentry
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -159,3 +160,12 @@ func TestFilterFrames(t *testing.T) {
 		})
 	}
 }
+
+func TestExtractXErrorsPC(t *testing.T) {
+	// This ensures that extractXErrorsPC does not break code that doesn't use
+	// golang.org/x/xerrors. For tests that check that it works on the
+	// appropriate type of errors, see stacktrace_external_test.go.
+	if got := extractXErrorsPC(errors.New("test")); got != nil {
+		t.Errorf("got %#v, want nil", got)
+	}
+}