logging

[paddlesteamer]

The package log is very limited. Only has Print, Fatal, Panic methods and I think the usage of Fatal and Panic should be avoided.

With fmt, we get to separate stdin and stderr outputs but still, it would be nice to have more indicators on logs.

So I suggest using one of the logging libraries here. It would be nice to have debug, info, warning and error levels.

debug is for verbose messages like the ones in the fs.go:

cloudstash/internal/fs/fs.go

Lines 27 to 29 in 928046f

	func (r *CloudStashFs) GetAttr(ino int64, info *fuse.FileInfo) (*fuse.InoAttr, fuse.Status) {
	fmt.Printf("getattr ino: %d\n", ino)

info should be used for information obviously :D. We don't have those in the code right now but I think we can use it for the messages like 'file x is uploaded to y'.

warning should be used for handled errors. For example, the error here is handled:

cloudstash/internal/manager/manager.go

Lines 345 to 349 in 928046f

	checksum, err := crypto.MD5Checksum(tmpfile)
	if err != nil {
	tmpfile.Close()
	os.Remove(tmpfile.Name())
	return nil, fmt.Errorf("couldn't compute md5 checksum of newly created file: %v", err)

We can log this error as a warning where it returned.

error is for fatal errors that shouldn't happen. There we can log as error and do a clean exit (unmount, wait for ongoing network operations, etc.)

[utkuufuk]

@paddlesteamer I think it would make sense to first decide where this log utility will be used first. Is it going to be used everywhere including the internal packages, or is it only going to be used from the main package?

If we're handle logging from the main package only, we could come up with our own simple helper function instead of introducing a new dependency.

Here's a very simple version of the helper function:

func log(msg, level string) {
  // check verbosity level from config

  // if verbose, write debug logs too, skip them otherwise

  // if error, write to os.Stderr
  // write to os.Stdout otherwise
}

func info(msg string) {
  log(msg, "info")
}

func error(msg string) {
  log(msg, "error")
}

...

[paddlesteamer]

I think we should use it everywhere. For example, it's not possible to log debug messages in the main. Also, the package manager (functions running on background) and fs have their own prints. And I don't think we should avoid dependencies like this. If it's a simple native go library, if it goes into binary without an outside dependency, we can use it.

[utkuufuk]

@paddlesteamer What I really meant was that maybe we could just get rid of debug logs altogether. I agree that we can use a 3rd party dependency if we decide to keep debug logs though 👍

The downside of writing info/debug logs is that it could take a lot of space in the user's machine in the long run, and they usually do not make sense to the user anyway.

Info/debug logs are for developers only, so it feels kind of wrong to introduce a dependency to our internal packages just so that we can print stuff to the console for development purposes.

What I'd do instead is, I would just delete all the existing debug/print lines that are not helpful to the user, and add them temporarily (without committing) while debugging the app. This is just my personal opinion though 🙂

Another approach could be that we just stop using log altogether, and only use fmt to write logs to stdout and stderr without writing anything to a file. That way, the logs won't take up any space in the user's machine and they won't even see them. We can run the app during development by directing stdout & stderr to a log file so that they are useful to us.

Let me know what you think! The above comments are just me thinking out loud 😄

[paddlesteamer]

Info/Debug logs are usually filtered in the releases but still kept in the binaries and could be enabled by --debug option or some interface in the program. It is useful for understanding the errors the users have. That's why a lot of popular programs have debug options in their releases. Also, they're good for developers who want to understand how code works.

For example, did you know you can run google chrome with debugging and logging enabled? Or firefox? I think you could find more if you search open source programs.

That's why I think we should keep them. And it doesn't needs to be logged on file, it may output to the console just with log levels. I don't mean logging like nginx does but it's best practice to have debug logs.

[utkuufuk]

That's why I think we should keep them. And it doesn't needs to be logged on file, it may output to the console just with log levels. I don't mean logging like nginx does but it's best practice to have debug logs.

@paddlesteamer This is actually similar to what I proposed in the end, but with multiple log levels:

Another approach could be that we just stop using log altogether, and only use fmt to write logs to stdout and stderr without writing anything to a file. That way, the logs won't take up any space in the user's machine and they won't even see them. We can run the app during development by directing stdout & stderr to a log file so that they are useful to us.

It sounds reasonable to use a log package so that we have multiple levels but not write anything to a file.
Users can view the logs on the console or pipe them into a file if they wish to do so. I'm sold 😄 👍

[paddlesteamer]

So the question is which logging library then?

[paddlesteamer]

I liked https://github.com/op/go-logging and https://github.com/sirupsen/logrus but even though logrus is more handsome it's verbose (needs fields specified everytime) so I'm voting for go-logging.

...or logrus with a wrapper :)

[utkuufuk]

@paddlesteamer
https://github.com/op/go-logging isn't being maintained for a long time and it has 1.6k stars.
https://github.com/sirupsen/logrus is actively maintained an has 15.3k stars.

So my vote goes to logrus.

[paddlesteamer]

logrus it is.

[utkuufuk]

needs fields specified everytime

@paddlesteamer To my understanding, you pass fields only if you want to.

[paddlesteamer]

needs fields specified everytime

@paddlesteamer To my understanding, you pass fields only if you want to.

Yeah it seems so