Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@pscott
Copy link
Contributor

@pscott pscott commented Jul 21, 2020

This PR adds the option to rotate logs. It does so by switching to another crate for loggin (previously env_logger, now flexi_logger).

A similar PR was already merged and reverted.

I would ask the reviewers to:

  1. Thoroughly read the original issue.
  2. Test and play a little bit with the configurations (using --log, --log-directory, --log-age, and --log-size). (cc @ddorgan )

This PR also fixes the problem of:

  • AgeOrSize: if --log-age AND --log-size are specified, the log rotation will happen once either the AGE or the SIZE gets reached.
  • Quitting if the Logger error'd: previously we would simply error and not start the client. Now the client simply prints an error message.

Closes #5926
polkadot companion: paritytech/polkadot#1450

@pscott pscott added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 21, 2020
@pscott pscott requested review from bkchr and tomaka July 21, 2020 18:28
@pscott pscott self-assigned this Jul 21, 2020
@pscott
Copy link
Contributor Author

pscott commented Jul 21, 2020

Please note that logger.rs was added by #6564 but not properly reverted by #6627 because of #6620 .. :p

So I would add the reviewers to make sure the code is sound in logger.rs too :)

@ddorgan
Copy link
Member

ddorgan commented Jul 22, 2020

@pscott what happened to doing it the standard linux way? e.g. a SIGUP flushes and logrotate rotates.

@pscott
Copy link
Contributor Author

pscott commented Jul 22, 2020

@pscott what happened to doing it the standard linux way? e.g. a SIGUP flushes and logrotate rotates.

Since this comment said:

Though custom log rotation would still be useful I think. Not all platforms have journald and for and end users it might be easier to use something build-in.

I thought using flexi_logger with custom log rotation was the way to go. I think there's no clear consensus atm (see original issue), so this PR is here because the work was already done, but we could as well decide not to merge it.

If I understand @ddorgan you'd want SIGUP to flush and rotate? No custom "rotation" provided out of the box?

PS: stupid question but how would this work on windows? (I have no idea how logs are usually handled on windows...)

@pscott
Copy link
Contributor Author

pscott commented Jul 22, 2020

Since init_logger now returns a Result and requires an extra parameter, I tagged breaksapi. Feel free to remove :)

@gnunicorn
Copy link
Contributor

closing because of inactivity.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log rotation

4 participants