ddtrace/tracer: add NoDebugStack finishing option (#355)

Adds a new finish option to the tracer which does not cause the generation of a stack trace on errors for performance reasons in some scenarios. Additionally, it adds this as opt-in to the grpc integration. Fixes #354
DataDog · Nov 4, 2018 · 48eeff2 · 48eeff2
1 parent ee63097
commit 48eeff2
Show file tree

Hide file tree

Showing 10 changed files with 67 additions and 22 deletions.
diff --git a/contrib/google.golang.org/grpc/client.go b/contrib/google.golang.org/grpc/client.go
@@ -26,7 +26,7 @@ func (cs *clientStream) RecvMsg(m interface{}) (err error) {
 		if p, ok := peer.FromContext(cs.Context()); ok {
 			setSpanTargetFromPeer(span, *p)
 		}
-		defer finishWithError(span, err)
+		defer finishWithError(span, err, cs.cfg.noDebugStack)
 	}
 	err = cs.ClientStream.RecvMsg(m)
 	return err
@@ -38,7 +38,7 @@ func (cs *clientStream) SendMsg(m interface{}) (err error) {
 		if p, ok := peer.FromContext(cs.Context()); ok {
 			setSpanTargetFromPeer(span, *p)
 		}
-		defer finishWithError(span, err)
+		defer finishWithError(span, err, cs.cfg.noDebugStack)
 	}
 	err = cs.ClientStream.SendMsg(m)
 	return err
@@ -62,7 +62,7 @@ func StreamClientInterceptor(opts ...InterceptorOption) grpc.StreamClientInterce
 					return err
 				})
 			if err != nil {
-				finishWithError(span, err)
+				finishWithError(span, err, cfg.noDebugStack)
 				return nil, err
 			}
 
@@ -74,7 +74,7 @@ func StreamClientInterceptor(opts ...InterceptorOption) grpc.StreamClientInterce
 
 			go func() {
 				<-stream.Context().Done()
-				finishWithError(span, stream.Context().Err())
+				finishWithError(span, stream.Context().Err(), cfg.noDebugStack)
 			}()
 		} else {
 			// if call tracing is disabled, just call streamer, but still return
@@ -111,7 +111,7 @@ func UnaryClientInterceptor(opts ...InterceptorOption) grpc.UnaryClientIntercept
 			func(ctx context.Context, opts []grpc.CallOption) error {
 				return invoker(ctx, method, req, reply, cc, opts...)
 			})
-		finishWithError(span, err)
+		finishWithError(span, err, cfg.noDebugStack)
 		return err
 	}
 }

diff --git a/contrib/google.golang.org/grpc/grpc.go b/contrib/google.golang.org/grpc/grpc.go
@@ -32,11 +32,17 @@ func startSpanFromContext(ctx context.Context, method, operation, service string
 }
 
 // finishWithError applies finish option and a tag with gRPC status code, disregarding OK, EOF and Canceled errors.
-func finishWithError(span ddtrace.Span, err error) {
+func finishWithError(span ddtrace.Span, err error, noDebugStack bool) {
 	errcode := status.Code(err)
 	if err == io.EOF || errcode == codes.Canceled || errcode == codes.OK || err == context.Canceled {
 		err = nil
 	}
 	span.SetTag(tagCode, errcode.String())
-	span.Finish(tracer.WithError(err))
+	finishOptions := []tracer.FinishOption{
+		tracer.WithError(err),
+	}
+	if noDebugStack {
+		finishOptions = append(finishOptions, tracer.NoDebugStack())
+	}
+	span.Finish(finishOptions...)
 }
diff --git a/contrib/google.golang.org/grpc/option.go b/contrib/google.golang.org/grpc/option.go
@@ -3,6 +3,7 @@ package grpc
 type interceptorConfig struct {
 	serviceName                           string
 	traceStreamCalls, traceStreamMessages bool
+	noDebugStack                          bool
 }
 
 func (cfg *interceptorConfig) serverServiceName() string {
@@ -49,3 +50,11 @@ func WithStreamMessages(enabled bool) InterceptorOption {
 		cfg.traceStreamMessages = enabled
 	}
 }
+
+// NoDebugStack disables debug stacks for traces with errors. This is useful in situations
+// where errors are frequent and the overhead of calling debug.Stack may affect performance.
+func NoDebugStack() InterceptorOption {
+	return func(cfg *interceptorConfig) {
+		cfg.noDebugStack = true
+	}
+}
diff --git a/contrib/google.golang.org/grpc/server.go b/contrib/google.golang.org/grpc/server.go
@@ -28,7 +28,7 @@ func (ss *serverStream) Context() context.Context {
 func (ss *serverStream) RecvMsg(m interface{}) (err error) {
 	if ss.cfg.traceStreamMessages {
 		span, _ := startSpanFromContext(ss.ctx, ss.method, "grpc.message", ss.cfg.serverServiceName())
-		defer finishWithError(span, err)
+		defer finishWithError(span, err, ss.cfg.noDebugStack)
 	}
 	err = ss.ServerStream.RecvMsg(m)
 	return err
@@ -37,7 +37,7 @@ func (ss *serverStream) RecvMsg(m interface{}) (err error) {
 func (ss *serverStream) SendMsg(m interface{}) (err error) {
 	if ss.cfg.traceStreamMessages {
 		span, _ := startSpanFromContext(ss.ctx, ss.method, "grpc.message", ss.cfg.serverServiceName())
-		defer finishWithError(span, err)
+		defer finishWithError(span, err, ss.cfg.noDebugStack)
 	}
 	err = ss.ServerStream.SendMsg(m)
 	return err
@@ -60,7 +60,7 @@ func StreamServerInterceptor(opts ...InterceptorOption) grpc.StreamServerInterce
 		if cfg.traceStreamCalls {
 			var span ddtrace.Span
 			span, ctx = startSpanFromContext(ctx, info.FullMethod, "grpc.server", cfg.serviceName)
-			defer finishWithError(span, err)
+			defer finishWithError(span, err, cfg.noDebugStack)
 		}
 
 		// call the original handler with a new stream, which traces each send
@@ -86,7 +86,7 @@ func UnaryServerInterceptor(opts ...InterceptorOption) grpc.UnaryServerIntercept
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		span, ctx := startSpanFromContext(ctx, info.FullMethod, "grpc.server", cfg.serverServiceName())
 		resp, err := handler(ctx, req)
-		finishWithError(span, err)
+		finishWithError(span, err, cfg.noDebugStack)
 		return resp, err
 	}
 }
diff --git a/contrib/mongodb/mongo-go-driver/mongo/example_test.go b/contrib/mongodb/mongo-go-driver/mongo/example_test.go
@@ -5,15 +5,15 @@ import (
 
 	"github.com/mongodb/mongo-go-driver/bson"
 	"github.com/mongodb/mongo-go-driver/mongo"
-	"github.com/mongodb/mongo-go-driver/mongo/clientopt"
+	"github.com/mongodb/mongo-go-driver/options"
 
 	mongotrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/mongodb/mongo-go-driver/mongo"
 )
 
 func Example() {
 	// connect to MongoDB
 	client, err := mongo.Connect(context.Background(), "mongodb://localhost:27017",
-		clientopt.Monitor(mongotrace.NewMonitor()))
+		options.Client().SetMonitor(mongotrace.NewMonitor()))
 	if err != nil {
 		panic(err)
 	}

diff --git a/contrib/mongodb/mongo-go-driver/mongo/mongo_test.go b/contrib/mongodb/mongo-go-driver/mongo/mongo_test.go
@@ -12,7 +12,7 @@ import (
 	"github.com/mongodb/mongo-go-driver/core/result"
 	"github.com/mongodb/mongo-go-driver/core/wiremessage"
 	"github.com/mongodb/mongo-go-driver/mongo"
-	"github.com/mongodb/mongo-go-driver/mongo/clientopt"
+	"github.com/mongodb/mongo-go-driver/options"
 	"github.com/stretchr/testify/assert"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer"
@@ -37,9 +37,7 @@ func Test(t *testing.T) {
 
 	addr := fmt.Sprintf("mongodb://%s", li.Addr().String())
 
-	client, err := mongo.Connect(ctx, addr,
-		clientopt.Single(true),
-		clientopt.Monitor(NewMonitor()))
+	client, err := mongo.Connect(ctx, addr, options.Client().SetSingle(true).SetMonitor(NewMonitor()))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -62,7 +60,7 @@ func Test(t *testing.T) {
 	assert.Equal(t, "mongo.insert", s.Tag(ext.ResourceName))
 	assert.Equal(t, hostname, s.Tag(ext.PeerHostname))
 	assert.Equal(t, port, s.Tag(ext.PeerPort))
-	assert.Contains(t, s.Tag(ext.DBStatement), `{"insert":"test-collection","$db":"test-database","documents":[{"test-item":"test-value","_id":{"`)
+	assert.Contains(t, s.Tag(ext.DBStatement), `{"insert":"test-collection","ordered":true,"$db":"test-database","documents":[{"test-item":"test-value","_id":{"`)
 	assert.Equal(t, "test-database", s.Tag(ext.DBInstance))
 	assert.Equal(t, "mongo", s.Tag(ext.DBType))
 }

diff --git a/ddtrace/ddtrace.go b/ddtrace/ddtrace.go
@@ -88,6 +88,9 @@ type FinishConfig struct {
 	// Error holds an optional error that should be set on the span before
 	// finishing.
 	Error error
+
+	// NoDebugStack will prevent any set errors from generating an attached stack trace tag.
+	NoDebugStack bool
 }
 
 // StartSpanConfig holds the configuration for starting a new span. It is usually passed

diff --git a/ddtrace/tracer/option.go b/ddtrace/tracer/option.go
@@ -161,3 +161,12 @@ func WithError(err error) FinishOption {
 		cfg.Error = err
 	}
 }
+
+// NoDebugStack prevents any error presented using the WithError finishing option
+// from generating a stack trace. This is useful in situations where errors are frequent
+// and performance is critical.
+func NoDebugStack() FinishOption {
+	return func(cfg *ddtrace.FinishConfig) {
+		cfg.NoDebugStack = true
+	}
+}
diff --git a/ddtrace/tracer/span.go b/ddtrace/tracer/span.go
@@ -81,7 +81,7 @@ func (s *span) SetTag(key string, value interface{}) {
 		return
 	}
 	if key == ext.Error {
-		s.setTagError(value)
+		s.setTagError(value, true)
 		return
 	}
 	if v, ok := value.(string); ok {
@@ -99,7 +99,10 @@ func (s *span) SetTag(key string, value interface{}) {
 
 // setTagError sets the error tag. It accounts for various valid scenarios.
 // This method is not safe for concurrent use.
-func (s *span) setTagError(value interface{}) {
+func (s *span) setTagError(value interface{}, debugStack bool) {
+	if s.finished {
+		return
+	}
 	switch v := value.(type) {
 	case bool:
 		// bool value as per Opentracing spec.
@@ -114,7 +117,9 @@ func (s *span) setTagError(value interface{}) {
 		s.Error = 1
 		s.Meta[ext.ErrorMsg] = v.Error()
 		s.Meta[ext.ErrorType] = reflect.TypeOf(v).String()
-		s.Meta[ext.ErrorStack] = string(debug.Stack())
+		if debugStack {
+			s.Meta[ext.ErrorStack] = string(debug.Stack())
+		}
 	case nil:
 		// no error
 		s.Error = 0
@@ -166,7 +171,9 @@ func (s *span) Finish(opts ...ddtrace.FinishOption) {
 		t = cfg.FinishTime.UnixNano()
 	}
 	if cfg.Error != nil {
-		s.SetTag(ext.Error, cfg.Error)
+		s.Lock()
+		s.setTagError(cfg.Error, !cfg.NoDebugStack)
+		s.Unlock()
 	}
 	s.finish(t)
 }

diff --git a/ddtrace/tracer/span_test.go b/ddtrace/tracer/span_test.go
@@ -115,6 +115,19 @@ func TestSpanFinishWithError(t *testing.T) {
 	assert.NotEmpty(span.Meta[ext.ErrorStack])
 }
 
+func TestSpanFinishWithErrorNoDebugStack(t *testing.T) {
+	assert := assert.New(t)
+
+	err := errors.New("test error")
+	span := newBasicSpan("web.request")
+	span.Finish(WithError(err), NoDebugStack())
+
+	assert.Equal(int32(1), span.Error)
+	assert.Equal("test error", span.Meta[ext.ErrorMsg])
+	assert.Equal("*errors.errorString", span.Meta[ext.ErrorType])
+	assert.Empty(span.Meta[ext.ErrorStack])
+}
+
 func TestSpanSetTag(t *testing.T) {
 	assert := assert.New(t)