-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added Support for the log appender roll-by-size-time to zip older files #259
Conversation
1. logname - you can override the name of the log file rather than using the EXE name, this means you don't have to call your EXE a different name, just name the winsw exe different. Default's the name to the EXE as before. 2. outfiledisabled - you can disable writing to the out file. Defaults to false. 3. errfiledisabled - you can disable writing to the error file. Defaults to false. 4. outfilepattern - you can choose the pattern of the out file. Defaults to .out.log. 5. errfilepattern - you can choos the pattern of the error file. Defaults to .err.log.
…led and errfilepattern. Created a new appender called roll-by-size-time see class RollingSizeTimeLogAppender, this appender supports rolling by time and size and rolling at a specific time each day. Added unit test for the new appender. Added a new option testwait which is similar to test but waits for the user to press any key before calling the stop method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request should also provide documentation for this new feature
<package id="ILMerge" version="2.14.1208" targetFramework="net20" /> | ||
<package id="log4net" version="2.0.8" targetFramework="net20" requireReinstallation="True" /> | ||
<package id="MSBuildTasks" version="1.4.0.88" targetFramework="net20" /> | ||
</packages> | ||
</packages> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, would be better to revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need the library ICSharpCode.SharpZipLib.dll to carry out zipping as the core code is still using Framework 2.0 and the Zip classes are only available from .Net Framework 4.5 +
src/Core/WinSWCore/LogAppenders.cs
Outdated
|
||
public RollingSizeTimeLogAppender(string logDirectory, string baseName, bool outFileDisabled, bool errFileDisabled, string outFilePattern, string errFilePattern, int sizeThreshold, string filePattern, TimeSpan? autoRollAtTime) | ||
public RollingSizeTimeLogAppender(string logDirectory, string baseName, bool outFileDisabled, bool errFileDisabled, string outFilePattern, string errFilePattern, int sizeThreshold, string filePattern, TimeSpan? autoRollAtTime, int? zipolderthannumdays, string zipdateformat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine, this project does not offer binary compatibility to API users so far
Documented the new fields zipolderthannumdays and zipdateformat
Added documentation for the two new zip elements |
Please let me know if you are happy to merge this to master or if there are still any other outstanding issues |
Yes, I still need to review it. Sorry for the delays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still drowning in the Jenkins project things, but I am more available now. Will do my best to finally test it. Some bits in docs should be fixed for sure though I agree there is no need to include it into the sample configurations provided in the project.
Bonus points for unit tests
doc/loggingAndErrorReporting.md
Outdated
@@ -56,6 +56,8 @@ Works in a combination of rotate size mode and rotate time mode, if the log file | |||
<sizeThreshold>10240</sizeThreshold> | |||
<pattern>yyyyMMdd</pattern> | |||
<autoRollAtTime>00:00:00</autoRollAtTime> | |||
<zipolderthannumdays>5</zipolderthannumdays> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to rename it to zipOlderThanNumDays
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/Core/WinSWCore/LogAppenders.cs
Outdated
@@ -328,13 +329,17 @@ public class RollingSizeTimeLogAppender : AbstractFileLogAppender | |||
public int SizeTheshold { private set; get; } | |||
public string FilePattern { private set; get; } | |||
public TimeSpan? AutoRollAtTime { private set; get; } | |||
public int? ZipOlderTthanNumDays { private set; get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it supposed to be ZipOlderThanNumDays
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
doc/loggingAndErrorReporting.md
Outdated
@@ -65,6 +67,10 @@ For example, in the above example, the log of Jan 1, 2013 gets written to `myapp | |||
The syntax of the autoRollAtTime is specified by [TimeSpan.ToString()](https://msdn.microsoft.com/en-us/library/1ecy8h51(v=vs.110).aspx). | |||
For example, in the above example, at the start of the day it will roll the file over. | |||
|
|||
The zipolderthannumdays can only be used in conjection with autoRollAtTime, provide the number of days of files to keep. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would split it to a separate documentation section so that it becomes explicit that the vars are optional.
I do not see specific reason why it's not supported in other rotating loggers, but fine with me if documented separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as code. will do some manual testing and hopefully ship it next week
Are you good to merge this into master? |
It's in my TODO list (somewhere on the top), I apologize for the delay. Unfortunately I have been unable to get to it yet, because it requires some spare time to test it + availability of a Windows development environment at the same time. So far I was unable to find time for that (Jenkins JEP-200, Google Summer of Code, etc.) If anybody is waiting for this change to be shipped, please do not hesitate to test the build and to report results. It may speedup the process. P.S: I am also looking for co-maintainers |
I have built my own version but would be nice to take it from nuget in the future, whenever you get to it would be great |
@@ -1,6 +1,7 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<packages> | |||
<package id="ICSharpCode.SharpZipLib.dll" version="0.85.4.369" targetFramework="net40" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be added to the Merge configuration so it is linked in the final executable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to ServiceWrapper/packages.config as well, is that what you meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ILMerge command in the csproj file's build section which links all the dlls into a single executable. This library needs to be referenced there. checkout winsw_dotNET4.csproj:142
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now there is some duplication in configs now. It may be confusing, but ILMerge is the only way to get such executable with the current project layout and .NET2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so this will be picked up automatically by ILMerge as it now added into the packages.config and referenced by winsw.csproj. There is nothing more for me to do, am I correct?
Tested it locally on the weekend. Looks good. |
Two new elements introduced zipolderthannumdays and zipdateformat introduced so the older log files can be zipped up automatically at roll time when autoRollAtTime element is used.