-
Notifications
You must be signed in to change notification settings - Fork 164
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 tweaks to improve usability #1235
Conversation
Signed-off-by: Bob Callaway <bcallaway@google.com>
Codecov Report
@@ Coverage Diff @@
## main #1235 +/- ##
==========================================
- Coverage 64.23% 63.16% -1.08%
==========================================
Files 82 82
Lines 7628 7658 +30
==========================================
- Hits 4900 4837 -63
- Misses 2100 2208 +108
+ Partials 628 613 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Bob Callaway <bcallaway@google.com>
enc.AddString("responseSize", h.responseSize) | ||
enc.AddString("userAgent", h.userAgent) | ||
enc.AddString("remoteIP", h.remoteIP) | ||
enc.AddString("latency", h.latency) |
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.
enc.AddDuration("latency", h.latency)
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.
https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#HttpRequest wants this represented as a string in units of seconds, so that's why I formatted it the way I did
fields = append(fields, zap.Any("extra", extra)) | ||
} | ||
|
||
log.ContextLogger(z.r.Context()).With(fields...).Info("completed request") |
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.
If the fields
were declared as []zapcore.Field
then this can be
log.ContextLogger(z.r.Context()).With(fields).Info("completed request")
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.
With([]interface{})
is how this is defined, and the logger doesn't work if you more strongly type this.
Signed-off-by: Bob Callaway <bcallaway@google.com>
status: status, | ||
responseSize: bytes, | ||
userAgent: z.r.Header.Get("User-Agent"), | ||
remoteIP: z.r.RemoteAddr, |
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.
Any reason to log IP? Some PII risk of being able to correlate requests across artifacts, gather usage data, etc
Signed-off-by: Bob Callaway bcallaway@google.com