diff --git a/src/Serilog.Sinks.Loggly/Sinks/Loggly/FileBufferDataProvider.cs b/src/Serilog.Sinks.Loggly/Sinks/Loggly/FileBufferDataProvider.cs index dbc387f..0370e06 100644 --- a/src/Serilog.Sinks.Loggly/Sinks/Loggly/FileBufferDataProvider.cs +++ b/src/Serilog.Sinks.Loggly/Sinks/Loggly/FileBufferDataProvider.cs @@ -24,7 +24,7 @@ interface IBufferDataProvider /// Provides a facade to all File operations, namely bookmark management and /// buffered data readings /// - class FileBufferDataProvider : IBufferDataProvider + internal class FileBufferDataProvider : IBufferDataProvider { #if HRESULTS //for Marshalling error checks @@ -45,7 +45,7 @@ class FileBufferDataProvider : IBufferDataProvider readonly JsonSerializer _serializer = JsonSerializer.Create(); // the following fields control the internal state and position of the queue - FileSetPosition _currentBookmark; + protected FileSetPosition CurrentBookmark { get; set; } FileSetPosition _futureBookmark; IEnumerable _currentBatchOfEventsToProcess; @@ -78,12 +78,12 @@ public IEnumerable GetNextBatchOfEvents() //if we have a bookmark in place, it may be the next position to read from // otherwise try to get a valid one - if (_currentBookmark == null) + if (CurrentBookmark == null) { //read the current bookmark from file, and if invalid, try to create a valid one - _currentBookmark = TryGetValidBookmark(); + CurrentBookmark = TryGetValidBookmark(); - if (!IsValidBookmark(_currentBookmark)) + if (!IsValidBookmark(CurrentBookmark)) return Enumerable.Empty(); } @@ -102,7 +102,7 @@ public void MarkCurrentBatchAsProcessed() _bookmarkProvider.UpdateBookmark(_futureBookmark); //we can move the marker to what's in "future" (next expected position) - _currentBookmark = _futureBookmark; + CurrentBookmark = _futureBookmark; _currentBatchOfEventsToProcess = null; } @@ -124,70 +124,80 @@ public void MoveBookmarkForward() //if we only have two files, move to the next one imediately, unless a locking situation // impeads us from doing so if (fileSet.Length == 2 - && fileSet.First() == _currentBookmark.File - && IsUnlockedAtLength(_currentBookmark.File, _currentBookmark.NextLineStart)) + && fileSet.First() == CurrentBookmark.File + && IsUnlockedAtLength(CurrentBookmark.File, CurrentBookmark.NextLineStart)) { //move to next file - _currentBookmark = new FileSetPosition(0, fileSet[1]); + CurrentBookmark = new FileSetPosition(0, fileSet[1]); //we can also delete the previously read file since we no longer need it _fileSystemAdapter.DeleteFile(fileSet[0]); } if (fileSet.Length > 2) { + //determine where in the fileset on disk is the current bookmark (if any) + //there is no garantee the the current one is the first, so we need to make + //sure we start reading the next one based on the current one + var currentBookMarkedFileInFileSet = CurrentBookmark != null + ? Array.FindIndex(fileSet, f => f == CurrentBookmark.File) + : -1; + //when we have more files, we want to delete older ones, but this depends on the // limit retention policy. If no limit retention policy is in place, the intent is to // send all messages, no matter how old. In this case, we should only delete the current - // file (since we are finished with it) and start at the next one. If we do have some + // file (since we are finished with it) and start the bookmark at the next one. If we do have some // retention policy in place, then delete anything older then the limit and the next // message read should be at the start of the policy limit if (_retainedFileCountLimit.HasValue) { - //move to first file within retention limite - _currentBookmark = _retainedFileCountLimit.Value >= fileSet.Length - ? new FileSetPosition(0, fileSet[1]) - : new FileSetPosition(0, fileSet[fileSet.Length - _retainedFileCountLimit.Value]); - - //delete all the old files - foreach (var oldFile in fileSet.Take(fileSet.Length - _retainedFileCountLimit.Value)) + if (fileSet.Length <= _retainedFileCountLimit.Value) { - _fileSystemAdapter.DeleteFile(oldFile); + //start + var fileIndexToStartAt = Math.Max(0, currentBookMarkedFileInFileSet + 1); + + //if index of current is not the last , use next; otherwise preserve current + CurrentBookmark = fileIndexToStartAt <= fileSet.Length - 1 + ? new FileSetPosition(0, fileSet[fileIndexToStartAt]) + : CurrentBookmark; + + //delete all the old files + DeleteFilesInFileSetUpToIndex(fileSet, fileIndexToStartAt); + } + else + { + //start at next or first in retention count + var fileIndexToStartAt = Math.Max(fileSet.Length - _retainedFileCountLimit.Value, currentBookMarkedFileInFileSet +1); + + CurrentBookmark =new FileSetPosition(0, fileSet[fileIndexToStartAt]); + + //delete all the old files + DeleteFilesInFileSetUpToIndex(fileSet, fileIndexToStartAt); } } else { - //move to the NEXT file and delete the current one - //there is no garantee the the current one is the first, so we need to make - //sure we start reading the next one based on the current on - var currentBookMarkedFileInFileSet = _currentBookmark != null - ? Array.FindIndex(fileSet, f => f == _currentBookmark.File) - : -1; - // not sure this can occur, but if for some reason the file is no longer in the list // we should start from the beginning, maybe; being a bit defensive here due to // https://github.com/serilog/serilog-sinks-loggly/issues/25 if (currentBookMarkedFileInFileSet == -1) { //if not in file set, use first in set (or none) - _currentBookmark = fileSet.Length > 0 + CurrentBookmark = fileSet.Length > 0 ? new FileSetPosition(0, fileSet[0]) : null; } else { //if index of current is not the last , use next; otherwise preserve current - _currentBookmark = currentBookMarkedFileInFileSet <= fileSet.Length - 2 + CurrentBookmark = currentBookMarkedFileInFileSet <= fileSet.Length - 2 ? new FileSetPosition(0, fileSet[currentBookMarkedFileInFileSet + 1]) - : _currentBookmark; + : CurrentBookmark; } - + //also clear all the previous files in the set to avoid problems (and because //they should no longer be considered); If no previous exists (index is -1); //keep existing; also do not reomve last file as it may be written to / locked - for (int i = 0; i <= currentBookMarkedFileInFileSet && i < fileSet.Length-1; i++) - { - _fileSystemAdapter.DeleteFile(fileSet[i]); - } + DeleteFilesInFileSetUpToIndex(fileSet, currentBookMarkedFileInFileSet + 1); } } } @@ -203,8 +213,16 @@ public void MoveBookmarkForward() //it's possible that no bookmark exists, especially if no valid messages have forced a // durable log file to be created. In this case, the bookmark file will be empty and // on disk - if(_currentBookmark != null) - _bookmarkProvider.UpdateBookmark(_currentBookmark); + if(CurrentBookmark != null) + _bookmarkProvider.UpdateBookmark(CurrentBookmark); + } + } + + void DeleteFilesInFileSetUpToIndex(string[] fileSet, int fileIndexToDeleteUpTo) + { + for (int i = 0; i < fileIndexToDeleteUpTo && i < fileSet.Length - 1; i++) + { + _fileSystemAdapter.DeleteFile(fileSet[i]); } } @@ -249,9 +267,9 @@ void RefreshCurrentListOfEvents() { var events = new List(); var count = 0; - var positionTracker = _currentBookmark.NextLineStart; + var positionTracker = CurrentBookmark.NextLineStart; - using (var currentBufferStream = _fileSystemAdapter.Open(_currentBookmark.File, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) + using (var currentBufferStream = _fileSystemAdapter.Open(CurrentBookmark.File, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) { while (count < _batchPostingLimit && TryReadLine(currentBufferStream, ref positionTracker, out string readLine)) { @@ -295,7 +313,7 @@ void RefreshCurrentListOfEvents() } } - _futureBookmark = new FileSetPosition(positionTracker, _currentBookmark.File); + _futureBookmark = new FileSetPosition(positionTracker, CurrentBookmark.File); _currentBatchOfEventsToProcess = events; } diff --git a/test/Serilog.Sinks.Loggly.Tests/Sinks/Loggly/FileBufferDataProviderTests.cs b/test/Serilog.Sinks.Loggly.Tests/Sinks/Loggly/FileBufferDataProviderTests.cs index b599aaa..a100e9b 100644 --- a/test/Serilog.Sinks.Loggly.Tests/Sinks/Loggly/FileBufferDataProviderTests.cs +++ b/test/Serilog.Sinks.Loggly.Tests/Sinks/Loggly/FileBufferDataProviderTests.cs @@ -11,6 +11,40 @@ namespace Serilog.Sinks.Loggly.Tests.Sinks.Loggly { + /// + /// This class allowws setting the initial state fo the provider in the tests, especially to + /// controll the current bookmark position, as that influences the way logic happens + /// + internal class FileBufferDataProviderThatAllowscurrentBookmarkToBeSet : FileBufferDataProvider + { + public FileBufferDataProviderThatAllowscurrentBookmarkToBeSet( + string baseBufferFileName, + IFileSystemAdapter fileSystemAdapter, + IBookmarkProvider bookmarkProvider, + Encoding encoding, + int batchPostingLimit, + long? eventBodyLimitBytes, + int? retainedFileCountLimit) + : base(baseBufferFileName, + fileSystemAdapter, + bookmarkProvider, + encoding, + batchPostingLimit, + eventBodyLimitBytes, + retainedFileCountLimit) + { + } + + /// + /// This helps set the inital bookmark in tests + /// + /// + internal void DefineCurrentBookmark(long nextLineStartToUse, string file) + { + CurrentBookmark = new FileSetPosition(nextLineStartToUse, file); + } + } + public class FileBufferDataProviderTests { static readonly string ResourceNamespace = $"Serilog.Sinks.Loggly.Tests.Sinks.Loggly"; @@ -601,7 +635,7 @@ public PreviousBookmarkStartedOnFirstFile() IFileSystemAdapter fsAdapter = CreateFileSystemAdapter(); - var provider = new FileBufferDataProvider( + var provider = new FileBufferDataProviderThatAllowscurrentBookmarkToBeSet( BaseBufferFileName, fsAdapter, bookmarkProvider, @@ -611,9 +645,7 @@ public PreviousBookmarkStartedOnFirstFile() null); //force the current Bookmark to be current file: - //This unfortunately depends on the "NoPreviousBookmark" test working / passing - //some method of setting the current in the provider could also help setup - provider.MoveBookmarkForward(); + provider.DefineCurrentBookmark(123, @"c:\a\file001.json"); //excercise the SUT provider.MoveBookmarkForward(); @@ -666,7 +698,7 @@ public PreviousBookmarkStartedOnSecondFile() IFileSystemAdapter fsAdapter = CreateFileSystemAdapter(); - var provider = new FileBufferDataProvider( + var provider = new FileBufferDataProviderThatAllowscurrentBookmarkToBeSet( BaseBufferFileName, fsAdapter, bookmarkProvider, @@ -676,12 +708,7 @@ public PreviousBookmarkStartedOnSecondFile() null); //force the current Bookmark to be current file: - //This unfortunately depends on the "NoPreviousBookmark" test working / passing - //some method of setting the current in the provider could also help setup - provider.MoveBookmarkForward(); - provider.MoveBookmarkForward(); //we can ignore the deletes here, for now - //but we need to reset the adapter in use - _deletedFiles.Clear(); + provider.DefineCurrentBookmark(123, @"c:\a\file002.json"); //excercise the SUT provider.MoveBookmarkForward(); @@ -736,7 +763,7 @@ public PreviousBookmarkStartedOnLastFile() IFileSystemAdapter fsAdapter = CreateFileSystemAdapter(); - var provider = new FileBufferDataProvider( + var provider = new FileBufferDataProviderThatAllowscurrentBookmarkToBeSet( BaseBufferFileName, fsAdapter, bookmarkProvider, @@ -746,13 +773,7 @@ public PreviousBookmarkStartedOnLastFile() null); //force the current Bookmark to be current file: - //This unfortunately depends on the "NoPreviousBookmark" test working / passing - //some method of setting the current in the provider could also help setup - provider.MoveBookmarkForward(); - provider.MoveBookmarkForward(); - provider.MoveBookmarkForward(); //we can ignore the deletes here, for now - //but we need to reset the adapter in use - _deletedFiles.Clear(); + provider.DefineCurrentBookmark(123, @"c:\a\file003.json"); //excercise the SUT provider.MoveBookmarkForward(); @@ -776,7 +797,7 @@ IFileSystemAdapter CreateFileSystemAdapter() } [Fact] - public void BookmarkShouldBeAtEndOfLastFile() => Assert.Equal(0, _sut.NextLineStart); //file is empty so zero is the end in this setup + public void BookmarkShouldBeAtEndOfLastFile() => Assert.Equal(123, _sut.NextLineStart); //preserves current [Fact] public void BookmarkShouldStillPointToLastFile() => Assert.Equal(@"c:\a\file003.json", _sut.File); @@ -855,6 +876,71 @@ public void SingleFileShouldHaveBeenDeleted() => Assert.Equal(2, _deletedFiles.Count); } + public class RetentionLimitLessThenNumberOfBufferFilesAndBookMarkOnSecondFile + { + const int Limit = 10; + readonly List _deletedFiles = new List(); + FileSetPosition _sut; + + public RetentionLimitLessThenNumberOfBufferFilesAndBookMarkOnSecondFile() + { + var bookmarkProvider = Substitute.For(); + bookmarkProvider + .When(x => x.UpdateBookmark(Arg.Any())) + .Do(x => _sut = x.ArgAt(0)); + + IFileSystemAdapter fsAdapter = CreateFileSystemAdapter(); + + var provider = new FileBufferDataProviderThatAllowscurrentBookmarkToBeSet( + BaseBufferFileName, + fsAdapter, + bookmarkProvider, + Utf8Encoder, + BatchLimit, + EventSizeLimit, + Limit); + + //force the current Bookmark to be second file: + provider.DefineCurrentBookmark(123, @"c:\a\file002.json"); + + provider.MoveBookmarkForward(); + } + + IFileSystemAdapter CreateFileSystemAdapter() + { + var fileSystemAdapter = Substitute.For(); + + //get files should return the single buffer file path in this scenario + fileSystemAdapter.GetFiles(Arg.Any(), Arg.Any()) + .Returns(new[] + { + @"c:\a\file001.json", + @"c:\a\file002.json", + @"c:\a\file003.json", + @"c:\a\file004.json" + }); + + //files exist + fileSystemAdapter.Exists(Arg.Any()).Returns(true); + fileSystemAdapter + .When(x => x.DeleteFile(Arg.Any())) + .Do(x => _deletedFiles.Add(x.ArgAt(0))); + + return fileSystemAdapter; + } + + [Fact] + public void BookmarkShouldBeAtStartOfNextFile() => Assert.Equal(0, _sut.NextLineStart); + + [Fact] + public void BookmarkShouldBeAtNextFile() => Assert.Equal(@"c:\a\file003.json", _sut.File); + + [Theory] + [InlineData(@"c:\a\file001.json", 0)] + [InlineData(@"c:\a\file002.json", 1)] + public void PreviousFilesShouldHaveBeenDeleted(string expectedDeletedFile, int expectedDeletedIndex) => Assert.Equal(expectedDeletedFile, _deletedFiles[expectedDeletedIndex]); + } + public class RetentionLimitMoreThenLimitNumberOfBufferFiles { const int Limit = 10; @@ -903,12 +989,84 @@ IFileSystemAdapter CreateFileSystemAdapter() public void BookmarkShouldBeAtStartOfNextFile() => Assert.Equal(0, _sut.NextLineStart); [Fact] - public void BookmarkShouldBeAtNextFile() => Assert.Equal(@"c:\a\file002.json", _sut.File); + public void BookmarkShouldBeAtNextFile() => Assert.Equal(@"c:\a\file001.json", _sut.File); [Fact] public void NoFilesShouldHaveBeenDeleted() => Assert.Empty(_deletedFiles); } + public class RetentionLimitMoreThenNumberOfBufferFilesAndBookMarkOnSecondFile + { + const int Limit = 4; + readonly List _deletedFiles = new List(); + FileSetPosition _sut; + + public RetentionLimitMoreThenNumberOfBufferFilesAndBookMarkOnSecondFile() + { + var bookmarkProvider = Substitute.For(); + bookmarkProvider + .When(x => x.UpdateBookmark(Arg.Any())) + .Do(x => _sut = x.ArgAt(0)); + + IFileSystemAdapter fsAdapter = CreateFileSystemAdapter(); + + var provider = new FileBufferDataProviderThatAllowscurrentBookmarkToBeSet( + BaseBufferFileName, + fsAdapter, + bookmarkProvider, + Utf8Encoder, + BatchLimit, + EventSizeLimit, + Limit); + + //force the current Bookmark to be current file: + provider.DefineCurrentBookmark(123, @"c:\a\file002.json"); + + provider.MoveBookmarkForward(); + } + + IFileSystemAdapter CreateFileSystemAdapter() + { + var fileSystemAdapter = Substitute.For(); + + //get files should return the single buffer file path in this scenario + fileSystemAdapter.GetFiles(Arg.Any(), Arg.Any()) + .Returns(new[] + { + @"c:\a\file001.json", + @"c:\a\file002.json", + @"c:\a\file003.json", + @"c:\a\file004.json", + @"c:\a\file005.json", //with a limit of 4, this should be the first to be read after moving forward + @"c:\a\file006.json", + @"c:\a\file007.json", + @"c:\a\file008.json" + }); + + //files exist + fileSystemAdapter.Exists(Arg.Any()).Returns(true); + fileSystemAdapter + .When(x => x.DeleteFile(Arg.Any())) + .Do(x => _deletedFiles.Add(x.ArgAt(0))); + + return fileSystemAdapter; + } + + [Fact] + public void BookmarkShouldBeAtStartOfNextFile() => Assert.Equal(0, _sut.NextLineStart); + + [Fact] + public void BookmarkShouldBeAtNextFile() => Assert.Equal(@"c:\a\file005.json", _sut.File); + + [Theory] + [InlineData(@"c:\a\file001.json", 0)] + [InlineData(@"c:\a\file002.json", 1)] + [InlineData(@"c:\a\file003.json", 2)] + [InlineData(@"c:\a\file004.json", 3)] + public void PreviousFilesShouldHaveBeenDeleted(string expectedDeletedFile, int expectedDeletedIndex) => Assert.Equal(expectedDeletedFile, _deletedFiles[expectedDeletedIndex]); + } + + }