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

Logging to a file? #178

Open
osa1 opened this issue Aug 21, 2020 · 13 comments
Open

Logging to a file? #178

osa1 opened this issue Aug 21, 2020 · 13 comments

Comments

@osa1
Copy link

osa1 commented Aug 21, 2020

Would it make sense to add a Target variant for logging to a file? It doesn't make sense to print to stdout or stderr in TUI applications as that breaks the TUI, and having to ask users to redirect stderr to a file is inconvenient.

Context: osa1/tiny#238

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

Sure, sounds like a simple enough addition. @sirwindfield wdyt?

One thing to note is that this can't be done in a backwards-compatible manner, because Target currently implements Copy.

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

If we decide to do this, we might want to copy what simplelog does and accept anything that is writable.

@osa1
Copy link
Author

osa1 commented Aug 26, 2020

One thing to note is that this can't be done in a backwards-compatible manner, because Target currently implements Copy.

Send and Sync will also be lost if we simply put a file handle in a Target variant, but they can be maintained by wrapping the File with Arc<Mutex<..>>.

Also, Target is currently exposed to users so adding a new variant to it is a breaking change that requires major version bump. So regardless of the traits I think this will require a version bump.

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

Also, Target is currently exposed to users so adding a new variant to it is a breaking change that requires major version bump. So regardless of the traits I think this will require a version bump.

Right. Could potentially be made non-exhaustive with the next version.

@KodrAus
Copy link
Collaborator

KodrAus commented Aug 26, 2020

I’ve previously punted on supporting file logging, because it’s a bit of a can of worms. You can’t really just pipe records into a file, you end up needing to have custom file patterns, rolling files with configurable sizes, deal with transient IO errors and slow writes. It’s definitely possible, and has been done plenty before, but it always ends up being a whole stack of its own to maintain 🙂

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

@KodrAus What about adding a note that we don't do any of this? I think just piping records into a file would work well for short-lived programs, even if it quickly breaks down for daemons or server applications.

@KodrAus
Copy link
Collaborator

KodrAus commented Aug 26, 2020

Maybe we could design it to indirectly support files, instead of baking it in? So it might look something like:

enum Target {
    Pipe(Box<dyn Write>),
    ..
}

which we then wrap in a Mutex in the logger and stream records to. You could use a File or rolling file wrapper and env_logger wouldn’t have to build its own rolling file API?

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

Yeah, I wouldn't want to bake in file either, that's why I mentioned how simplelog does it. Box<dyn Write> should be good. I didn't even think of the possibility of building log rotation into a File wrapper that implements Write, but that sounds like a really nice approach for people who need rotation. 👍

@mainrs
Copy link
Contributor

mainrs commented Aug 26, 2020

Ye, I wouldn't want to just support a File object alone. We should definitely abstract it somehow away. Box<dyn Write> definitely goes into the right direction. I'd probably use that one if nothing better comes up.

Just to be sure here, non_exhaustive forces users to basically add a catch all into their matches, doesn't it? At least that would allow us to extend functionality later on if needed without introducing another breaking change.

@jplatte
Copy link
Contributor

jplatte commented Aug 26, 2020

Just to be sure here, non_exhaustive forces users to [basically] add a catch all into their matches, doesn't it?

That's exactly what it does.

@saona-raimundo
Copy link

Hi!

I tried to work out logging to a file based on custom_target example, but I could not.
Here is my attempt. I was expecting to have the logging written in the file and not in the command line. I simply get an empty file (from when it is created): there are no records in it.

Do you mind telling me my mistakes, or adding an example on how to do this?
Thanks in advance!

@TilCreator
Copy link
Contributor

Have a look at #208, the Pipe Target sadly is broken, because we forgot to actually test it, ups. ^^
#211 already fixes this, but it's not merged jet.

@saona-raimundo
Copy link

Thank you very much!
I will test it when it is merged then :)

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

No branches or pull requests

7 participants