Add rotated logs support #5
Conversation
|
Thx!
makes sense
Fine with me. btw, looks like lumberjack has some issue with dynamically created logs (leaking goroutine on Close), so this decision is justified.
I think it is really easy to do if you wrap with your prefixer implementation of A few more things missing:
|
|
@umputun what do you see as a prefix? I also see It's not so straightforward how to get it in |
I think we can have the command as a prefix limited by some reasonable maxlen (16?), i.e.
In this case, you already have the command, so nothing new should be passed in |
3d380dc to
8b4138c
Compare
|
Now it's like this I also added some tests, updated README and moved arguments under "log" group. |
|
yep, exactly what I meant |
| return logPrefixer | ||
| } | ||
|
|
||
| func (p *LogPrefixer) Write(data []byte) (int, error) { |
There was a problem hiding this comment.
do we really need all of this? How about this:
if len(data) >0 {
p.Write(p.prefix)
_, err := p.Write(data)
return len(data), err
}
There was a problem hiding this comment.
But it will only prefix only first line of the command output.
There was a problem hiding this comment.
It would probably work if command produces output line by line, means internally calling Write for every line. Which is not the case even for ls -l, just tried.
2020/07/28 02:05:05 INFO executing: "ls -l"
{ls -l} total 17576
-rw-r--r-- 1 egor staff 650 Jul 26 14:40 Dockerfile
-rw-r--r-- 1 egor staff 1842 Jul 26 14:32 Dockerfile.artifacts
-rw-r--r-- 1 egor staff 1064 Jul 26 14:30 LICENSE
...
2020/07/28 02:05:05 INFO completed ls -
So all these complications are to prefix each line, no matter how many of them are contained in one data portion.
The required enhancement was described in issue #4.
New flags help (from
--helpoutput)Some considerations:
Looks like
lumberjackhas a bit slower development pace now indeed. But it is still quite powerful and very popular solution, let's hope it will get some maintainers' or community attention soon. It's always possible to switch anyway.I think it can be reasonable to move
--logand--dbginside of newlogflags group. Having something like this looks more consistent for me:But I feel like such decision is out of the scope of the current pull request, can be done in some next PR easily.
I decided to make a single file log, just as usual cron does, for example. I see a lot of questions for having separate log files per job: log file name, directory structure, separate or common rotation. Potentially it can be done, but the scope is a lot bigger and the profit is not very understandable.
It looks to me that prefixing each output line is definitely possible, but not very easy thing to do with the current logger, because it doesn't have a concept of "child" loggers and custom prefixes for them (please correct me if I'm wrong).
I think current output maybe not ideal, but still a good start for future enhancements.