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

FileArchiveModeFactory - Extracted from FileTarget #1993

Merged
merged 5 commits into from
Jun 23, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Feb 27, 2017

Attempt to resolve the issues #1670 and #1681, which is mostly caused by not understanding how to configure the NLog Archive Logic (And also major refactoring of the archive logic in FileTarget).

I guess more effort could be applied to FileArchiveModeDynamicSequence.GenerateFileNameMask() to better recognize any date-format-output.

Enabled the unit-tests from #1788, but had to modify them, so they would work with Mono on Linux (Changed hardcoded windows-backslash to Path.Combine). The tests also suffered from confusing debug- and trace-level (Logged to debug-level and didn't expect it to reach trace-level).

!! Breaking change !!
If using ArchiveNumberingMode.Sequence (default) and not having configured an ArchiveFileName (default) and have enabled archiving (MaxArchiveFiles > 0). Then it will now enable dynamic-sequence mode, instead of strict-sequence-mode.

Dynamic-sequence-mode will use the same archive-filename-format (as strict-sequence-mode), but its filemask-wildcard for doing archive-cleanup is a little more greedy as it replaces the largest range of digits (must be more than one) with a wildcard (and not just the sequence-no).

This improves the out-of-box archive-cleanup, when the FileTarget.FileName-Layout includes date-renderer.

New behavior for dynamice sequence-mode
New behavior when ArchiveFileName does not contain sequence-no-pattern (#}:

  • More adaptive filemask-wildcard used when doing archive-cleanup. Before it was just all files with the same file-extension. Now also using the actual LogEvent (Instead of Null-LogEvent), so the filemask for cleanup is a lot better (Doesn't need an isolated archive directory).
  • Now delays archive cleanup until first write-operation occurs, instead of FileTarget.Initialize(). This allows archive-cleanup to work properly as it will use the actual LogEvent for rendering archive filemask for cleanup (Instead of Null-LogEvent).
    • This also removes extra overhead at startup where all FileTarget would scan their archive-folders.
    • This generates extra overhead at first new file write, where archive-cleanup occurs. But the archive-cleanup is skipped if new file write is caused by just having executed a file-archive-operation.

This improves the out-of-box archive-cleanup, when the ArchiveFileName-Layout includes date-renderer.

Other changes:

  • Now delays the monitoring of the archive-folder until the first-write. And the filemask monitored is generated from the actual logevent instead of Null-LogEvent (More correct).
  • ArchiveNumberingMode.Date now skips an unnecessary archive-cleanup when moving to new file after just having performed archive of the previous file (with cleanup).
  • Attempt to handle that logging-folder and archive-folder is the same folder. Crude Path.GetDirectoryName-comparison that probably can be cheated.
  • Attempt to handle file-creation-date-issue on Linux for archiving. This should improve how NLog performs archive-cleanup of dated-archives.
  • Attempt to handle non-standard-path-usage, that is different from those returned by the standard filesystem.

This change is Reviewable

@snakefoot snakefoot force-pushed the FileArchiveWildcardMask branch 2 times, most recently from 95f2de4 to a0def89 Compare February 27, 2017 00:59
@304NotModified
Copy link
Member

It's the limited silverlight api.

Targets\FileArchiveStyleFactory.cs(583,81): error CS1061: 'System.IO.FileInfo' does not contain a definition for 'CreationTimeUtc' and no extension method 'CreationTimeUtc' accepting a first argument of type 'System.IO.FileInfo' could be found (are you missing a using directive or an assembly reference?)

There's maybe a helper for this already.

@snakefoot snakefoot force-pushed the FileArchiveWildcardMask branch 3 times, most recently from eb86191 to cf074c5 Compare February 27, 2017 20:56
@codecov-io
Copy link

codecov-io commented Feb 27, 2017

Codecov Report

Merging #1993 into master will increase coverage by <1%.
The diff coverage is 89%.

@@           Coverage Diff           @@
##           master   #1993    +/-   ##
=======================================
+ Coverage      81%     82%   +<1%     
=======================================
  Files         291     299     +8     
  Lines       20124   20305   +181     
  Branches     2392    2433    +41     
=======================================
+ Hits        16356   16593   +237     
+ Misses       3152    3107    -45     
+ Partials      616     605    -11

@snakefoot snakefoot force-pushed the FileArchiveWildcardMask branch 10 times, most recently from b0dbb8d to 45b8254 Compare February 28, 2017 20:12
@snakefoot
Copy link
Contributor Author

@304NotModified Thanks that fixed the appveyor-build. The travis-build required some changes to the unit-tests. But now things are looking green. And I have implemented a fix for my worries about performing too many archive cleanups.

@304NotModified 304NotModified added bug Bug report / Bug fix file-target labels Feb 28, 2017
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

Cool! I afraid of the merge issues with the CoreCLR branch, but I guess it's only an issue with the FileTarget file.

Please move all the classes to their own file :)
(and maybe own folder)

It the UTC change really needed? I prefer tests with localtime as that would be a better test IMO

src/NLog/Internal/FileInfoExt.cs Outdated Show resolved Hide resolved
src/NLog/Targets/FileArchiveStyleFactory.cs Outdated Show resolved Hide resolved
src/NLog/Targets/FileArchiveStyleFactory.cs Outdated Show resolved Hide resolved
src/NLog/Targets/FileArchiveStyleFactory.cs Outdated Show resolved Hide resolved
src/NLog/Targets/FileArchiveStyleFactory.cs Outdated Show resolved Hide resolved
src/NLog/Targets/FileTarget.cs Outdated Show resolved Hide resolved
tests/NLog.UnitTests/Targets/FileTargetTests.cs Outdated Show resolved Hide resolved
@304NotModified 304NotModified added this to the 4.5 milestone Feb 28, 2017
@snakefoot snakefoot changed the title FileArchiveStyleFactory - Extracted from FileTarget FileArchiveModeFactory - Extracted from FileTarget Feb 28, 2017
@snakefoot
Copy link
Contributor Author

Please move all the classes to their own file :)

Have now moved the classes into their own individual file.

@snakefoot snakefoot force-pushed the FileArchiveWildcardMask branch 8 times, most recently from baa107d to 095f84c Compare May 8, 2017 20:20
@snakefoot snakefoot force-pushed the FileArchiveWildcardMask branch 4 times, most recently from 2330666 to 4a89a1a Compare May 30, 2017 22:53
@304NotModified
Copy link
Member

fixes #1670, fixes #1681, fixes #1748

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix enhancement Improvement on existing feature file-target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants