From 08a7e75e8c48a08da39a16eff3346e2995198a5d Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Mon, 12 Jun 2023 19:33:06 +0900 Subject: [PATCH 01/12] Add non error func option for grpc --- contrib/google.golang.org/grpc/grpc.go | 2 +- contrib/google.golang.org/grpc/option.go | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/contrib/google.golang.org/grpc/grpc.go b/contrib/google.golang.org/grpc/grpc.go index ff3ff7f79e..a185a2cab1 100644 --- a/contrib/google.golang.org/grpc/grpc.go +++ b/contrib/google.golang.org/grpc/grpc.go @@ -78,7 +78,7 @@ func finishWithError(span ddtrace.Span, err error, cfg *config) { err = nil } errcode := status.Code(err) - if errcode == codes.OK || cfg.nonErrorCodes[errcode] { + if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.nonErrorFunc == nil || cfg.nonErrorFunc(err)) { err = nil } span.SetTag(tagCode, errcode.String()) diff --git a/contrib/google.golang.org/grpc/option.go b/contrib/google.golang.org/grpc/option.go index 0cc59246a6..87f002dca2 100644 --- a/contrib/google.golang.org/grpc/option.go +++ b/contrib/google.golang.org/grpc/option.go @@ -27,6 +27,7 @@ type config struct { serviceName string spanName string nonErrorCodes map[codes.Code]bool + nonErrorFunc func(error) bool traceStreamCalls bool traceStreamMessages bool noDebugStack bool @@ -116,6 +117,15 @@ func NonErrorCodes(cs ...codes.Code) InterceptorOption { } } +// NonErrorFunc sets a custom function to determine whether an error should not be considered as an error for tracing purposes. +// This function is evaluated when an error occurs, and if it returns true, the error will not be recorded in the trace. +// f: A function that takes an error as an argument and returns a boolean indicating whether the error should be ignored. +func NonErrorFunc(f func(error) bool) InterceptorOption { + return func(cfg *config) { + cfg.nonErrorFunc = f + } +} + // WithAnalytics enables Trace Analytics for all started spans. func WithAnalytics(on bool) Option { return func(cfg *config) { From 252e2753945f4ca3be82cd9440b47c30072990dd Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 13 Jun 2023 01:46:22 +0900 Subject: [PATCH 02/12] Add FullMethod string for `nonErrorFunc` --- contrib/google.golang.org/grpc/client.go | 10 +++++----- contrib/google.golang.org/grpc/grpc.go | 7 +++++-- contrib/google.golang.org/grpc/option.go | 4 ++-- contrib/google.golang.org/grpc/server.go | 8 ++++---- contrib/google.golang.org/grpc/stats_client.go | 10 +++++++++- contrib/google.golang.org/grpc/stats_server.go | 11 ++++++++++- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/contrib/google.golang.org/grpc/client.go b/contrib/google.golang.org/grpc/client.go index f0e769c3f1..b28fa8ac55 100644 --- a/contrib/google.golang.org/grpc/client.go +++ b/contrib/google.golang.org/grpc/client.go @@ -45,7 +45,7 @@ func (cs *clientStream) RecvMsg(m interface{}) (err error) { if p, ok := peer.FromContext(cs.Context()); ok { setSpanTargetFromPeer(span, *p) } - defer func() { finishWithError(span, err, cs.cfg) }() + defer func() { finishWithError(span, err, cs.method, cs.cfg) }() } err = cs.ClientStream.RecvMsg(m) return err @@ -64,7 +64,7 @@ func (cs *clientStream) SendMsg(m interface{}) (err error) { if p, ok := peer.FromContext(cs.Context()); ok { setSpanTargetFromPeer(span, *p) } - defer func() { finishWithError(span, err, cs.cfg) }() + defer func() { finishWithError(span, err, cs.method, cs.cfg) }() } err = cs.ClientStream.SendMsg(m) return err @@ -104,7 +104,7 @@ func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { return err }) if err != nil { - finishWithError(span, err, cfg) + finishWithError(span, err, method, cfg) return nil, err } @@ -116,7 +116,7 @@ func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { go func() { <-stream.Context().Done() - finishWithError(span, stream.Context().Err(), cfg) + finishWithError(span, stream.Context().Err(), method, cfg) }() } else { // if call tracing is disabled, just call streamer, but still return @@ -158,7 +158,7 @@ func UnaryClientInterceptor(opts ...Option) grpc.UnaryClientInterceptor { func(ctx context.Context, opts []grpc.CallOption) error { return invoker(ctx, method, req, reply, cc, opts...) }) - finishWithError(span, err, cfg) + finishWithError(span, err, method, cfg) return err } } diff --git a/contrib/google.golang.org/grpc/grpc.go b/contrib/google.golang.org/grpc/grpc.go index a185a2cab1..3e40821eeb 100644 --- a/contrib/google.golang.org/grpc/grpc.go +++ b/contrib/google.golang.org/grpc/grpc.go @@ -34,6 +34,8 @@ func init() { // cache a constant option: saves one allocation per call var spanTypeRPC = tracer.SpanType(ext.AppTypeRPC) +type fullMethodNameKey struct{} + func (cfg *config) startSpanOptions(opts ...tracer.StartSpanOption) []tracer.StartSpanOption { if len(cfg.tags) == 0 && len(cfg.spanOpts) == 0 { return opts @@ -69,16 +71,17 @@ func startSpanFromContext( if sctx, err := tracer.Extract(grpcutil.MDCarrier(md)); err == nil { opts = append(opts, tracer.ChildOf(sctx)) } + ctx = context.WithValue(ctx, fullMethodNameKey{}, method) return tracer.StartSpanFromContext(ctx, operation, opts...) } // finishWithError applies finish option and a tag with gRPC status code, disregarding OK, EOF and Canceled errors. -func finishWithError(span ddtrace.Span, err error, cfg *config) { +func finishWithError(span ddtrace.Span, err error, method string, cfg *config) { if errors.Is(err, io.EOF) || errors.Is(err, context.Canceled) { err = nil } errcode := status.Code(err) - if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.nonErrorFunc == nil || cfg.nonErrorFunc(err)) { + if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.nonErrorFunc == nil || cfg.nonErrorFunc(method, err)) { err = nil } span.SetTag(tagCode, errcode.String()) diff --git a/contrib/google.golang.org/grpc/option.go b/contrib/google.golang.org/grpc/option.go index 87f002dca2..d338dbf310 100644 --- a/contrib/google.golang.org/grpc/option.go +++ b/contrib/google.golang.org/grpc/option.go @@ -27,7 +27,7 @@ type config struct { serviceName string spanName string nonErrorCodes map[codes.Code]bool - nonErrorFunc func(error) bool + nonErrorFunc func(method string, err error) bool traceStreamCalls bool traceStreamMessages bool noDebugStack bool @@ -120,7 +120,7 @@ func NonErrorCodes(cs ...codes.Code) InterceptorOption { // NonErrorFunc sets a custom function to determine whether an error should not be considered as an error for tracing purposes. // This function is evaluated when an error occurs, and if it returns true, the error will not be recorded in the trace. // f: A function that takes an error as an argument and returns a boolean indicating whether the error should be ignored. -func NonErrorFunc(f func(error) bool) InterceptorOption { +func NonErrorFunc(f func(method string, err error) bool) InterceptorOption { return func(cfg *config) { cfg.nonErrorFunc = f } diff --git a/contrib/google.golang.org/grpc/server.go b/contrib/google.golang.org/grpc/server.go index fab33e4c73..f887d2d4c4 100644 --- a/contrib/google.golang.org/grpc/server.go +++ b/contrib/google.golang.org/grpc/server.go @@ -53,7 +53,7 @@ func (ss *serverStream) RecvMsg(m interface{}) (err error) { defer func() { withMetadataTags(ss.ctx, ss.cfg, span) withRequestTags(ss.cfg, m, span) - finishWithError(span, err, ss.cfg) + finishWithError(span, err, ss.method, ss.cfg) }() } err = ss.ServerStream.RecvMsg(m) @@ -72,7 +72,7 @@ func (ss *serverStream) SendMsg(m interface{}) (err error) { ss.cfg.startSpanOptions(tracer.Measured())..., ) span.SetTag(ext.Component, componentName) - defer func() { finishWithError(span, err, ss.cfg) }() + defer func() { finishWithError(span, err, ss.method, ss.cfg) }() } err = ss.ServerStream.SendMsg(m) return err @@ -110,7 +110,7 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { case info.IsClientStream: span.SetTag(tagMethodKind, methodKindClientStream) } - defer func() { finishWithError(span, err, cfg) }() + defer func() { finishWithError(span, err, info.FullMethod, cfg) }() if appsec.Enabled() { handler = appsecStreamHandlerMiddleware(span, handler) } @@ -157,7 +157,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { handler = appsecUnaryHandlerMiddleware(span, handler) } resp, err := handler(ctx, req) - finishWithError(span, err, cfg) + finishWithError(span, err, info.FullMethod, cfg) return resp, err } } diff --git a/contrib/google.golang.org/grpc/stats_client.go b/contrib/google.golang.org/grpc/stats_client.go index ccce6ad004..f14da83c22 100644 --- a/contrib/google.golang.org/grpc/stats_client.go +++ b/contrib/google.golang.org/grpc/stats_client.go @@ -59,7 +59,15 @@ func (h *clientStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) { span.SetTag(ext.TargetPort, port) } case *stats.End: - finishWithError(span, rs.Error, h.cfg) + val := ctx.Value(fullMethodNameKey{}) + if val == nil { + return + } + fullMethod, ok := val.(string) + if !ok { + return + } + finishWithError(span, rs.Error, fullMethod, h.cfg) } } diff --git a/contrib/google.golang.org/grpc/stats_server.go b/contrib/google.golang.org/grpc/stats_server.go index 955e03d14b..709c7f5915 100644 --- a/contrib/google.golang.org/grpc/stats_server.go +++ b/contrib/google.golang.org/grpc/stats_server.go @@ -43,6 +43,7 @@ func (h *serverStatsHandler) TagRPC(ctx context.Context, rti *stats.RPCTagInfo) h.cfg.serviceName, spanOpts..., ) + ctx = context.WithValue(ctx, fullMethodNameKey{}, rti.FullMethodName) return ctx } @@ -52,8 +53,16 @@ func (h *serverStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) { if !ok { return } + val := ctx.Value(fullMethodNameKey{}) + if val == nil { + return + } + fullMethod, ok := val.(string) + if !ok { + return + } if v, ok := rs.(*stats.End); ok { - finishWithError(span, v.Error, h.cfg) + finishWithError(span, v.Error, fullMethod, h.cfg) } } From 06ce8185ae9c26045d595a43f276d1fc34ba3e2b Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 13 Jun 2023 01:54:04 +0900 Subject: [PATCH 03/12] Fix condition --- contrib/google.golang.org/grpc/grpc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/google.golang.org/grpc/grpc.go b/contrib/google.golang.org/grpc/grpc.go index 3e40821eeb..f26a7172db 100644 --- a/contrib/google.golang.org/grpc/grpc.go +++ b/contrib/google.golang.org/grpc/grpc.go @@ -81,7 +81,7 @@ func finishWithError(span ddtrace.Span, err error, method string, cfg *config) { err = nil } errcode := status.Code(err) - if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.nonErrorFunc == nil || cfg.nonErrorFunc(method, err)) { + if errcode == codes.OK || cfg.nonErrorCodes[errcode] || cfg.nonErrorFunc != nil || cfg.nonErrorFunc(method, err) { err = nil } span.SetTag(tagCode, errcode.String()) From 6a8cac12de3ee51a2f71afaaa4d0d512944b1131 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 13 Jun 2023 02:47:59 +0900 Subject: [PATCH 04/12] fix conditon --- contrib/google.golang.org/grpc/grpc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/google.golang.org/grpc/grpc.go b/contrib/google.golang.org/grpc/grpc.go index f26a7172db..83ea759643 100644 --- a/contrib/google.golang.org/grpc/grpc.go +++ b/contrib/google.golang.org/grpc/grpc.go @@ -81,7 +81,7 @@ func finishWithError(span ddtrace.Span, err error, method string, cfg *config) { err = nil } errcode := status.Code(err) - if errcode == codes.OK || cfg.nonErrorCodes[errcode] || cfg.nonErrorFunc != nil || cfg.nonErrorFunc(method, err) { + if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.nonErrorFunc != nil && cfg.nonErrorFunc(method, err)) { err = nil } span.SetTag(tagCode, errcode.String()) From 057eaab66b31fa61fd1a4959a91bd7805973ca32 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 13 Jun 2023 03:04:47 +0900 Subject: [PATCH 05/12] Fix comment --- contrib/google.golang.org/grpc/option.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/google.golang.org/grpc/option.go b/contrib/google.golang.org/grpc/option.go index d338dbf310..4ef0b8520b 100644 --- a/contrib/google.golang.org/grpc/option.go +++ b/contrib/google.golang.org/grpc/option.go @@ -119,7 +119,7 @@ func NonErrorCodes(cs ...codes.Code) InterceptorOption { // NonErrorFunc sets a custom function to determine whether an error should not be considered as an error for tracing purposes. // This function is evaluated when an error occurs, and if it returns true, the error will not be recorded in the trace. -// f: A function that takes an error as an argument and returns a boolean indicating whether the error should be ignored. +// f: A function taking the gRPC method and error as arguments, returning a boolean to indicate if the error should be ignored. func NonErrorFunc(f func(method string, err error) bool) InterceptorOption { return func(cfg *config) { cfg.nonErrorFunc = f From fb4336fe1f70754d1878d22a872494e160937d5f Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Thu, 13 Jul 2023 03:01:56 +0900 Subject: [PATCH 06/12] Fix deprecated InterceptorOption --- contrib/google.golang.org/grpc/option.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/google.golang.org/grpc/option.go b/contrib/google.golang.org/grpc/option.go index 8ab495e167..14a8172b34 100644 --- a/contrib/google.golang.org/grpc/option.go +++ b/contrib/google.golang.org/grpc/option.go @@ -133,7 +133,7 @@ func NonErrorCodes(cs ...codes.Code) InterceptorOption { // NonErrorFunc sets a custom function to determine whether an error should not be considered as an error for tracing purposes. // This function is evaluated when an error occurs, and if it returns true, the error will not be recorded in the trace. // f: A function taking the gRPC method and error as arguments, returning a boolean to indicate if the error should be ignored. -func NonErrorFunc(f func(method string, err error) bool) InterceptorOption { +func NonErrorFunc(f func(method string, err error) bool) Option { return func(cfg *config) { cfg.nonErrorFunc = f } From 50c90b20490aa876cdbcaebc1cd02038953e7d63 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Thu, 13 Jul 2023 03:02:15 +0900 Subject: [PATCH 07/12] Add unit tests --- contrib/google.golang.org/grpc/grpc_test.go | 215 ++++++++++++++++++++ 1 file changed, 215 insertions(+) diff --git a/contrib/google.golang.org/grpc/grpc_test.go b/contrib/google.golang.org/grpc/grpc_test.go index 7836eb046e..d2893fb46b 100644 --- a/contrib/google.golang.org/grpc/grpc_test.go +++ b/contrib/google.golang.org/grpc/grpc_test.go @@ -663,6 +663,221 @@ func waitForSpans(mt mocktracer.Tracer, sz int) { } } +func TestNonErrorFunc(t *testing.T) { + t.Run("unary", func(t *testing.T) { + for name, tt := range map[string]struct { + nonErrorFunc func(method string, err error) bool + message string + withError bool + wantCode string + wantMessage string + }{ + "Invalid_with_no_error": { + message: "invalid", + nonErrorFunc: func(method string, err error) bool { + if err == nil { + return true + } + + errCode := status.Code(err) + if errCode == codes.InvalidArgument && method == "/grpc.Fixture/Ping" { + return true + } + + return false + }, + withError: false, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", + }, + "Invalid_with_error": { + message: "invalid", + nonErrorFunc: func(method string, err error) bool { + if err == nil { + return true + } + + errCode := status.Code(err) + if errCode == codes.InvalidArgument && method == "/some/endpoint" { + return true + } + + return false + }, + withError: true, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", + }, + "Invalid_with_error_without_nonErrorFunc": { + message: "invalid", + nonErrorFunc: nil, + withError: true, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", + }, + } { + t.Run(name, func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + var ( + rig *rig + err error + ) + if tt.nonErrorFunc == nil { + rig, err = newRig(true) + } else { + rig, err = newRig(true, NonErrorFunc(tt.nonErrorFunc)) + } + if err != nil { + t.Fatalf("error setting up rig: %s", err) + } + + client := rig.client + _, err = client.Ping(context.Background(), &FixtureRequest{Name: tt.message}) + assert.Error(t, err) + assert.Equal(t, tt.wantCode, status.Code(err).String()) + assert.Equal(t, tt.wantMessage, status.Convert(err).Message()) + + spans := mt.FinishedSpans() + assert.Len(t, spans, 2) + + var serverSpan, clientSpan mocktracer.Span + + for _, s := range spans { + // order of traces in buffer is not garanteed + switch s.OperationName() { + case "grpc.server": + serverSpan = s + case "grpc.client": + clientSpan = s + } + } + + if tt.withError { + assert.NotNil(t, clientSpan.Tag(ext.Error)) + assert.NotNil(t, serverSpan.Tag(ext.Error)) + } else { + assert.Nil(t, clientSpan.Tag(ext.Error)) + assert.Nil(t, serverSpan.Tag(ext.Error)) + } + + rig.Close() + mt.Reset() + }) + } + }) + + t.Run("stream", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + for name, tt := range map[string]struct { + nonErrorFunc func(method string, err error) bool + message string + withError bool + wantCode string + wantMessage string + }{ + "Invalid_with_no_error": { + message: "invalid", + nonErrorFunc: func(method string, err error) bool { + if err == nil { + return true + } + + errCode := status.Code(err) + if errCode == codes.InvalidArgument && method == "/grpc.Fixture/StreamPing" { + return true + } + + return false + }, + withError: false, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", + }, + "Invalid_with_error": { + message: "invalid", + nonErrorFunc: func(method string, err error) bool { + if err == nil { + return true + } + + errCode := status.Code(err) + if errCode == codes.InvalidArgument && method == "/some/endpoint" { + return true + } + + return false + }, + withError: true, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", + }, + "Invalid_with_error_without_nonErrorFunc": { + message: "invalid", + nonErrorFunc: nil, + withError: true, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", + }, + } { + t.Run(name, func(t *testing.T) { + var ( + rig *rig + err error + ) + if tt.nonErrorFunc == nil { + rig, err = newRig(true) + } else { + rig, err = newRig(true, NonErrorFunc(tt.nonErrorFunc)) + } + if err != nil { + t.Fatalf("error setting up rig: %s", err) + } + + ctx, done := context.WithCancel(context.Background()) + client := rig.client + stream, err := client.StreamPing(ctx) + assert.NoError(t, err) + + err = stream.Send(&FixtureRequest{Name: tt.message}) + assert.NoError(t, err) + + _, err = stream.Recv() + assert.Error(t, err) + assert.Equal(t, tt.wantCode, status.Code(err).String()) + assert.Equal(t, tt.wantMessage, status.Convert(err).Message()) + + assert.NoError(t, stream.CloseSend()) + done() // close stream from client side + rig.Close() + + waitForSpans(mt, 5) + + spans := mt.FinishedSpans() + assert.Len(t, spans, 5) + + noError := true + for _, s := range spans { + if s.Tag(ext.Error) != nil { + noError = false + break + } + } + + if tt.withError { + assert.False(t, noError) + } else { + assert.True(t, noError) + } + + mt.Reset() + }) + } + }) +} + func TestAnalyticsSettings(t *testing.T) { assertRate := func(t *testing.T, mt mocktracer.Tracer, rate interface{}, opts ...InterceptorOption) { rig, err := newRig(true, opts...) From 100083f78905ea0f4530597d88c8bf31e4cb8848 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 5 Dec 2023 15:46:49 +0900 Subject: [PATCH 08/12] Renam function and config field name --- contrib/google.golang.org/grpc/grpc.go | 2 +- contrib/google.golang.org/grpc/grpc_test.go | 54 ++++++++++----------- contrib/google.golang.org/grpc/option.go | 8 +-- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/contrib/google.golang.org/grpc/grpc.go b/contrib/google.golang.org/grpc/grpc.go index 81c76a3aec..53e89289cb 100644 --- a/contrib/google.golang.org/grpc/grpc.go +++ b/contrib/google.golang.org/grpc/grpc.go @@ -81,7 +81,7 @@ func finishWithError(span ddtrace.Span, err error, method string, cfg *config) { err = nil } errcode := status.Code(err) - if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.nonErrorFunc != nil && cfg.nonErrorFunc(method, err)) { + if errcode == codes.OK || cfg.nonErrorCodes[errcode] || (cfg.errCheck != nil && cfg.errCheck(method, err)) { err = nil } span.SetTag(tagCode, errcode.String()) diff --git a/contrib/google.golang.org/grpc/grpc_test.go b/contrib/google.golang.org/grpc/grpc_test.go index d2893fb46b..edcf947581 100644 --- a/contrib/google.golang.org/grpc/grpc_test.go +++ b/contrib/google.golang.org/grpc/grpc_test.go @@ -663,18 +663,18 @@ func waitForSpans(mt mocktracer.Tracer, sz int) { } } -func TestNonErrorFunc(t *testing.T) { +func TestWithErrorCheck(t *testing.T) { t.Run("unary", func(t *testing.T) { for name, tt := range map[string]struct { - nonErrorFunc func(method string, err error) bool - message string - withError bool - wantCode string - wantMessage string + errCheck func(method string, err error) bool + message string + withError bool + wantCode string + wantMessage string }{ "Invalid_with_no_error": { message: "invalid", - nonErrorFunc: func(method string, err error) bool { + errCheck: func(method string, err error) bool { if err == nil { return true } @@ -692,7 +692,7 @@ func TestNonErrorFunc(t *testing.T) { }, "Invalid_with_error": { message: "invalid", - nonErrorFunc: func(method string, err error) bool { + errCheck: func(method string, err error) bool { if err == nil { return true } @@ -708,12 +708,12 @@ func TestNonErrorFunc(t *testing.T) { wantCode: codes.InvalidArgument.String(), wantMessage: "invalid", }, - "Invalid_with_error_without_nonErrorFunc": { - message: "invalid", - nonErrorFunc: nil, - withError: true, - wantCode: codes.InvalidArgument.String(), - wantMessage: "invalid", + "Invalid_with_error_without_errCheck": { + message: "invalid", + errCheck: nil, + withError: true, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", }, } { t.Run(name, func(t *testing.T) { @@ -772,15 +772,15 @@ func TestNonErrorFunc(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() for name, tt := range map[string]struct { - nonErrorFunc func(method string, err error) bool - message string - withError bool - wantCode string - wantMessage string + errCheck func(method string, err error) bool + message string + withError bool + wantCode string + wantMessage string }{ "Invalid_with_no_error": { message: "invalid", - nonErrorFunc: func(method string, err error) bool { + errCheck: func(method string, err error) bool { if err == nil { return true } @@ -798,7 +798,7 @@ func TestNonErrorFunc(t *testing.T) { }, "Invalid_with_error": { message: "invalid", - nonErrorFunc: func(method string, err error) bool { + errCheck: func(method string, err error) bool { if err == nil { return true } @@ -814,12 +814,12 @@ func TestNonErrorFunc(t *testing.T) { wantCode: codes.InvalidArgument.String(), wantMessage: "invalid", }, - "Invalid_with_error_without_nonErrorFunc": { - message: "invalid", - nonErrorFunc: nil, - withError: true, - wantCode: codes.InvalidArgument.String(), - wantMessage: "invalid", + "Invalid_with_error_without_errCheck": { + message: "invalid", + errCheck: nil, + withError: true, + wantCode: codes.InvalidArgument.String(), + wantMessage: "invalid", }, } { t.Run(name, func(t *testing.T) { diff --git a/contrib/google.golang.org/grpc/option.go b/contrib/google.golang.org/grpc/option.go index 14a8172b34..4c8292c409 100644 --- a/contrib/google.golang.org/grpc/option.go +++ b/contrib/google.golang.org/grpc/option.go @@ -29,7 +29,7 @@ type config struct { serviceName func() string spanName string nonErrorCodes map[codes.Code]bool - nonErrorFunc func(method string, err error) bool + errCheck func(method string, err error) bool traceStreamCalls bool traceStreamMessages bool noDebugStack bool @@ -130,12 +130,12 @@ func NonErrorCodes(cs ...codes.Code) InterceptorOption { } } -// NonErrorFunc sets a custom function to determine whether an error should not be considered as an error for tracing purposes. +// WithErrorCheck sets a custom function to determine whether an error should not be considered as an error for tracing purposes. // This function is evaluated when an error occurs, and if it returns true, the error will not be recorded in the trace. // f: A function taking the gRPC method and error as arguments, returning a boolean to indicate if the error should be ignored. -func NonErrorFunc(f func(method string, err error) bool) Option { +func WithErrorCheck(f func(method string, err error) bool) Option { return func(cfg *config) { - cfg.nonErrorFunc = f + cfg.errCheck = f } } From 6a3227efb83a2ca1ed60996dd6b6824815e36158 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 5 Dec 2023 15:48:02 +0900 Subject: [PATCH 09/12] Refactor error check in grpc_test.go --- contrib/google.golang.org/grpc/grpc_test.go | 24 +++++++-------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/contrib/google.golang.org/grpc/grpc_test.go b/contrib/google.golang.org/grpc/grpc_test.go index edcf947581..e610332d95 100644 --- a/contrib/google.golang.org/grpc/grpc_test.go +++ b/contrib/google.golang.org/grpc/grpc_test.go @@ -720,15 +720,11 @@ func TestWithErrorCheck(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() - var ( - rig *rig - err error - ) - if tt.nonErrorFunc == nil { - rig, err = newRig(true) - } else { - rig, err = newRig(true, NonErrorFunc(tt.nonErrorFunc)) + var ops []Option + if tt.errCheck != nil { + ops = append(ops, WithErrorCheck(tt.errCheck)) } + rig, err := newRig(true, ops...) if err != nil { t.Fatalf("error setting up rig: %s", err) } @@ -823,15 +819,11 @@ func TestWithErrorCheck(t *testing.T) { }, } { t.Run(name, func(t *testing.T) { - var ( - rig *rig - err error - ) - if tt.nonErrorFunc == nil { - rig, err = newRig(true) - } else { - rig, err = newRig(true, NonErrorFunc(tt.nonErrorFunc)) + var opts []Option + if tt.errCheck != nil { + opts = append(opts, WithErrorCheck(tt.errCheck)) } + rig, err := newRig(true, opts...) if err != nil { t.Fatalf("error setting up rig: %s", err) } From 5b83467f878be33173e8a755726d82b9586fd2e2 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 5 Dec 2023 15:48:41 +0900 Subject: [PATCH 10/12] Refactor stats_client and stats_server to use a more concise code structure --- contrib/google.golang.org/grpc/stats_client.go | 9 +-------- contrib/google.golang.org/grpc/stats_server.go | 10 ++-------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/contrib/google.golang.org/grpc/stats_client.go b/contrib/google.golang.org/grpc/stats_client.go index f14da83c22..1d397041f4 100644 --- a/contrib/google.golang.org/grpc/stats_client.go +++ b/contrib/google.golang.org/grpc/stats_client.go @@ -59,14 +59,7 @@ func (h *clientStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) { span.SetTag(ext.TargetPort, port) } case *stats.End: - val := ctx.Value(fullMethodNameKey{}) - if val == nil { - return - } - fullMethod, ok := val.(string) - if !ok { - return - } + fullMethod, _ := ctx.Value(fullMethodNameKey{}).(string) finishWithError(span, rs.Error, fullMethod, h.cfg) } } diff --git a/contrib/google.golang.org/grpc/stats_server.go b/contrib/google.golang.org/grpc/stats_server.go index 709c7f5915..0a9c39c362 100644 --- a/contrib/google.golang.org/grpc/stats_server.go +++ b/contrib/google.golang.org/grpc/stats_server.go @@ -53,14 +53,8 @@ func (h *serverStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) { if !ok { return } - val := ctx.Value(fullMethodNameKey{}) - if val == nil { - return - } - fullMethod, ok := val.(string) - if !ok { - return - } + + fullMethod, _ := ctx.Value(fullMethodNameKey{}).(string) if v, ok := rs.(*stats.End); ok { finishWithError(span, v.Error, fullMethod, h.cfg) } From 4fca22416cf9726a6cb7943f58a39941a7b5b5a3 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 5 Dec 2023 15:49:14 +0900 Subject: [PATCH 11/12] Move mocktrace start for stream test --- contrib/google.golang.org/grpc/grpc_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/google.golang.org/grpc/grpc_test.go b/contrib/google.golang.org/grpc/grpc_test.go index e610332d95..6d5788b7b7 100644 --- a/contrib/google.golang.org/grpc/grpc_test.go +++ b/contrib/google.golang.org/grpc/grpc_test.go @@ -765,8 +765,6 @@ func TestWithErrorCheck(t *testing.T) { }) t.Run("stream", func(t *testing.T) { - mt := mocktracer.Start() - defer mt.Stop() for name, tt := range map[string]struct { errCheck func(method string, err error) bool message string @@ -819,6 +817,8 @@ func TestWithErrorCheck(t *testing.T) { }, } { t.Run(name, func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() var opts []Option if tt.errCheck != nil { opts = append(opts, WithErrorCheck(tt.errCheck)) From cff830cb2b154a250a03d7db09bfe9b43e82c581 Mon Sep 17 00:00:00 2001 From: Shota Iwami Date: Tue, 5 Dec 2023 16:08:03 +0900 Subject: [PATCH 12/12] Fix error handling in TestWithError --- contrib/google.golang.org/grpc/grpc_test.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/contrib/google.golang.org/grpc/grpc_test.go b/contrib/google.golang.org/grpc/grpc_test.go index 6d5788b7b7..78a4e33684 100644 --- a/contrib/google.golang.org/grpc/grpc_test.go +++ b/contrib/google.golang.org/grpc/grpc_test.go @@ -850,20 +850,12 @@ func TestWithErrorCheck(t *testing.T) { spans := mt.FinishedSpans() assert.Len(t, spans, 5) - noError := true for _, s := range spans { - if s.Tag(ext.Error) != nil { - noError = false - break + if s.Tag(ext.Error) != nil && !tt.withError { + assert.FailNow(t, "expected no error tag on the span") } } - if tt.withError { - assert.False(t, noError) - } else { - assert.True(t, noError) - } - mt.Reset() }) }