Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileWriter Filename #34

Closed
ogimenezb opened this issue Jun 12, 2021 · 12 comments
Closed

FileWriter Filename #34

ogimenezb opened this issue Jun 12, 2021 · 12 comments

Comments

@ogimenezb
Copy link
Contributor

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

  • If you are testing
  • If you have a console command options
  • If you need to stop and start the project a couple of times
  • ...

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
Copy link
Owner

phuslu commented Jun 12, 2021

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

natefinch/lumberjack#115 (comment)

natefinch/lumberjack#98 (comment)

@ogimenezb
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

phuslu commented Jun 13, 2021

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

golang/go#22874
ipfs/kubo#4956

@ogimenezb
Copy link
Contributor Author

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
Copy link
Owner

phuslu commented Jun 13, 2021

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

@ogimenezb
Copy link
Contributor Author

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
Copy link
Owner

phuslu commented Jun 13, 2021

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

@ogimenezb
Copy link
Contributor Author

Thanks!

@ogimenezb
Copy link
Contributor Author

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
Copy link
Owner

phuslu commented Jun 13, 2021

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
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants