Add proper categorization for client connection closing error #6844
Conversation
timl3136
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
|
|
||
| // Check for gRPC connection closing error | ||
| if strings.Contains(err.Error(), constants.GRPCConnectionClosingError) { | ||
| logger.Warn(constants.GRPCConnectionClosingError, tag.Error(err)) |
There was a problem hiding this comment.
other failure types have a corresponding metric. let's define one for this to be able to keep track.
There was a problem hiding this comment.
If this is normal, should we still have as a warn and not info?
There was a problem hiding this comment.
Even though this error is not within our control, I think making it into a warn level can help during events like network outage.
There was a problem hiding this comment.
Is there a reason we can't use errors.Is/As so we can handle wrapping?
There was a problem hiding this comment.
Thanks, I have change the code to use error.Is instead.
There was a problem hiding this comment.
errors.Is expect same type of error. I'm not sure the actual error we receive will match with your inline errors.New(constants.GRPCConnectionClosingError). Can you try to repro the issue locally to make sure this part works (stop matching while there's a long poll request made by frontend)?
There was a problem hiding this comment.
after some discussion offline I agree it might be slightly fiddly to be able to match it. I think it might be possible, but is not worth blocking on. It's not 100% clear to me if the error bubbling up is a YARPC error or a GPRC error and so any attempt to catch the wrapped variant would need to handle this.
There was a problem hiding this comment.
From grpc manual, it's cancelled error. Yarpc must have a mapping of canceled error type. Thus it should be handled like
if err.Code() == yarpcerrors.Canceled
| // Check for gRPC connection closing error | ||
| if strings.Contains(err.Error(), constants.GRPCConnectionClosingError) { | ||
| logger.Warn(constants.GRPCConnectionClosingError, tag.Error(err)) | ||
| return err |
There was a problem hiding this comment.
What is the implication of returning err as is vs returning frontendInternalServiceError from this function? does that change retry behavior on the caller side?
There was a problem hiding this comment.
frontendInternalServiceError is simply a wrapper of fmt.Errorf and make sure the error not being a thrift error. It does not change any retry behavior on the caller side.
…ror type cannot be ascertained through gRPC transport
| metricsClient := metrics.NewClient(testScope, metrics.Frontend) | ||
| handler := &apiHandler{} | ||
|
|
||
| err := handler.handleErr(errors.New(constants.GRPCConnectionClosingError), metricsClient.Scope(0), logger) |
There was a problem hiding this comment.
This is not covering the yarpc canceled error branch.
What changed?
Why?
Previously, gRPC client connection closing errors were being treated as uncategorized errors, which:
These errors are common during normal operation (e.g., when clients disconnect) and should be handled gracefully
How did you test it?
Unit tests
Potential risks
Release notes
Documentation Changes