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

Figure out logging output #22

Closed
2 tasks
KodrAus opened this issue Sep 18, 2017 · 3 comments
Closed
2 tasks

Figure out logging output #22

KodrAus opened this issue Sep 18, 2017 · 3 comments
Milestone

Comments

@KodrAus
Copy link
Collaborator

KodrAus commented Sep 18, 2017

We've got a couple of open questions related to the logging sink:

It would be good to iterate towards a design that satisfies all these things, without complicating things too much. I think it might be worth looking at opening up the Target enum into a trait, and seeing where that gets us.

Issues to file

  • Support UTC timestamps in default format
  • Lock/buffer stderr
@tailhook
Copy link
Contributor

I have a couple of points:

  1. Alternative sinks do not belong this crate I think
  2. Buffering writes is a must. Not sure how to handle buffer though. It would be cool to cache per-thread buffer (look at regex crate for inspiration)
  3. It's unclear how to combine color output with custom formatting
  4. The default format isn't good. I think timestamps must be there by default (this might require large time-formatting library and may create a timezone issue; personally I'd ISO-formatted UTC time always)

@KodrAus
Copy link
Collaborator Author

KodrAus commented Sep 24, 2017

Thanks for your thoughts @tailhook!

Alternative sinks do not belong this crate I think

I agree, but it would be fairly easy to add support for users defining them with a Target::Write(Box<Write>) or some such. It would need some more design than that though.

Buffering writes is a must

We could handle this with a wrapper over stderr that's passed when that's the target, using thread-local storage even. Would changing this line to pass stderr().lock() help? I suppose not if the underlying handle it's locking on isn't buffered anyways? I did write a little demo program that threw concurrent error! logs and didn't observe any garbled output. Do you have a sample that demonstrates this we can use to verify a fix?

EDIT: I suppose you'd need a format that calls write multiple times for it to garble. Which someone could do, so we should probably pass a locked stderr anyways.

Any other thoughts you have here would be much appreciated!

It's unclear how to combine color output with custom formatting

Yeh I'm not too sure about this either... We'd have to build some infrastructure around the type passed to the format to support colour if it's available. Seems like a lot of work for not a whole lot of gain. I would call this a low priority.

The default format isn't good. I think timestamps must be there by default

I agree, although they do add some noise. ISO-formatted UTC would make the most sense.

So my thoughts thus far are that we don't need to fundamentally change the way env_logger works now, but finding a way to make the happy-path as happy as possible should be priorities before 1.0.

@KodrAus KodrAus added this to the 0.3.x milestone Oct 23, 2017
@KodrAus
Copy link
Collaborator Author

KodrAus commented Oct 30, 2017

I've logged #30 and #31 for cleaning up the issues raised here.

If there's anything else we can open this back up. Thanks!

@KodrAus KodrAus closed this as completed Oct 30, 2017
epage added a commit to epage/env_logger that referenced this issue Sep 27, 2024
…mat-args

Have clippy warn about uninlined format arguments
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

2 participants