Add errorsbp.Suppressor

Also update both server and client middlewares in thriftbp to support
it.

errorsbp/BUILD.bazel

@@ -5,6 +5,7 @@ go_library(
     srcs = [
         "batch.go",
         "doc.go",
+        "suppressor.go",
     ],
     importpath = "github.com/reddit/baseplate.go/errorsbp",
     visibility = ["//visibility:public"],
@@ -16,6 +17,8 @@ go_test(
     srcs = [
         "batch_test.go",
         "doc_test.go",
+        "suppressor_test.go",
     ],
     embed = [":go_default_library"],
+    deps = ["//internal/gen-go/reddit/baseplate:go_default_library"],
 )

errorsbp/doc_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 
 	"github.com/reddit/baseplate.go/errorsbp"
+	"github.com/reddit/baseplate.go/internal/gen-go/reddit/baseplate"
 )
 
 func Example() {
@@ -90,3 +91,17 @@ func ExampleBatch_As() {
 	// true
 	// true
 }
+
+// This example demonstrates how to implement Suppressor.
+func ExampleSuppressor() {
+	baseplateErrorSuppressor := func(err error) bool {
+		// baseplate.Error is from a thrift exception defined in baspleate.thrift,
+		// from thrift compiled go code.
+		// We use that type as an example here.
+		// In real code you probably should also add additional exceptions defined in
+		// your thrift files.
+		var baseplateError *baseplate.Error
+		return errors.As(err, &baseplateError)
+	}
+	var _ errorsbp.Suppressor = baseplateErrorSuppressor
+}

errorsbp/suppressor.go

@@ -0,0 +1,33 @@
+package errorsbp
+
+// Suppressor defines a type of function can be used to suppress certain errors.
+type Suppressor func(err error) bool
+
+// Suppress is the nil-safe way of calling a Suppressor.
+func (s Suppressor) Suppress(err error) bool {
+	if s != nil {
+		return s(err)
+	}
+	return SuppressNone(err)
+}
+
+// Wrap wraps the error based on the decision of Suppressor.
+//
+// If the error shall be suppressed, Wrap returns nil.
+// Otherwise Wrap returns the error as-is.
+func (s Suppressor) Wrap(err error) error {
+	if s.Suppress(err) {
+		return nil
+	}
+	return err
+}
+
+// SuppressNone is a Suppressor implementation that always return false,
+// thus suppress none of the errors.
+//
+// It's the default implementation nil Suppressor falls back into.
+func SuppressNone(err error) bool {
+	return false
+}
+
+var _ Suppressor = SuppressNone

errorsbp/suppressor_test.go

@@ -0,0 +1,138 @@
+package errorsbp_test
+
+import (
+	"errors"
+	"fmt"
+	"math/rand"
+	"reflect"
+	"testing"
+	"testing/quick"
+
+	"github.com/reddit/baseplate.go/errorsbp"
+)
+
+func TestSuppressorNil(t *testing.T) {
+	// Test nil safe that it doesn't panic. No real tests here.
+	var s errorsbp.Suppressor
+	s.Suppress(nil)
+	s.Wrap(nil)
+}
+
+type specialError struct{}
+
+func (specialError) Error() string {
+	return "special error"
+}
+
+func specialErrorSuppressor(err error) bool {
+	return errors.As(err, new(specialError))
+}
+
+type randomError struct {
+	err error
+}
+
+func (randomError) Generate(r *rand.Rand, _ int) reflect.Value {
+	var err error
+	if r.Float64() < 0.2 {
+		if r.Float64() < 0.5 {
+			// For 10% (0.2*0.5) of chance, return specialError
+			err = specialError{}
+		}
+		// For the rest 10%, return nil error
+	} else {
+		// For the rest 80%, use a random error
+		err = fmt.Errorf("random error: %d", r.Int63())
+	}
+	return reflect.ValueOf(randomError{
+		err: err,
+	})
+}
+
+var (
+	_ errorsbp.Suppressor = specialErrorSuppressor
+	_ quick.Generator     = randomError{}
+)
+
+func TestSuppressor(t *testing.T) {
+	t.Run(
+		"SuppressNone",
+		func(t *testing.T) {
+			var s errorsbp.Suppressor
+
+			t.Run(
+				"Suppress",
+				func(t *testing.T) {
+					f := func(e randomError) bool {
+						return !s.Suppress(e.err)
+					}
+					if err := quick.Check(f, nil); err != nil {
+						t.Error(err)
+					}
+				},
+			)
+
+			t.Run(
+				"Wrap",
+				func(t *testing.T) {
+					f := func(e randomError) bool {
+						wrapped := s.Wrap(e.err)
+						if wrapped != e.err {
+							t.Errorf("Expected unchanged error %v, got %v", e.err, wrapped)
+						}
+						return !t.Failed()
+					}
+					if err := quick.Check(f, nil); err != nil {
+						t.Error(err)
+					}
+				},
+			)
+		},
+	)
+
+	t.Run(
+		"specialErrorSuppressor",
+		func(t *testing.T) {
+			var s errorsbp.Suppressor = specialErrorSuppressor
+
+			t.Run(
+				"Suppress",
+				func(t *testing.T) {
+					f := func(e randomError) bool {
+						err := e.err
+						expected := errors.As(err, new(specialError))
+						actual := s.Suppress(err)
+						if actual != expected {
+							t.Errorf("Expected %v for err %v, got %v", expected, err, actual)
+						}
+						return !t.Failed()
+					}
+					if err := quick.Check(f, nil); err != nil {
+						t.Error(err)
+					}
+				},
+			)
+
+			t.Run(
+				"Wrap",
+				func(t *testing.T) {
+					f := func(e randomError) bool {
+						err := e.err
+						expected := err
+						if errors.As(err, new(specialError)) {
+							expected = nil
+						}
+						actual := s.Wrap(err)
+						if actual != expected {
+							t.Errorf("Expected %v for error %v, got %v", expected, err, actual)
+						}
+						return !t.Failed()
+					}
+					if err := quick.Check(f, nil); err != nil {
+						t.Error(err)
+					}
+				},
+			)
+		},
+	)
+}

thriftbp/BUILD.bazel

@@ -20,6 +20,7 @@ go_library(
         "//:go_default_library",
         "//clientpool:go_default_library",
         "//edgecontext:go_default_library",
+        "//errorsbp:go_default_library",
         "//internal/gen-go/reddit/baseplate:go_default_library",
         "//log:go_default_library",
         "//metricsbp:go_default_library",

thriftbp/client_middlewares.go

@@ -10,13 +10,14 @@ import (
 	opentracing "github.com/opentracing/opentracing-go"
 
 	"github.com/reddit/baseplate.go/edgecontext"
+	"github.com/reddit/baseplate.go/errorsbp"
 	"github.com/reddit/baseplate.go/retrybp"
 	"github.com/reddit/baseplate.go/tracing"
 )
 
 // MonitorClientWrappedSlugSuffix is a suffix to be added to the service slug
 // arg of MonitorClient function, in order to distinguish from the spans that
-// are the raw client calls..
+// are the raw client calls.
 //
 // The MonitorClient with this suffix will have span operation names like:
 //
@@ -67,6 +68,14 @@ type DefaultClientMiddlewareArgs struct {
 	// automatically retry any requests.  You can set retry behavior per-call by
 	// using retrybp.WithOptions.
 	RetryOptions []retry.Option
+
+	// Suppress some of the errors returned by the server before sending them to
+	// the client span.
+	//
+	// See MonitorClientArgs.ErrorSpanSuppressor for more details.
+	//
+	// This is optional. If it's not set none of the errors will be suppressed.
+	ErrorSpanSuppressor errorsbp.Suppressor
 }
 
 // BaseplateDefaultClientMiddlewares returns the default client middlewares that
@@ -93,21 +102,50 @@ func BaseplateDefaultClientMiddlewares(args DefaultClientMiddlewareArgs) []thrif
 	}
 	return []thrift.ClientMiddleware{
 		ForwardEdgeRequestContext,
-		MonitorClient(args.ServiceSlug + MonitorClientWrappedSlugSuffix),
+		MonitorClient(MonitorClientArgs{
+			ServiceSlug:         args.ServiceSlug + MonitorClientWrappedSlugSuffix,
+			ErrorSpanSuppressor: args.ErrorSpanSuppressor,
+		}),
 		Retry(args.RetryOptions...),
-		MonitorClient(args.ServiceSlug),
+		MonitorClient(MonitorClientArgs{
+			ServiceSlug:         args.ServiceSlug,
+			ErrorSpanSuppressor: args.ErrorSpanSuppressor,
+		}),
 		SetDeadlineBudget,
 	}
 }
 
+// MonitorClientArgs are the args to be passed into MonitorClient function.
+type MonitorClientArgs struct {
+	// The slug string of the service.
+	//
+	// Note that if this is the MonitorClient before retry,
+	// ServiceSlug should also come with MonitorClientWrappedSlugSuffix.
+	ServiceSlug string
+
+	// Suppress some of the errors returned by the server before sending them to
+	// the client span.
+	//
+	// Based on Baseplate spec, the errors defined in the server's thrift IDL are
+	// not treated as errors, and should be suppressed here. So in most cases
+	// that's what should be implemented as the Suppressor here.
+	//
+	// Note that this suppressor only affects the errors send to the span. It
+	// won't affect the errors returned to the caller of the client function.
+	//
+	// This is optional. If it's not set none of the errors will be suppressed.
+	ErrorSpanSuppressor errorsbp.Suppressor
+}
+
 // MonitorClient is a ClientMiddleware that wraps the inner thrift.TClient.Call
 // in a thrift client span.
 //
 // If you are using a thrift ClientPool created by NewBaseplateClientPool,
 // this will be included automatically and should not be passed in as a
 // ClientMiddleware to NewBaseplateClientPool.
-func MonitorClient(service string) thrift.ClientMiddleware {
-	prefix := service + "."
+func MonitorClient(args MonitorClientArgs) thrift.ClientMiddleware {
+	prefix := args.ServiceSlug + "."
+	s := args.ErrorSpanSuppressor
 	return func(next thrift.TClient) thrift.TClient {
 		return thrift.WrappedTClient{
 			Wrapped: func(ctx context.Context, method string, args, result thrift.TStruct) (err error) {
@@ -120,7 +158,7 @@ func MonitorClient(service string) thrift.ClientMiddleware {
 				defer func() {
 					span.FinishWithOptions(tracing.FinishOptions{
 						Ctx: ctx,
-						Err: err,
+						Err: s.Wrap(err),
 					}.Convert())
 				}()
 

thriftbp/client_middlewares_test.go

@@ -352,6 +352,7 @@ func TestRetry(t *testing.T) {
 	server, err := thrifttest.NewBaseplateServer(
 		store,
 		processor,
+		nil,
 		thrifttest.ServerConfig{
 			ClientConfig: thriftbp.ClientPoolConfig{
 				DefaultRetryOptions: []retry.Option{

thriftbp/example_client_test.go

@@ -31,7 +31,9 @@ func ExampleMonitorClient() {
 				factory.GetProtocol(transport),
 				factory.GetProtocol(transport),
 			),
-			thriftbp.MonitorClient("service"),
+			thriftbp.MonitorClient(thriftbp.MonitorClientArgs{
+				ServiceSlug: "service",
+			}),
 		),
 	)
 	// Create a context with a server span

thriftbp/example_server_test.go

@@ -23,7 +23,7 @@ func ExampleNewBaseplateServer() {
 	var handler bpgen.BaseplateService
 	processor := bpgen.NewBaseplateServiceProcessor(handler)
 
-	server, err := thriftbp.NewBaseplateServer(bp, processor)
+	server, err := thriftbp.NewBaseplateServer(bp, processor, nil)
 	if err != nil {
 		log.Fatal(err)
 	}

thriftbp/server.go

@@ -6,6 +6,7 @@ import (
 	"github.com/apache/thrift/lib/go/thrift"
 
 	baseplate "github.com/reddit/baseplate.go"
+	"github.com/reddit/baseplate.go/errorsbp"
 	"github.com/reddit/baseplate.go/log"
 )
 
@@ -69,17 +70,25 @@ func NewServer(
 //
 // The TProcessor underlying the server will be wrapped in the default
 // Baseplate Middleware and any additional middleware passed in.
+//
+// The suppressor arg will be used as
+// DefaultProcessorMiddlewaresArgs.ErrorSpanSuppressor,
+// please refer to its documentation for more details.
 func NewBaseplateServer(
 	bp baseplate.Baseplate,
 	processor thrift.TProcessor,
+	suppressor errorsbp.Suppressor,
 	middlewares ...thrift.ProcessorMiddleware,
 ) (baseplate.Server, error) {
 	cfg := ServerConfig{
 		Addr:    bp.Config().Addr,
 		Timeout: bp.Config().Timeout,
 		Logger:  thrift.Logger(log.ZapWrapper(bp.Config().Log.Level)),
 	}
-	wrapped := BaseplateDefaultProcessorMiddlewares(bp.EdgeContextImpl())
+	wrapped := BaseplateDefaultProcessorMiddlewares(DefaultProcessorMiddlewaresArgs{
+		EdgeContextImpl:     bp.EdgeContextImpl(),
+		ErrorSpanSuppressor: suppressor,
+	})
 	wrapped = append(wrapped, middlewares...)
 	srv, err := NewServer(cfg, processor, wrapped...)
 	if err != nil {