-
Notifications
You must be signed in to change notification settings - Fork 43
feat: allow to customize log message #74
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,11 @@ func RequestLogger(logger *slog.Logger, o *Options) func(http.Handler) http.Hand | |||||||||||||||
| s = SchemaECS | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| logFormat := o.LogFormat | ||||||||||||||||
| if logFormat == nil { | ||||||||||||||||
| logFormat = defaultLogFormat | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+38
to
+41
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| return func(next http.Handler) http.Handler { | ||||||||||||||||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||||||||||||||||
| ctx := context.WithValue(r.Context(), ctxKeyLogAttrs{}, &[]slog.Attr{}) | ||||||||||||||||
|
|
@@ -166,7 +171,7 @@ func RequestLogger(logger *slog.Logger, o *Options) func(http.Handler) http.Hand | |||||||||||||||
| logAttrs = groupAttrs(logAttrs, s.GroupDelimiter) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| msg := fmt.Sprintf("%s %s => HTTP %v (%v)", r.Method, r.URL, statusCode, duration) | ||||||||||||||||
| msg := logFormat(r, statusCode, duration) | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| logger.LogAttrs(ctx, lvl, msg, logAttrs...) | ||||||||||||||||
| }() | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,9 @@ | ||||||||||
| package httplog | ||||||||||
|
|
||||||||||
| import ( | ||||||||||
| "fmt" | ||||||||||
| "time" | ||||||||||
|
|
||||||||||
| "log/slog" | ||||||||||
| "net/http" | ||||||||||
| ) | ||||||||||
|
|
@@ -95,6 +98,10 @@ type Options struct { | |||||||||
| // | ||||||||||
| // WARNING: Be careful not to leak any sensitive information in the logs. | ||||||||||
| LogExtraAttrs func(req *http.Request, reqBody string, respStatus int) []slog.Attr | ||||||||||
|
|
||||||||||
| // LogFormat is a optional function that lets you control the format of the log message | ||||||||||
| // If not provided, default format will be used | ||||||||||
| LogFormat func(*http.Request, int, time.Duration) string | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you see people needing more than statusCode and duration in the future? Should we pass the arguments via an extendable struct?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's do it via struct, so we don't make breaking changes in future. type LogFormatParameters struct {
StatusCode int
Duration time.Duration
}
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this @david-littlefarmer. Perhaps we can call it "attrs"? How feasible would it be to pass the raw logAttrs?
Suggested change
Would that be developer friendly?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are already populating attributes of the log. This should just build the message for log. So it don't makes sense to me to pass same parameter again to message. Also every log we would have to iterate through slice of attributes and find the wanted one. But the flexibility is nice, no argues against it. So what about this? We can pass the type LogFormatParameters struct {
StatusCode int
Duration time.Duration
Attributes []slog.Attr
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm fine with this. But remember that we're in the hotpath and every extra attr copy counts (N * number of HTTP requests). So better if we can avoid the extra copying just to alter the format.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And as I mentioned the above, I realized it'd be probably better to print the default message without calling the defaultLogFormatter function. IDK if Go can inline a function that is dynamic, I guess not? Meaning we'd copy the attrs on function stack for no reason. var msg string
if opts.LogFormat != nil {
msg = logFormat(r, LogFormatArgs{...})
} else {
msg = fmt.Sprintf("%s %s => HTTP %v (%v)", r.Method, r.URL, statusCode, duration)
}How bad would it be for users to go though []log.Attrs? It should be a straight forward loop; with no allocations, right? TL;DR: I'm trying to avoid extra allocations per each request.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this would be the best approach. |
||||||||||
| } | ||||||||||
|
|
||||||||||
| var defaultOptions = Options{ | ||||||||||
|
|
@@ -105,4 +112,9 @@ var defaultOptions = Options{ | |||||||||
| LogResponseHeaders: []string{"Content-Type"}, | ||||||||||
| LogBodyContentTypes: []string{"application/json", "application/xml", "text/plain", "text/csv", "application/x-www-form-urlencoded", ""}, | ||||||||||
| LogBodyMaxLen: 1024, | ||||||||||
| LogFormat: defaultLogFormat, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func defaultLogFormat(r *http.Request, statusCode int, duration time.Duration) string { | ||||||||||
| return fmt.Sprintf("%s %s => HTTP %v (%v)", r.Method, r.URL, statusCode, duration) | ||||||||||
| } | ||||||||||
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.
out of curiosity, what's the format you're thinking of for yourself?
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.
Just constant string, like "request processed".
Because all info about request already stored in slog attrs, and variance part in
messagefield sort of difficult to work with aggregations (for example, % of uniq clients were affected by a particular error in this operation).I thought about just putting the template string in the params, but the function seems more flexible, in general.