Support non-colorized log output, logging refactor #420
Conversation
| @@ -288,7 +289,7 @@ func (h *PollerMap) EnsurePolling(pid PollerID, accessToken, v2since string, isS | |||
|
|
|||
| // replace the poller. If we don't need to wait, then we just want to nab to-device events initially. | |||
| // We don't do that on startup though as we cannot be sure that other pollers will not be using expired tokens. | |||
| poller = newPoller(pid, accessToken, h.v2Client, h, logger, !needToWait && !isStartup) | |||
There was a problem hiding this comment.
This logger has contextual information like the user ID and device ID which is now lost.
There was a problem hiding this comment.
This was a conscious decision. In the implementation of newPoller() there are calls like this:
logger.Error().Str("user", p.userID).Str("device", p.deviceID).Msgf("%s. Traceback:\n%s", panicErr, debug.Stack())
where context is sent to the log output even though the logger already has that context. My worry is this happened because it wasn't clear that the logger already has that context or because it wasn't added in an earlier (and now-deleted) path or just to ensure the context is there in case one day it's handed a logger without it.
In any case, it makes more sense to me to add those values locally. There are several other logging calls in newPoller() that don't explicitly add user/device to output but it doesn't seem particularly useful to have it at those points. If that's not the case, my inclination would be to add those values locally instead of far up the call stack.
Going back to using the per-file local loggers would mean plumbing each of the dozen or so zerolog.New() call sites with the new environment variable logic.
I've rebased the PR on the current main branch to accommodate new code changes and removed the unused logger formal parameter. Please let me know if this works for you.
There was a problem hiding this comment.
I would rather we double reported the user ID and device ID than risked not reporting the user ID / device ID at all. By having a poller-local logger instance we guarantee that those keys are always logged. We rely on these keys existing for debugging as we often search for these IDs when that user reports a problem.
I'm happy with the global logger usage everywhere else, but the poller really should be using the contextual one please.
Three things are happening here:
1) An environment variable is added to allow for non-colorized text to
be output. This is so /usr/sbin/daemon can direct stdout to syslog
without escaped control sequences:
Apr 15 14:48:52 orla syncv3[41839]: ^[[90m14:48:52^[[0m ^[[32mINF^[[0m Poller: accumulated data ^[[36mdevice [events,changed,left,account]=^[[0m[0,0,0,0] ^[[36mdevice_id=^[[0mQRZLYYCRTO ^[[36mrooms [timeline,state,typing,receipts,invites]=^[[0m[0,0,0,0,0] ^[[36muser_id=^[[0m@doug:yutz.horph.com
I would have preferred a command-line switch instead of the env var
but am following the existing pattern.
2) zerolog use is refactored. Since there really are no different
types of log output and only minimal use of additional context, it
makes sense to use zerolog's existing global logger. This simplifies
things a bit both textually and cyclomatically and allows for central
changes to the logger output based on the new env var.
3) A `go fmt ./...` changed a few very minor things.
Three things are happening here:
An environment variable is added to allow for non-colorized text to be output. This is so /usr/sbin/daemon can direct stdout to syslog without escaped control sequences:
Apr 15 14:48:52 orla syncv3[41839]: ^[[90m14:48:52^[[0m ^[[32mINF^[[0m Poller: accumulated data ^[[36mdevice [events,changed,left,account]=^[[0m[0,0,0,0] ^[[36mdevice_id=^[[0mQRZLYYCRTO ^[[36mrooms [timeline,state,typing,receipts,invites]=^[[0m[0,0,0,0,0] ^[[36muser_id=^[[0m@doug:yutz.horph.com
I would have preferred a command-line switch instead of the env var but am following the existing pattern.
zerolog use is refactored. Since there really are no different types of log output and only minimal use of additional context, it makes sense to use zerolog's existing global logger. This simplifies things a bit both textually and cyclomatically and allows for central changes to the logger output based on the new env var.
A
go fmt ./...changed a few very minor things.Pull Request Checklist