Clean up tchannel client/endpoint/server logs #498
Conversation
| if err == nil { | ||
| c.logger.Info("Finished an outgoing client TChannel request", c.logFields()...) | ||
| } else { | ||
| c.logger.Warn("Failed to send outgoing client TChannel request", zap.Error(err)) |
There was a problem hiding this comment.
should this be logger.error
There was a problem hiding this comment.
This is open to discussion. My reasoning is that when error happens, we will look at one error message (which is logged at endpoint handler and anywhere else is warn) and could understand what happened, if that is not sufficient we then look further for the warn logs associated with the request. Based on what I have experienced, multiple error log sites that describe the same cause create distraction and make logs less effective.
There was a problem hiding this comment.
We could have a zap.field GUID associated with each warn, and let the error catch that GUID which is the root cause of this Error. We could have a runbook/wiki document all the mitigation step for each GUID and handle the oncall to SRE
There was a problem hiding this comment.
Once we use context.logger everywhere, we will have the request uuid in the context and all the logs that associated with the request will have the same request uuid.
| if err == nil { | ||
| c.endpoint.contextLogger.Info(c.ctx, "Finished an incoming server TChannel request", c.logFields()...) | ||
| } else { | ||
| c.endpoint.contextLogger.Warn(c.ctx, "Failed to serve incoming TChannel request", zap.Error(err)) |
There was a problem hiding this comment.
is this contextLogger.Error
This PR cleans up tchannel client/endpoint/server logs in the following way:
errorlevel, everywhere else errrors are logged withwarnlevel;There will be a another PR that takes cares of http client/endpoint/server logs.