Adding custom attribute support for http middleware #26
Conversation
| customWriter := &responseWriter{ResponseWriter: w} | ||
| // If any fields in CustomAttributes are nil, do not call them to avoid errors | ||
| addExtraAttributes := validateCustomAttributes(&l.customAttributeAssigner) | ||
| var extraAttributes map[string]interface{} |
There was a problem hiding this comment.
this will be initialized to the zero value, correct? For maps it's nil. So this should just be initialized to an empty map
| l.LogRequestEnd(ctx, r, "finished call", customWriter.statusCode, latency, updatedAttrs) | ||
| } | ||
|
|
||
| func (l *loggingMiddleware) BuildLoggingAttributes(ctx context.Context, r *http.Request, extra ...interface{}) []interface{} { |
There was a problem hiding this comment.
any particular reason why extra changed to an interface? Could you just keep the map[string]interface{} type throughout?
http/server/logging/logging.go
Outdated
| type loggingFunc func(w http.ResponseWriter, r *http.Request, attrs map[string]interface{}) map[string]interface{} | ||
| type CustomAttributes struct { | ||
| AttributeInitializer *initFunc // sets keys for custom attributes at the beginning of ServeHTTP() | ||
| AttributeAssigner *loggingFunc // assigns values for custom attributes after request has completed |
There was a problem hiding this comment.
Should we provide default Attribute initializers and assigners?
| now: time.Now, | ||
| logger: *logger, | ||
| source: source, | ||
| customAttributeAssigner: customAttributeAssigner, |
There was a problem hiding this comment.
Do you support the initializer and assigner funcs to be nil? If not then you should assign them to default functions if not provided
|
|
||
| var updatedAttrs map[string]interface{} | ||
| if addExtraAttributes { | ||
| updatedAttrs = (l.customAttributeAssigner.AttributeAssigner)(w, r, extraAttributes) |
There was a problem hiding this comment.
Why do we pass in the original http.ResponseWriter rather than the customWriter here?
| } | ||
|
|
||
| // sets default source "ApiRequestLog" | ||
| func setSourceIfEmtpy(source *string) { |
| type CustomAttributes struct { | ||
| AttributeInitializer initFunc // sets keys for custom attributes at the beginning of ServeHTTP() | ||
| AttributeAssigner loggingFunc // assigns values for custom attributes after request has completed | ||
| } |
There was a problem hiding this comment.
Consider renaming the struct to something like "AttributeManager" and the methods to be along the lines of "Initialize" and "Assign"
| type CustomAttributes struct { | ||
| AttributeInitializer initFunc // sets keys for custom attributes at the beginning of ServeHTTP() | ||
| AttributeAssigner loggingFunc // assigns values for custom attributes after request has completed | ||
| } |
There was a problem hiding this comment.
Why don't we just set the keys and assign the values in the same function after the request has completed?
This PR allows extra attributes to be assigned to the logger.
CustomAttributes struct:
loggingMiddleware takes source as a field. If specified, it is assigned. Otherwise, the default is "ApiRequestLog"