Custom contextual loggers #2319
Conversation
1. added ability to create custom loggers for structs and add custom context 2. implements custom loggers for a few selected structs
sparrc
force-pushed
the
logging-refactor-4
branch
from
87cc1a9
to
e9fc7ab
Compare
sparrc
force-pushed
the
logging-refactor-4
branch
from
e9fc7ab
to
2c8f46c
Compare
agent/logger/log.go
Outdated
| cc, ok := context.CustomContext().(map[string]string) | ||
| var customContext string | ||
| if ok && len(cc) > 0 { | ||
| var sortedContext []string | ||
| for k, v := range cc { | ||
| sortedContext = append(sortedContext, k+"="+v) | ||
| } | ||
| sort.Strings(sortedContext) | ||
| customContext = " " + strings.Join(sortedContext, " ") | ||
| } | ||
| return fmt.Sprintf(`level=%s time=%s msg=%q module=%s%s | ||
| `, level.String(), context.CallTime().UTC().Format(time.RFC3339), message, context.FileName(), customContext) |
There was a problem hiding this comment.
this and lines 74-82 share enough in common that I'd like to see them as a separate function. (You might have done this in one of the later commits... If so, please disregard. I'm going through in chronological order.)
agent/logger/log.go
Outdated
| if _, ok = cc["module"]; !ok { | ||
| cc["module"] = context.FileName() | ||
| } | ||
|
|
||
| var ccStr string | ||
| var ccSorted []string | ||
| for k, v := range cc { | ||
| ccSorted = append(ccSorted, k+"="+v) | ||
| } | ||
| sort.Strings(ccSorted) | ||
| ccStr = " " + strings.Join(ccSorted, " ") | ||
| return fmt.Sprintf(`level=%s time=%s msg=%q%s | ||
| `, level.String(), context.CallTime().UTC().Format(time.RFC3339), message, ccStr) |
There was a problem hiding this comment.
could this and lines 80-91 be a single function with a contextSort=True flag?
There was a problem hiding this comment.
I've refactored out some of the code that I think made the most sense. Didn't make the sorting part reusable because that only applies to the logfmt formatter and not the JSON. That part would have been messy to factor out because they require formatting the 'context' map differently to match the desired output.
| @@ -1392,7 +1392,7 @@ func (engine *DockerTaskEngine) applyContainerState(task *apitask.Task, containe | |||
| } | |||
| metadata := transitionFunction(task, container) | |||
| if metadata.Error != nil { | |||
| seelog.Infof("Task engine [%s]: error transitioning container [%s] to [%s]: %v", | |||
| seelog.Errorf("Task engine [%s]: error transitioning container [%s] to [%s]: %v", | |||
There was a problem hiding this comment.
do we know full implication of this change?
There was a problem hiding this comment.
As it's logging a non-nil error I thought it more appropriate to be logging at the ERROR level
This reverts commit 52a57ff.
Summary
closes #2284
logger.ContextualImplementation details
Testing
unit tests were updated and overall logging change manual tests were executed.
New tests cover the changes: yes
Description for the changelog
Custom loggers for structs with consistent context
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.