Enhanced handling of grpc errors (issue #331) #338
Conversation
nr-swilloughby
changed the title
| // or `UnaryServiceInterceptor()` functions (q.v.); however, in this case the new handlers | ||
| // become the default for any subsequent interceptors created by the above functions. | ||
| // | ||
| func Configure(options ...handlerOption) { |
There was a problem hiding this comment.
Do you think Configure() might be too general a name for this? There might be other things to configure in the future.
Oh, but I suppose we could have other "With..." options that have nothing to do with StatusHandlers, and then the Configure method name would still make sense. I'm a little fuzzy on what this would look like.
There was a problem hiding this comment.
I was pondering that too. Since it's in the nrgrpc package, it would be just configuring things within the package, but the idea was that we'd add other "With..." options for other package configuration needs, so we don't have multiple calls to make to customize the package in various ways.
|
I had a thought about the spurious error reporting that's being triggered elsewhere being related to our setting the transaction's response code. We may decide not to call |
| // return &sampleapp.Message{Text: "Hello from DoUnaryUnary"}, nil | ||
| return &sampleapp.Message{}, status.New(codes.DataLoss, "oooooops!").Err() |
There was a problem hiding this comment.
I think this is my test to force the error -- we might want to revert it back to the commented-out line above.
There was a problem hiding this comment.
Yes, good catch.
| txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) | ||
| txn.NoticeError(&newrelic.Error{ | ||
| Message: s.Message(), | ||
| Class: "gRPC Error: " + s.Code().String(), |
There was a problem hiding this comment.
I was going back and forth on whether to include the text " Error" in this message. When displayed in the UI, it's already on a page called "Errors," under a heading called "Error class."
There was a problem hiding this comment.
It would be more accurate to call it "gRPC Status Code", actually.
There was a problem hiding this comment.
Oh, yeah. That's much better.
| var interceptorStatusHandlerRegistry = statusHandlerMap{ | ||
| codes.OK: OKInterceptorStatusHandler, | ||
| codes.Canceled: InfoInterceptorStatusHandler, | ||
| codes.Unknown: ErrorInterceptorStatusHandler, | ||
| codes.InvalidArgument: InfoInterceptorStatusHandler, | ||
| codes.DeadlineExceeded: WarningInterceptorStatusHandler, | ||
| codes.NotFound: InfoInterceptorStatusHandler, | ||
| codes.AlreadyExists: InfoInterceptorStatusHandler, | ||
| codes.PermissionDenied: WarningInterceptorStatusHandler, | ||
| codes.ResourceExhausted: WarningInterceptorStatusHandler, | ||
| codes.FailedPrecondition: WarningInterceptorStatusHandler, | ||
| codes.Aborted: WarningInterceptorStatusHandler, | ||
| codes.OutOfRange: WarningInterceptorStatusHandler, | ||
| codes.Unimplemented: ErrorInterceptorStatusHandler, | ||
| codes.Internal: ErrorInterceptorStatusHandler, | ||
| codes.Unavailable: WarningInterceptorStatusHandler, | ||
| codes.DataLoss: ErrorInterceptorStatusHandler, | ||
| codes.Unauthenticated: InfoInterceptorStatusHandler, | ||
| } |
There was a problem hiding this comment.
Looks like we have tests for ErrorInterceptorStatusHandler below, using codes.DataLoss. Would it make sense to include tests for the other handlers, as well?
There was a problem hiding this comment.
Deferring for later, @nr-swilloughby 's putting something on the Go Engineering Board so it doesn't get forgotten.
Amends the error handling capability of the nrgrpc integration to differentiate between grpc statuses which should be reported as errors with those to be reported as warnings or informational messages.
Details
Rather than calling
NoticeError()on any non-OKstatus, we callNoticeError()only on ones we think merit that designation. Others are reported by adding attributes to the transaction which describe the grpc status codes returned from the external service.If the built-in default actions are not sufficient or don't match the application's needs, they may be changed by the user by changing the designation of any specific grpc error, or even by supplying a custom handler function to perform any action desired when the corresponding grpc error is seen.