Improve logger #566
Improve logger #566
Conversation
Itxaka
commented
Feb 18, 2025
Itxaka
commented
- Try to log to jorunald if available
- fallback to file writing if not
- write to single file instead of appending times
- add a locking mechanism so several processes cant write to the same file
- prepend the PID of the process if we are running outside journald for easy tracking of grouped messages
- Try to log to jorunald if available - fallback to file writing if not - write to single file instead of appending times - add a locking mechanism so several processes cant write to the same file - prepend the PID of the process if we are running outside journald for easy tracking of grouped messages Signed-off-by: Itxaka <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
=======================================
Coverage 52.55% 52.55%
=======================================
Files 19 19
Lines 1998 1998
=======================================
Hits 1050 1050
Misses 818 818
Partials 130 130 ☔ View full report in Codecov by Sentry. |
|
I'm not sure the locking mechanism is needed: rs/zerolog#29 |
Oh nice, that's a good find. If we can reduce complexity in here I rather do so |
| // Functions to implement the logger.Interface that most of our other stuff needs | ||
|
|
||
| func (m KairosLogger) Infof(tpl string, args ...interface{}) { | ||
| if !m.journald { |
There was a problem hiding this comment.
Something like this might offer some de-duplication:
func (m KairosLogger) withLock(fun func(tpl string, args ...interface{}), tpl string, args ...interface{}) {
if !m.journald {
m.fileLock.Lock()
defer m.fileLock.Unlock()
tpl = fmt.Sprintf("[%v] ", os.Getpid()) + tpl
}
fun(tpl, args)
}
func (m KairosLogger) Errorf(tpl string, args ...interface{}) {
m.withLock(func(tpl string, args ...interface{}) {
m.Logger.Error().Msg(fmt.Sprintf(tpl, args...))
}, tpl, args...)
}
(a similar one for Info and the rest)
but I can't decide if it's clearer or not. Maybe if Info and Infof versions could also be consolidated somehow it would make even more sense.
There was a problem hiding this comment.
meh, it's messing with my head. Ignore it unless you can find a cleaner version of it.
There was a problem hiding this comment.
we need to expose info and infof as that makes it backwards compatible with all our code without having to go and change all the lines in the world :)
Or we could tackle the current problem and then rework this, and cut a higher version of the sdk and start moving towards a simpler logger? We have too much baggage from the older ones still around :(
There was a problem hiding this comment.
I agree. If we can DRY this code a bit in a backwards compatible way, let's go for it. Otherwise it's a bigger refactoring, outside the scope of this PR.
| @@ -10,6 +10,7 @@ require ( | |||
| github.com/docker/docker v27.5.1+incompatible | |||
| github.com/edsrzf/mmap-go v1.2.0 | |||
| github.com/foxboron/go-uefi 69fb7dba244f | |||
| github.com/gofrs/flock v0.8.1 | |||
There was a problem hiding this comment.
something is wrong with indentation or Github doesn't style it correctly?
There was a problem hiding this comment.
seems a real issue, somehow I screwed the indentation
