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

Added Support for the log appender roll-by-size-time to zip older files #259

Merged
merged 21 commits into from Jun 19, 2018

Conversation

DHirani
Copy link
Contributor

@DHirani DHirani commented Jan 18, 2018

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.

DHirani and others added 16 commits August 29, 2017 14:16
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.
Copy link
Member

@oleg-nenashev oleg-nenashev left a 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>
Copy link
Member

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

Copy link
Contributor Author

@DHirani DHirani Feb 5, 2018

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 +


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)
Copy link
Member

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
@DHirani
Copy link
Contributor Author

DHirani commented Feb 5, 2018

Added documentation for the two new zip elements

@DHirani
Copy link
Contributor Author

DHirani commented Feb 12, 2018

Please let me know if you are happy to merge this to master or if there are still any other outstanding issues

@oleg-nenashev
Copy link
Member

Yes, I still need to review it. Sorry for the delays

Copy link
Member

@oleg-nenashev oleg-nenashev left a 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

@@ -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>
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -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; }
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -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.
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@oleg-nenashev oleg-nenashev left a 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

@oleg-nenashev oleg-nenashev self-assigned this Feb 26, 2018
@DHirani
Copy link
Contributor Author

DHirani commented Mar 27, 2018

Are you good to merge this into master?

@oleg-nenashev
Copy link
Member

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

@DHirani
Copy link
Contributor Author

DHirani commented Mar 27, 2018

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" />
Copy link

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

Copy link
Contributor Author

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.

Copy link

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

Copy link
Member

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

Copy link
Contributor Author

@DHirani DHirani Apr 26, 2018

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?

@oleg-nenashev
Copy link
Member

Tested it locally on the weekend. Looks good.
Sorry for being non-responsive in the repo, I will try to catch-up

@oleg-nenashev oleg-nenashev merged commit 9014f38 into winsw:master Jun 19, 2018
@nxtn nxtn added this to the 2.2.0 milestone Mar 25, 2020
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.

None yet

4 participants