phuslu / log

Fastest structured logging
MIT License
672 stars 44 forks source link

FileWriter Filename #34

Closed ogimenezb closed 3 years ago

ogimenezb commented 3 years ago

This could be considered as an improvement? maybe?

When you output to file, you have name.timestamp.ext that's ok... but when there are many cases when this is a little bit unconfutable. I think I should be name.ext and when rotated name.timestamp.ext

When rotation happens, because of whatever condition, having timestamp is ok, but if there are no conditions that have been meet it should be just the name and append data.

For compatibility issues we could implement an AutoRotateOnInitialize (bool), or OnlyTimestampOnRotate (bool) a simpler name...

phuslu commented 3 years ago

I wrote my ideas and thoughts here, please take a look, comments and feedback are welcome.

https://github.com/natefinch/lumberjack/issues/115#issuecomment-739835573

https://github.com/natefinch/lumberjack/issues/98#issuecomment-747849432

ogimenezb commented 3 years ago

Ok, I understand.

How about only rotate when conditions are meet. If MaxSize is not meet why rotate on initialize?

I have just tried using TimeFormat: "2006-01-02" and it appends on the same file. If the file is bigger than MaxSize it does not rotate. So rotation is only based on date format. Maybe implement a check on TimeFormat and if is the same then add %_1?

Finally I have been testing on Windows since most of out users use Windows. Our servers are on Linux, can't (can not) control what users prefer... That's why I don't see the symlink on logs.

ogimenezb commented 3 years ago

Have tested on Windows.

err := os.Link("plog.2021-06-12.log", "link_plog.log") Works Fine!

err = os.Symlink("plog.2021-06-12.log", "sym_plog.log") Error: symlink plog.2021-06-12.log sym_plog.log: A required privilege is not held by the client.

Don't know if it needs to be Hard or Soft but maybe we can check if Program is on Windows && Admin and create a Hard or Soft?

phuslu commented 3 years ago

I remember os.Symlink is not supported well on windows.

https://github.com/golang/go/issues/22874 https://github.com/ipfs/go-ipfs/pull/4956

ogimenezb commented 3 years ago

Should we add a note on Readme about being admin on Windows or check if os is Windows and Error == "A required privilege is not held by the client." and use os.Link

phuslu commented 3 years ago

Okay, please let me have a try to fix symlink on windows...

ogimenezb commented 3 years ago

I don't think it's possible without being an admin. You can only do hard.

tester-windows: mklink
Creates a symbolic link.

MKLINK [[/D] | [/H] | [/J]] Link Target

        /D      Creates a directory symbolic link.  Default is a file
                symbolic link.
        /H      Creates a hard link instead of a symbolic link.
        /J      Creates a Directory Junction.
        Link    Specifies the new symbolic link name.
        Target  Specifies the path (relative or absolute) that the new link
                refers to.

tester-windows: mklink soft.log plog.2021-06-12.log
You do not have sufficient privilege to perform this operation.

tester-windows: mklink /H soft.log plog.2021-06-12.log
Hardlink created for soft.log <<===>> plog.2021-06-12.log
phuslu commented 3 years ago

Yes, I also found that. I will add a note to README, thanks!

ogimenezb commented 3 years ago

Thanks!

ogimenezb commented 3 years ago

I have just tried using TimeFormat: "2006-01-02" and it appends on the same file. If the file is bigger than MaxSize it does not rotate. So rotation is only based on date format. Maybe implement a check on TimeFormat and if is the same then add %_1?

How about this, when the name is the same, the MaxSize is not respected.

phuslu commented 3 years ago

I can add a "fix" to respect the MaxSize

diff --git a/file.go b/file.go
index 6f9b2f7..7427585 100644
--- a/file.go
+++ b/file.go
@@ -223,6 +223,9 @@ func (w *FileWriter) create() (err error) {
        return err
    }
    w.size = 0
+   if st, err := w.file.Stat(); err == nil {
+       w.size = st.Size()
+   }

    os.Remove(w.Filename)
    if !w.ProcessID {

But the problem is, when the rotation happens, it will not create a new file, because of TimeFormat: "2006-01-02 always be same in a day.

In that case, may you also need specifies the

    // Cleaner specifies an optional cleanup function of log backups after rotation,
    // if not set, the default behavior is to delete more than MaxBackups log files.
    Cleaner func(filename string, maxBackups int, matches []os.FileInfo)

to rename the original file to <name>.<timestamp>.1

ogimenezb commented 3 years ago

Thanks!