Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add info logging for 400s, error for 500s, adjust timestamp error level on GetCurrent #798

Merged
merged 4 commits into from
Jul 18, 2016

Conversation

riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Jun 22, 2016

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]

@cyli
Copy link
Contributor

cyli commented Jun 23, 2016

@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 util/http.go where if there's an error when calling the handler?).

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.

@riyazdf
Copy link
Contributor Author

riyazdf commented Jun 23, 2016

@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 riyazdf force-pushed the server-logging branch 2 times, most recently from b886d6a to 7417a83 Compare June 23, 2016 00:48
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good catch. I'll change the logrus call below too 👍

@cyli
Copy link
Contributor

cyli commented Jun 23, 2016

Thanks @riyazdf! Other than the above nitpick, LGTM!

@riyazdf riyazdf modified the milestone: Notary 0.4 Jun 24, 2016
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, added!

Signed-off-by: Riyaz Faizullabhoy <[email protected]>
@endophage
Copy link
Contributor

LGTM

@endophage endophage merged commit 56c6701 into master Jul 18, 2016
@riyazdf riyazdf deleted the server-logging branch July 19, 2016 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants