Add info logging for 400s, error for 500s, adjust timestamp error level on GetCurrent #798
Conversation
…el on GetCurrent Signed-off-by: Riyaz Faizullabhoy <[email protected]>
|
@riyazdf Thanks so much for adding these! I was wondering actually, though, if it'd make sense to bottleneck the logging all in one place (e.g. at Since all of these errors, if they are of a errcode type, can get the HTTP status code, we can determine what log level to log at. That way we can make sure to get all of them at once. |
|
@cyli: great idea. I've added that logic in addition to the previous logging - I'm ok with logging in both places so we have more context in the logs (for codepath, http request type, etc) |
riyazdf
force-pushed
the
server-logging
branch
2 times, most recently
from
b886d6a
to
7417a83
Compare
| // info level logging for non-5XX http errors | ||
| httpErrCode := httpErr.ErrorCode().Descriptor().HTTPStatusCode | ||
| if (httpErrCode < http.StatusOK || httpErrCode >= http.StatusMultipleChoices) && httpErrCode < http.StatusInternalServerError { | ||
| logrus.Info(httpErr) |
There was a problem hiding this comment.
Can we call log.Info here and log.Error below instead of directly using logrus, so we can get associate the logs with the request context too?
There was a problem hiding this comment.
ah good catch. I'll change the logrus call below too 👍
|
Thanks @riyazdf! Other than the above nitpick, LGTM! |
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
| @@ -152,6 +162,7 @@ func DeleteHandler(ctx context.Context, w http.ResponseWriter, r *http.Request) | |||
| s := ctx.Value("metaStore") | |||
| store, ok := s.(storage.MetaStore) | |||
| if !ok { | |||
| logrus.Error("500 DELETE repository: no storage exists") | |||
There was a problem hiding this comment.
Should this be logger.Error? (would need to pull the logger out of the context like other handlers)
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
| if httpErr, ok := err.(errcode.ErrorCoder); ok { | ||
| // info level logging for non-5XX http errors | ||
| httpErrCode := httpErr.ErrorCode().Descriptor().HTTPStatusCode | ||
| if (httpErrCode < http.StatusOK || httpErrCode >= http.StatusMultipleChoices) && httpErrCode < http.StatusInternalServerError { |
There was a problem hiding this comment.
We shouldn't have an httpErr if the status is 200 but it we do it would probably want to be logged at info level. Seems like we could just collapse this could be simplified to:
if httpErrCode >= http.StatusInternalServerError {
log.Error(httpErr)
} else {
log.Info(httpErr)
}
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
|
LGTM |
Adds info-level logging for 4XX errors and error-level for 5XX in the server. Also lowers errors on retrieving timestamps to debug-level.
Closes #766
Signed-off-by: Riyaz Faizullabhoy [email protected]