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

Add custom log rotator option and hourly log rotation #420

Merged
merged 7 commits into from
Jan 16, 2018

Conversation

windkit
Copy link
Contributor

@windkit windkit commented Oct 12, 2017

I am seeking to use lager as the logging back-end for LeoFS, so would like to open up the possibility of custom log rotator.

This should not change the current behaviour, only when user opt-in with custom log rotator

@jadeallenx
Copy link
Member

Thanks for the PR. We will review this during our normal triage session, Oct 19.

@jadeallenx
Copy link
Member

Thanks for your patience! This is great work. Thank you for the PR!

We like this patch and would like to merge it, but first we would like to see more documentation for the features being added here:

  1. There should be a section in the README describing how to write a custom log rotation module that conforms to the behavior included in this PR.
  2. The section that discusses file rotation ought to be extended to include the hourly rotation feature.

Thanks again. Lookjng forward to getting this into the project.

@windkit
Copy link
Contributor Author

windkit commented Oct 22, 2017

@mrallen1 Thanks for taking a look on this

@jadeallenx
Copy link
Member

Sorry to do this, but would you mind rebasing this PR? If you can get to it, I'll merge it. Thank you.

@windkit
Copy link
Contributor Author

windkit commented Jan 1, 2018 via email

@Vagabond
Copy link
Member

Vagabond commented Jan 1, 2018

If you're willing to refactor the rotator into its own module (get it out of util now it is pluggable), that'd be great too. Otherwise we can do it after this merges.

@windkit
Copy link
Contributor Author

windkit commented Jan 3, 2018 via email

@windkit windkit force-pushed the for-leofs branch 2 times, most recently from 6027aac to 12171e1 Compare January 4, 2018 05:50
@windkit
Copy link
Contributor Author

windkit commented Jan 4, 2018

@mrallen1 @Vagabond I have rebased the PR and refactored the rotator into a separate module.
Could you help to review this again?

While the code passed the unit test from my environment with OTP 18.3, it fails in Travis with

  lager_test_backend: async_threshold_test_ (async threshold works)...*failed*
in function lager_test_backend:'-async_threshold_test_/0-fun-6-'/0 (test/lager_test_backend.erl, line 1768)
in call from lager_test_backend:'-async_threshold_test_/0-fun-10-'/0 (test/lager_test_backend.erl, line 1768)
**error:{assertMatch,[{module,lager_test_backend},
              {line,1768},
              {expression,"ets : lookup ( async_threshold_test , sync_toggled )"},
              {pattern,"[ { sync_toggled , N } ] when N > 0"},
              {value,[{sync_toggled,0}]}]}
  output:<<"">>
  lager_test_backend: high_watermark_test_ (Nothing dropped when error_logger high watermark is undefined)...[0.602 s] ok
  lager_test_backend: high_watermark_test_ (Mostly dropped according to error_logger high watermark)...[1.102 s] ok
  lager_test_backend: high_watermark_test_ (Non-notifications are not dropped)...*failed*
in function lager_test_backend:'-high_watermark_test_/0-fun-8-'/0 (test/lager_test_backend.erl, line 1867)
**error:{assert,[{module,lager_test_backend},
         {line,1867},
         {expression,"count ( ) < 10"},
         {expected,true},
         {value,false}]}
  output:<<"">>

Do you have any clues what do these errors mean?

@Vagabond
Copy link
Member

Vagabond commented Jan 4, 2018

It's a race condition, not sure why it hits 18.3 so consistently, it is not a concern.

README.md Outdated
-------------------
Custom log rotator could be configured with option for `lager_file_backend`
```erlang
{rotator, lager_util}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, sorry for that, let me fix it. Rebase later

@Vagabond
Copy link
Member

Vagabond commented Jan 4, 2018

Looking at this now, what would a custom rotator look like, given it seems to mainly just handle file operations, not the decision around when to rotate.

@jadeallenx
Copy link
Member

Just want to say "thank you!" for rebasing and pulling all of the changes into their own module.

@windkit
Copy link
Contributor Author

windkit commented Jan 5, 2018

@Vagabond To us, we want to change how the log files are named.
https://github.com/leo-project/leo_logger/blob/develop/src/leo_logger_rotator.erl

@jadeallenx
Copy link
Member

OK, this is a good start. Ultimately, I think we want all the rotation logic to be encapsulated inside a module/behaviour. So I am going to merge this and open an issue to migrate the rotation decision making inside the same module/behaviour.

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

Successfully merging this pull request may close these issues.

3 participants