address Alex comments

modules/rpc/handler.go

@@ -28,6 +28,8 @@ import (
 	"go.uber.org/fx/modules/rpc/internal/stats"
 	"go.uber.org/fx/service"
 	"go.uber.org/yarpc/api/transport"
+
+	"github.com/pkg/errors"
 )
 
 const _panicResponse = "Server Error"
@@ -108,7 +110,7 @@ func (p panicOnewayInboundMiddleware) HandleOneway(ctx context.Context, req *tra
 func panicRecovery(ctx context.Context) {
 	if err := recover(); err != nil {
 		stats.RPCPanicCounter.Inc(1)
-		fx.Logger(ctx).Error("Panic recovered serving request", "error", err)
+		fx.Logger(ctx).Error("Panic recovered serving request", "error", errors.Errorf("panic: %+v", err))
 		// rethrow panic back to yarpc
 		// before https://github.com/yarpc/yarpc-go/issues/734 fixed, throw a generic error.
 		panic(_panicResponse)

modules/rpc/handler_test.go

@@ -31,6 +31,7 @@ import (
 	"go.uber.org/yarpc/api/transport"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/uber-go/tally"
 )
 
 type fakeEnveloper struct {
@@ -54,31 +55,31 @@ func TestInboundMiddleware_fxContext(t *testing.T) {
 		Host: service.NopHost(),
 	}
 	stats.SetupRPCMetrics(unary.Host.Metrics())
-	err := unary.Handle(context.Background(), &transport.Request{}, nil, &fakeUnaryHandler{t: t})
+	err := unary.Handle(context.Background(), &transport.Request{}, nil, &fakeUnary{t: t})
 	assert.Equal(t, "handle", err.Error())
 }
 
 func TestOnewayInboundMiddleware_fxContext(t *testing.T) {
 	oneway := contextOnewayInboundMiddleware{
 		Host: service.NopHost(),
 	}
-	err := oneway.HandleOneway(context.Background(), &transport.Request{}, &fakeOnewayHandler{t: t})
+	err := oneway.HandleOneway(context.Background(), &transport.Request{}, &fakeOneway{t: t})
 	assert.Equal(t, "oneway handle", err.Error())
 }
 
 func TestInboundMiddleware_auth(t *testing.T) {
 	unary := authInboundMiddleware{
 		Host: service.NopHost(),
 	}
-	err := unary.Handle(context.Background(), &transport.Request{}, nil, &fakeUnaryHandler{t: t})
+	err := unary.Handle(context.Background(), &transport.Request{}, nil, &fakeUnary{t: t})
 	assert.EqualError(t, err, "handle")
 }
 
 func TestInboundMiddleware_authFailure(t *testing.T) {
 	unary := authInboundMiddleware{
 		Host: service.NopHostAuthFailure(),
 	}
-	err := unary.Handle(context.Background(), &transport.Request{}, nil, &fakeUnaryHandler{t: t})
+	err := unary.Handle(context.Background(), &transport.Request{}, nil, &fakeUnary{t: t})
 	assert.EqualError(t, err, "Error authorizing the service")
 
 }
@@ -87,61 +88,73 @@ func TestOnewayInboundMiddleware_auth(t *testing.T) {
 	oneway := authOnewayInboundMiddleware{
 		Host: service.NopHost(),
 	}
-	err := oneway.HandleOneway(context.Background(), &transport.Request{}, &fakeOnewayHandler{t: t})
+	err := oneway.HandleOneway(context.Background(), &transport.Request{}, &fakeOneway{t: t})
 	assert.EqualError(t, err, "oneway handle")
 }
 
 func TestOnewayInboundMiddleware_authFailure(t *testing.T) {
 	oneway := authOnewayInboundMiddleware{
 		Host: service.NopHostAuthFailure(),
 	}
-	err := oneway.HandleOneway(context.Background(), &transport.Request{}, &fakeOnewayHandler{t: t})
+	err := oneway.HandleOneway(context.Background(), &transport.Request{}, &fakeOneway{t: t})
 	assert.EqualError(t, err, "Error authorizing the service")
 }
 
-type fakeUnaryHandler struct {
-	t *testing.T
+func TestInboundMiddleware_panic(t *testing.T) {
+	host := service.NopHost()
+	testScope := host.Metrics()
+	stats.SetupRPCMetrics(testScope)
+
+	defer testPanicHandler(t, testScope)
+	unary := panicInboundMiddleware{}
+	unary.Handle(context.Background(), &transport.Request{}, nil, &alwaysPanicUnary{})
 }
 
-func (f fakeUnaryHandler) Handle(ctx context.Context, _param1 *transport.Request, _param2 transport.ResponseWriter) error {
-	assert.NotNil(f.t, ctx)
-	return errors.New("handle")
+func TestOnewayInboundMiddleware_panic(t *testing.T) {
+	host := service.NopHost()
+	testScope := host.Metrics()
+	stats.SetupRPCMetrics(testScope)
+
+	defer testPanicHandler(t, testScope)
+	oneway := panicOnewayInboundMiddleware{}
+	oneway.HandleOneway(context.Background(), &transport.Request{}, &alwaysPanicOneway{})
 }
 
-type fakeOnewayHandler struct {
-	t *testing.T
+func testPanicHandler(t *testing.T, testScope tally.Scope) {
+	r := recover()
+	assert.EqualValues(t, r, _panicResponse)
+
+	snapshot := testScope.(tally.TestScope).Snapshot()
+	counters := snapshot.Counters()
+	assert.True(t, counters["panic"].Value() > 0)
 }
 
-func (f fakeOnewayHandler) HandleOneway(ctx context.Context, p *transport.Request) error {
-	assert.NotNil(f.t, ctx)
-	return errors.New("oneway handle")
+type fakeUnary struct {
+	t *testing.T
 }
 
-func TestInboundMiddleware_panic(t *testing.T) {
-	defer testPanicHandler(t)
-	unary := panicInboundMiddleware{}
-	unary.Handle(context.Background(), &transport.Request{}, nil, &panicUnaryHandler{})
+func (f fakeUnary) Handle(ctx context.Context, _param1 *transport.Request, _param2 transport.ResponseWriter) error {
+	assert.NotNil(f.t, ctx)
+	return errors.New("handle")
 }
 
-func TestOnewayInboundMiddleware_panic(t *testing.T) {
-	defer testPanicHandler(t)
-	oneway := panicOnewayInboundMiddleware{}
-	oneway.HandleOneway(context.Background(), &transport.Request{}, &panicOnewayHandler{})
+type fakeOneway struct {
+	t *testing.T
 }
 
-func testPanicHandler(t *testing.T) {
-	r := recover()
-	assert.EqualValues(t, r, _panicResponse)
+func (f fakeOneway) HandleOneway(ctx context.Context, p *transport.Request) error {
+	assert.NotNil(f.t, ctx)
+	return errors.New("oneway handle")
 }
 
-type panicUnaryHandler struct{}
+type alwaysPanicUnary struct{}
 
-func (p panicUnaryHandler) Handle(_ context.Context, _ *transport.Request, _ transport.ResponseWriter) error {
+func (p alwaysPanicUnary) Handle(_ context.Context, _ *transport.Request, _ transport.ResponseWriter) error {
 	panic("panic")
 }
 
-type panicOnewayHandler struct{}
+type alwaysPanicOneway struct{}
 
-func (p panicOnewayHandler) HandleOneway(_ context.Context, _ *transport.Request) error {
+func (p alwaysPanicOneway) HandleOneway(_ context.Context, _ *transport.Request) error {
 	panic("panic")
 }