-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
logging: add similar logging to v3 #1041
logging: add similar logging to v3 #1041
Conversation
Can one of the admins verify this patch? |
Add back in HTTP logging to what was present in v3, to each of the handlers. Signed-off-by: mcoops <cooper.d.mark@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, small nit.
httptransport/logginghandler.go
Outdated
Str("remote addr", r.RemoteAddr). | ||
Str("method", r.Method). | ||
Str("request uri", r.RequestURI). | ||
Str("status", strconv.Itoa(http.StatusOK)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this always log 200?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's embarrassing runs I hate how that works. Readding
Signed-off-by: mcoops <cooper.d.mark@gmail.com>
I assume we squash on merge? Otherwise can rebase. |
Signed-off-by: mcoops <cooper.d.mark@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I suggest running Clair in local dev or on prem to ensure the logs work correctly.
Add back in HTTP logging to what was present in v3, to each of the handlers.
Matched the existing format/fields from v3 here:
clair/api/v3/server.go
Line 62 in 1d14177
Didn't add tests as there isn't really much to test.