contrib/google.golang.org/grpc: add `WithErrorCheck` option #2035

BIwashi · 2023-06-12T18:11:28Z

What does this PR do?

This PR adds a new option called WithErrorCheck which allows users to set a custom function to determine whether an error should not be considered as an error for tracing purposes before sending it to DataDog.

Motivation

There are cases where an application produces errors, but they should not be sent to DataDog as errors. While the WithErrorCheck option allows for not treating specific gRPC status codes as errors, there are use cases where a more granular decision is needed based on the specific error, rather than just the status code.
For example, a user might want to consider a NotFound error as non-error for a specific RPC.

example

opts = []rpc.Option{
	...
	rpc.WithUnaryInterceptors(
		ddgrpc.UnaryServerInterceptor(
			ddgrpc.WithErrorCheck(func(method string, err error) bool {
				if err == nil {
					return true
				}
				errCode := status.Code(err)
				if errCode == codes.NotFound && method == "/example.ExampleService/GetInfo" {
					return true
				}
				return false
			}),
		),
	),
	...
}
...
rpcServer = rpc.NewServer(rpcHandler, opts...)

Describe how to test/QA your changes

Unit tests are included.

Reviewer's Checklist

Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

dianashevchenko

Hi @BIwashi, thank you for the contribution! I think it wouldn't hurt to add some unit tests covering the NonErrorFunc option.

BIwashi · 2023-07-12T18:06:53Z

@dianashevchenko
I added unit tests for the NonErrorFunc option!
50c90b2
How does this implementation look to you? I would appreciate your feedback.

dianashevchenko · 2023-07-25T13:44:20Z

contrib/google.golang.org/grpc/option.go

+// 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) Option {
+	return func(cfg *config) {
+		cfg.nonErrorFunc = f
+	}
+}
+


I suggest aligning naming with other Options in this package, for example 'WithErrorCheck' naming is widely used across other packages.

I have renamed the function name and config field names considering this comment as well. Thanks!
100083f

dianashevchenko · 2023-07-25T13:44:46Z

contrib/google.golang.org/grpc/grpc_test.go

+				var (
+					rig *rig
+					err error
+				)
+				if tt.nonErrorFunc == nil {
+					rig, err = newRig(true)
+				} else {
+					rig, err = newRig(true, NonErrorFunc(tt.nonErrorFunc))
+				}


Suggested change

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.nonErrorFunc != nil {

opts = append(opts, NonErrorFunc(tt.nonErrorFunc))

}

rig, err := newRig(true, opts...)

Thank you so much!
I fixed it.
6a3227e

dianashevchenko · 2023-07-25T13:45:14Z

contrib/google.golang.org/grpc/grpc_test.go

+				var (
+					rig *rig
+					err error
+				)
+				if tt.nonErrorFunc == nil {
+					rig, err = newRig(true)
+				} else {
+					rig, err = newRig(true, NonErrorFunc(tt.nonErrorFunc))
+				}


Suggested change

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.nonErrorFunc != nil {

opts = append(opts, NonErrorFunc(tt.nonErrorFunc))

}

rig, err := newRig(true, opts...)

dianashevchenko · 2023-07-25T13:48:11Z

contrib/google.golang.org/grpc/grpc_test.go

+		mt := mocktracer.Start()
+		defer mt.Stop()


Please move the start of the mocktracer this into the test case run

Is this the correct revision of your point?
4fca224

dianashevchenko · 2023-07-25T13:59:40Z

contrib/google.golang.org/grpc/stats_server.go

+	val := ctx.Value(fullMethodNameKey{})
+	if val == nil {
+		return
+	}
+	fullMethod, ok := val.(string)
+	if !ok {
+		return
+	}


Returning early here will result in finishWithError(span, v.Error, fullMethod, h.cfg) not being called unless fullMethodNameKey is set. Is this the expected behaviour?

Suggested change

val := ctx.Value(fullMethodNameKey{})

if val == nil {

return

}

fullMethod, ok := val.(string)

if !ok {

return

}

method, _ := ctx.Value(fullMethodNameKey{}).(string)

if v, ok := rs.(*stats.End); ok {

finishWithError(span, v.Error, method, h.cfg)

}

You are correct, finishWithError needs to be called. I made a mistake. Thanks.
I fixed it.

5b83467

dianashevchenko · 2023-07-25T14:01:04Z

contrib/google.golang.org/grpc/stats_client.go

+		val := ctx.Value(fullMethodNameKey{})
+		if val == nil {
+			return
+		}
+		fullMethod, ok := val.(string)
+		if !ok {
+			return
+		}
+		finishWithError(span, rs.Error, fullMethod, h.cfg)


Same here, don't we want to run finishWithError regardless?

Suggested change

val := ctx.Value(fullMethodNameKey{})

if val == nil {

return

}

fullMethod, ok := val.(string)

if !ok {

return

}

finishWithError(span, rs.Error, fullMethod, h.cfg)

method, _ := ctx.Value(fullMethodNameKey{}).(string)

finishWithError(span, rs.Error, method, h.cfg)

I fixed it, too.
5b83467

dianashevchenko · 2023-07-25T15:07:52Z

contrib/google.golang.org/grpc/grpc_test.go

+				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)
+				}


Suggested change

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)

}

for _, s := range spans {

if s.Tag(ext.Error) != nil && !tt.withError {

assert.FailNow(t, "expected no error tag on the span")

}

}

I had not thought of this idea. Thank you so much!
I fixed it.
cff830c

ajgajg1134

See comment on proposal here: #2043 (comment)

more concise code structure

BIwashi · 2023-12-05T07:25:26Z

I apologize for the delay in making the corrections. It would be much appreciated if you could take the time to review the revisions again. Thanks.
@dianashevchenko cc @ajgajg1134

BIwashi requested a review from a team June 12, 2023 18:11

BIwashi added 5 commits June 13, 2023 03:13

Add non error func option for grpc

08a7e75

Add FullMethod string for nonErrorFunc

252e275

Fix condition

06ce818

fix conditon

6a8cac1

Fix comment

057eaab

BIwashi force-pushed the add-rpc-non-error-func-option branch from c39258b to 057eaab Compare June 12, 2023 18:13

BIwashi changed the title ~~Add rpc non error func option~~ contrib/google.golang.org/grpc: add NonErrorFunc option for interceptor Jun 12, 2023

BIwashi changed the title ~~contrib/google.golang.org/grpc: add NonErrorFunc option for interceptor~~ contrib/google.golang.org/grpc: add NonErrorFunc option Jun 12, 2023

BIwashi mentioned this pull request Jun 14, 2023

proposal: contrib/google.golang.org/grpc: add a WithErrorCheck option #2043

Open

knusbaum assigned dianashevchenko Jun 29, 2023

Merge branch 'main' into add-rpc-non-error-func-option

c6b7f79

dianashevchenko requested changes Jul 10, 2023

View reviewed changes

BIwashi added 2 commits July 13, 2023 03:01

Fix deprecated InterceptorOption

fb4336f

Add unit tests

50c90b2

Merge branch 'main' into add-rpc-non-error-func-option

0fd73e7

BIwashi requested a review from dianashevchenko July 12, 2023 18:08

Merge branch 'main' into add-rpc-non-error-func-option

b925a3a

dianashevchenko requested changes Jul 25, 2023

View reviewed changes

ahmed-mez added the apm:ecosystem contrib/* related feature requests or bugs label Aug 17, 2023

ajgajg1134 requested changes Oct 31, 2023

View reviewed changes

katiehockman added the stale Stuck for more than 1 month label Nov 28, 2023

BIwashi added 5 commits December 5, 2023 15:46

Renam function and config field name

100083f

Refactor error check in grpc_test.go

6a3227e

Refactor stats_client and stats_server to use a

5b83467

more concise code structure

Move mocktrace start for stream test

4fca224

Fix error handling in TestWithError

cff830c

BIwashi requested a review from dianashevchenko December 5, 2023 07:26

BIwashi requested a review from ajgajg1134 December 5, 2023 07:26

BIwashi changed the title ~~contrib/google.golang.org/grpc: add NonErrorFunc option~~ contrib/google.golang.org/grpc: add WithErrorCheck option Dec 7, 2023

BIwashi changed the title ~~contrib/google.golang.org/grpc: add WithErrorCheck option~~ contrib/google.golang.org/grpc: add WithErrorCheck option Dec 7, 2023

BIwashi added 2 commits December 14, 2023 13:37

Merge branch 'main' into add-rpc-non-error-func-option

ca200c3

Merge branch 'main' into add-rpc-non-error-func-option

7f0c9b5

BIwashi requested a review from a team as a code owner January 18, 2024 04:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/google.golang.org/grpc: add `WithErrorCheck` option #2035

contrib/google.golang.org/grpc: add `WithErrorCheck` option #2035

BIwashi commented Jun 12, 2023 •

edited

dianashevchenko left a comment

BIwashi commented Jul 12, 2023 •

edited

dianashevchenko Jul 25, 2023

BIwashi Dec 5, 2023

dianashevchenko Jul 25, 2023

BIwashi Dec 5, 2023

dianashevchenko Jul 25, 2023

dianashevchenko Jul 25, 2023

BIwashi Dec 5, 2023

dianashevchenko Jul 25, 2023

BIwashi Dec 5, 2023

dianashevchenko Jul 25, 2023

BIwashi Dec 5, 2023

dianashevchenko Jul 25, 2023

BIwashi Dec 5, 2023

ajgajg1134 left a comment

BIwashi commented Dec 5, 2023 •

edited

contrib/google.golang.org/grpc: add WithErrorCheck option #2035

Are you sure you want to change the base?

contrib/google.golang.org/grpc: add WithErrorCheck option #2035

Conversation

BIwashi commented Jun 12, 2023 • edited

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

dianashevchenko left a comment

Choose a reason for hiding this comment

BIwashi commented Jul 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

BIwashi commented Dec 5, 2023 • edited

contrib/google.golang.org/grpc: add `WithErrorCheck` option #2035

contrib/google.golang.org/grpc: add `WithErrorCheck` option #2035

BIwashi commented Jun 12, 2023 •

edited

BIwashi commented Jul 12, 2023 •

edited

BIwashi commented Dec 5, 2023 •

edited