Repository: rollbar/rollbar-go
Pull request #66 (2019-09-30 23:41:12)

Wrap functions with arguments, and add Rollbar to defer/recover functions
`Wrap` and `WrapAndWait` are convenient for reporting unhandled panics in wrapped functions, but until now could only wrap functions with no arguments and no return values. This PR allows any function signature to be used. It also exposes a new helper, `LogPanic`, that can be used to add Rollbar reporting to any defer/recover function.

Since wrapping a function in main() won't recover panics in goroutines, and since many programs do use goroutines liberally, this extra flexibility should help make it easier to report panics wherever needed.

The updated `Wrap` and `WrapAndWait` are fully compatible with previous/existing behavior, accepting `func()` just fine, and the existing behavior of returning the error is preserved.

While `Wrap` and `WrapAndWait` can now accept functions with any return signature, they do not return the function's return value. In cases where this is needed, the `LogPanic` helper can be used to add Rollbar to the function's defer/recover handler, instead of using any wrapper. Using `LogPanic` makes the resulting defer function simple and compact:

```
func ExampleFunction() {
  defer func() {
    err := recover()
    rollbar.LogPanic(err, true) // bool argument sets wait behavior
  }()

  // function body ...
}
```

An even more streamlined `defer RollbarRecover()` helper was considered, but `recover()` only works correctly when in a closure of the containing function, not when the defer function is a separate non-closure function.


Patch:
diff --git a/client.go b/client.go
index 189a13f..84b4f45 100644
--- a/client.go
+++ b/client.go
@@ -464,60 +464,77 @@ func (c *Client) RequestMessageWithExtrasAndContext(ctx context.Context, level s
 
 // -- Panics
 
-// Wrap calls f and then recovers and reports a panic to Rollbar if it occurs.
-// If an error is captured it is subsequently returned
-func (c *Client) Wrap(f func()) (err interface{}) {
-	defer func() {
-		err = recover()
-		switch val := err.(type) {
-		case nil:
+// LogPanic accepts an error value returned by recover() and
+// handles logging to Rollbar with stack info.
+func (c *Client) LogPanic(err interface{}, wait bool) {
+	switch val := err.(type) {
+	case nil:
+		return
+	case error:
+		if c.configuration.checkIgnore(val.Error()) {
 			return
-		case error:
-			if c.configuration.checkIgnore(val.Error()) {
-				return
-			}
-			c.ErrorWithStackSkip(CRIT, val, 2)
-		default:
-			str := fmt.Sprint(val)
-			if c.configuration.checkIgnore(str) {
-				return
-			}
-			errValue := errors.New(str)
-			c.ErrorWithStackSkip(CRIT, errValue, 2)
 		}
-	}()
+		c.ErrorWithStackSkip(CRIT, val, 2)
+	default:
+		str := fmt.Sprint(val)
+		if c.configuration.checkIgnore(str) {
+			return
+		}
+		errValue := errors.New(str)
+		c.ErrorWithStackSkip(CRIT, errValue, 2)
+	}
+	if wait {
+		c.Wait()
+	}
+}
+
+// WrapWithArgs calls f with the supplied args and reports a panic to Rollbar if it occurs.
+// If wait is true, this also waits before returning to ensure the message was reported.
+// If an error is captured it is subsequently returned.
+// WrapWithArgs is compatible with any return type for f, but does not return its return value(s).
+func (c *Client) WrapWithArgs(f interface{}, wait bool, inArgs ...interface{}) (err interface{}) {
+	if f == nil {
+		err = fmt.Errorf("function is nil")
+		return
+	}
+	funcType := reflect.TypeOf(f)
+	funcValue := reflect.ValueOf(f)
+
+	if funcType.Kind() != reflect.Func {
+		err = fmt.Errorf("function kind %s is not %s", funcType.Kind(), reflect.Func)
+		return
+	}
+
+	argValues := make([]reflect.Value, len(inArgs))
+	for i, v := range inArgs {
+		argValues[i] = reflect.ValueOf(v)
+	}
+
+	handler := func(args []reflect.Value) []reflect.Value {
+		defer func() {
+			err = recover()
+			c.LogPanic(err, wait)
+		}()
+
+		return funcValue.Call(args)
+	}
+
+	handler(argValues)
 
-	f()
 	return
 }
 
+// Wrap calls f and then recovers and reports a panic to Rollbar if it occurs.
+// If an error is captured it is subsequently returned.
+func (c *Client) Wrap(f interface{}, args ...interface{}) (err interface{}) {
+	return c.WrapWithArgs(f, false, args...)
+}
+
 // WrapAndWait calls f, and recovers and reports a panic to Rollbar if it occurs.
 // This also waits before returning to ensure the message was reported
 // If an error is captured it is subsequently returned.
-func (c *Client) WrapAndWait(f func()) (err interface{}) {
-	defer func() {
-		err = recover()
-		switch val := err.(type) {
-		case nil:
-			return
-		case error:
-			if c.configuration.checkIgnore(val.Error()) {
-				return
-			}
-			c.ErrorWithStackSkip(CRIT, val, 2)
-		default:
-			str := fmt.Sprint(val)
-			if c.configuration.checkIgnore(str) {
-				return
-			}
-			errValue := errors.New(str)
-			c.ErrorWithStackSkip(CRIT, errValue, 2)
-		}
-		c.Wait()
-	}()
-
-	f()
-	return
+func (c *Client) WrapAndWait(f interface{}, args ...interface{}) (err interface{}) {
+	return c.WrapWithArgs(f, true, args...)
 }
 
 // LambdaWrapper calls handlerFunc with arguments, and recovers and reports a
@@ -537,23 +554,7 @@ func (c *Client) LambdaWrapper(handlerFunc interface{}) interface{} {
 	handler := func(args []reflect.Value) []reflect.Value {
 		defer func() {
 			err := recover()
-			switch val := err.(type) {
-			case nil:
-				return
-			case error:
-				if c.configuration.checkIgnore(val.Error()) {
-					return
-				}
-				c.ErrorWithStackSkip(CRIT, val, 2)
-			default:
-				str := fmt.Sprint(val)
-				if c.configuration.checkIgnore(str) {
-					return
-				}
-				errValue := errors.New(str)
-				c.ErrorWithStackSkip(CRIT, errValue, 2)
-			}
-			c.Wait()
+			c.LogPanic(err, true)
 		}()
 
 		ret := handlerValue.Call(args)
diff --git a/doc.go b/doc.go
index fb681c0..2346753 100644
--- a/doc.go
+++ b/doc.go
@@ -34,7 +34,7 @@ We provide two implementations of the `Transport` interface, `AsyncTransport` an
 
 Handling Panics
 
-Go does not provide a mechanism for handling all panics automatically, therefore we provide two functions `Wrap` and `WrapAndWait` to make working with panics easier. They both take a function and then report to Rollbar if that function panics. They use the recover mechanism to capture the panic, and therefore if you wish your process to have the normal behaviour on panic (i.e. to crash), you will need to re-panic the result of calling `Wrap`. For example,
+Go does not provide a mechanism for handling all panics automatically, therefore we provide two functions `Wrap` and `WrapAndWait` to make working with panics easier. They both take a function with arguments and then report to Rollbar if that function panics. They use the recover mechanism to capture the panic, and therefore if you wish your process to have the normal behaviour on panic (i.e. to crash), you will need to re-panic the result of calling `Wrap`. For example,
 
   package main
 
@@ -43,13 +43,14 @@ Go does not provide a mechanism for handling all panics automatically, therefore
     "github.com/rollbar/rollbar-go"
   )
 
-  func PanickyFunction() {
+  func PanickyFunction(arg string) string {
     panic(errors.New("AHHH!!!!"))
+    return arg
   }
 
   func main() {
     rollbar.SetToken("MY_TOKEN")
-    err := rollbar.Wrap(PanickyFunction)
+    err := rollbar.Wrap(PanickyFunction, "function arg1")
     if err != nil {
       // This means our function panic'd
       // Uncomment the next line to get normal
@@ -61,6 +62,20 @@ Go does not provide a mechanism for handling all panics automatically, therefore
 
 The above pattern of calling `Wrap(...)` and then `Wait(...)` can be combined via `WrapAndWait(...)`. When `WrapAndWait(...)` returns if there was a panic it has already been sent to the Rollbar API. The error is still returned by this function if there is one.
 
+`Wrap` and `WrapAndWait` will accept functions with any number and type of arguments and return values. However, they do not return the function's return value, instead returning the error value. To add Rollbar panic handling to a function while preserving access to the function's return values, we provide the `LogPanic` helper designed to be used inside your deferred function.
+
+  func PanickyFunction(arg string) string {
+    defer func() {
+      err := recover()
+      rollbar.LogPanic(err, true) // bool argument sets wait behavior
+    }()
+
+    panic(errors.New("AHHH!!!!"))
+    return arg
+  }
+
+This offers virtually the same functionality as `Wrap` and `WrapAndWait` while preserving access to the function return values.
+
 Tracing Errors
 
 Due to the nature of the `error` type in Go, it can be difficult to attribute errors to their original origin without doing some extra work. To account for this, we provide multiple ways of configuring the client to unwrap errors and extract stack traces.
diff --git a/rollbar.go b/rollbar.go
index 1fa734a..33e8e2b 100644
--- a/rollbar.go
+++ b/rollbar.go
@@ -588,17 +588,31 @@ func Close() {
 	std.Close()
 }
 
+// LogPanic accepts an error value returned by recover() and
+// handles logging to Rollbar with stack info.
+func LogPanic(err interface{}, wait bool) {
+	std.LogPanic(err, wait)
+}
+
+// WrapWithArgs calls f with the supplied args and reports a panic to Rollbar if it occurs.
+// If wait is true, this also waits before returning to ensure the message was reported.
+// If an error is captured it is subsequently returned.
+// WrapWithArgs is compatible with any return type for f, but does not return its return value(s).
+func WrapWithArgs(f interface{}, wait bool, args ...interface{}) interface{} {
+	return std.WrapWithArgs(f, wait, args...)
+}
+
 // Wrap calls f and then recovers and reports a panic to Rollbar if it occurs.
 // If an error is captured it is subsequently returned.
-func Wrap(f func()) interface{} {
-	return std.Wrap(f)
+func Wrap(f interface{}, args ...interface{}) interface{} {
+	return std.WrapWithArgs(f, false, args...)
 }
 
 // WrapAndWait calls f, and recovers and reports a panic to Rollbar if it occurs.
 // This also waits before returning to ensure the message was reported.
 // If an error is captured it is subsequently returned.
-func WrapAndWait(f func()) interface{} {
-	return std.WrapAndWait(f)
+func WrapAndWait(f interface{}, args ...interface{}) interface{} {
+	return std.WrapWithArgs(f, true, args...)
 }
 
 // LambdaWrapper calls handlerFunc with arguments, and recovers and reports a


Test patch:
diff --git a/client_test.go b/client_test.go
index 82453ae..7eaf3eb 100644
--- a/client_test.go
+++ b/client_test.go
@@ -8,6 +8,7 @@ import (
 	"reflect"
 	"testing"
 	"strings"
+	"fmt"
 )
 
 type TestTransport struct {
@@ -39,6 +40,28 @@ func testClient() *rollbar.Client {
 	return c
 }
 
+func TestLogPanic(t *testing.T) {
+	client := testClient()
+	client.LogPanic(errors.New("logged error"), false)
+	if transport, ok := client.Transport.(*TestTransport); ok {
+		if transport.WaitCalled {
+			t.Error("Wait called unexpectedly")
+		}
+		body := transport.Body
+		if body["data"] == nil {
+			t.Error("body should have data")
+		}
+		data := body["data"].(map[string]interface{})
+		dataError := errorFromData(data)
+		if dataError["message"] != "logged error" {
+			t.Error("data should have correct error message")
+		}
+	} else {
+		t.Fail()
+	}
+	client.Close()
+}
+
 func TestWrap(t *testing.T) {
 	client := testClient()
 	err := errors.New("bork")
@@ -58,6 +81,33 @@ func TestWrap(t *testing.T) {
 	client.Close()
 }
 
+func TestWrapWithArgs(t *testing.T) {
+	client := testClient()
+	result := client.Wrap(func(foo string, num int) (string, int) {
+		panic(fmt.Errorf("%v-%v", foo, num))
+	}, "foo", 42)
+	if fmt.Sprintf("%T", result) != "*errors.errorString" {
+		t.Error("Return value should be error type")
+	}
+	if transport, ok := client.Transport.(*TestTransport); ok {
+		if transport.WaitCalled {
+			t.Error("Wait called unexpectedly")
+		}
+		body := transport.Body
+		if body["data"] == nil {
+			t.Error("body should have data")
+		}
+		data := body["data"].(map[string]interface{})
+		dataError := errorFromData(data)
+		if dataError["message"] != "foo-42" {
+			t.Error("data should have correct error message")
+		}
+	} else {
+		t.Fail()
+	}
+	client.Close()
+}
+
 func TestWrapNonError(t *testing.T) {
 	client := testClient()
 	err := "hello rollbar"
@@ -583,3 +633,9 @@ func configuredOptionsFromData(data map[string]interface{}) map[string]interface
 	configuredOptions := diagnostic["configuredOptions"].(map[string]interface{})
 	return configuredOptions
 }
+
+func errorFromData(data map[string]interface{}) map[string]interface{} {
+	body := data["body"].(map[string]interface{})
+	traceChain := body["trace_chain"].([]map[string]interface{})
+	return traceChain[0]["exception"].(map[string]interface{})
+}