Feature/issue25

[MiguelAlho]

This is the initial approach, as reference in #25 . Still some refactoring and testing to do, but could work as a start of a discussion.

[MiguelAlho]

branch name is breaking the build (doesn't accept feature/) so i'll change as soon as possible.

[MiguelAlho]

I've been getting "beat up" by the newline character in the tests - Windows is obviously \r\n but appVeyour seems to only use \n. Since the expected data differs from what is really obtained in that environment, the tests fail

[MiguelAlho]

init:

• git config --global core.autocrlf false

was added to appveyor.yml to avoid test failure (as described in http://help.appveyor.com/discussions/problems/3954-test-failure-cause-by-different-newlines) , since newlines were being atomatically processed by git. Turns out alot of the prevous changes weren't code related (though some were). This took soooooo much time to figue out.. :(

I'll be doing cleanup on this next, and some manual testing before requesting final review and approval

[MiguelAlho]

I've been doing tests with the set of original buffer files we captured that caused the looping error. Up till now, rerunning the log shipping sends things out nicely without looping. There is one behaviour though that isn't totally clear what it should be (and can probably be configurable).

The scenario is the following:

• due to whatever reason (disconnected, etc) over multiple days, many buffer files have acumulated (10 days worth, for instance).
• when reconnecting, the shipper works on the buffer, starting from the file that has been bookmarked (lets start with the first).
• events are shipped, till finishing the file
• once all messages from the file are traversed, the buffer cleanup routine kicks in, and it's unclear what we should expect :

1-(current scenario) The algorithm has a hard coded number of files (2) to consider/maintain, so it begins deleting buffer*json files, oldest ones first, till only two files are left (last two days) and restarts traversing the events and shipping to Loggly. This considers that any thing 2 days or older is uninteresting and can be ignored (except for the first file that we read and could be way older). This makes sense if you consider that only the most recent occurrences are relevant. 2-(possible an/or optional scenario) The algorithm guarantees that all buffer files are traversed, and all events are sent, independent of how old they are.

I believe they are really different concerns and worth considering if they represent valid options or if we should just stick with one.

Also, in the case of the first scenario, should the buffer from the oldest file even be read, if we are to afterwards skip N files till we reach the buffers for the last 2 days?

All considerations are appreciated, here.

[MiguelAlho]

Other than the comments above (both questions in the main thread, and to some of @tamirdresher 's suggestions). I don't have much to add at the moment.

I would probably consider looking into moving to the .NET Standard FileProvider in a future PR (I would like to see the bug that directed these changes corrected as soon as possible). Relative to buffer file removal, it is aligned with what's in Seq's sink, so expectations aren't far from what is there, but can be enhanced.

[nblumhardt]

Hi @MiguelAlho - I think option (2) is actually what I'd expect here; guessing there's some kind of bug at play. I'm unfortunately a bit snowed under right now, battling my way out :-), but won't have time to dig in deeper this week. Happy to review patches (and see if anything is done differently in the Seq sink) if anyone else has a chance to investigate what changes are required 👍

[MiguelAlho]

thanks @nblumhardt . I'll review the algorithm on this one.

I discussed this with some of my colleagues here, and for our purposes, it might be useful to actually only consider the last N days (configurable), because the older data is stale and won't contribute much.

Both scenarios seem legit and useful, so I guess the user should be able to decide on which to use with some sensible default. This will require a constructor change to the sink, or an extra constructor (less breaking).

[MiguelAlho]

another Q: Would it make sense to target the dev branch first (to create alpha packages) and then merge into master?

[nblumhardt]

Generally that's the workflow used by the other Serilog sinks 👍

[MiguelAlho]

Rebased onto Dev

[MiguelAlho]

@tamirdresher @nblumhardt mind if I merge this into dev to gen an alpha package?

[tamirdresher]

:ok_hand:

[MiguelAlho]

With the dev package (5.1.1-dev00127), the loop problem is still visible. I'll be working on a fix.