FileLifecycleHooks

[cocowalla]

Enabled wrapping FileSink and RollingFileSink's output stream in another stream, such as a GZipStream. This forms part of a solution to #48

[nblumhardt]

🎉 :-)

Added some comments/thoughts; cheers!

[nblumhardt]

Since the parameter lists to methods of are hot real-estate :-) we might avoid some future churn by naming this FileLifecycleHooks or something along those lines? It would need some thought put into the naming of Wrap(), since the convention would ideally be consistent with future methods we might add, like OnFileOpened(), OnFileClosed(). (Maybe we could pre-empt some of that? Wrap() could almost be the OnFileOpened() method, give or take a parameter or two?)

[cocowalla]

I'm with you on renaming StreamWrapper to avoid ctor hell as more hooks are added.

At first I wasn't too sure about OnFileOpened, since as-is the Wrap method is quite different to a 'standard' event handler. But after thinking a bit, you might have a point. Perhaps, as I guess you are alluding to, we could add a string path parameter, which would also have the bonus effect of making it more "general purpose". It would become:

public abstract Stream OnFileOpened(Stream underlyingStream, string path);

The idea would be that if you're not interested in changing the stream, you just return underlyingStream (this would be in the xml docs)

[nblumhardt]

The added parameter here will be a binary breaking change, even though calling code will recompile successfully.

There are a couple of options - for FileSink(), we should probably add a constructor overload to accept the new parameter, and have the old calls redirect to it.

At the level of these extension methods, it's possible to add an overload also, but the old methods have to be carefully un-defaulted and [Obsolete]d so that recompilation targets the new method (thereby giving us some future room to remove the old code). There are some examples of this in this file - see the first two methods in this class. Probably the best option for now, though in some future 5.0 we might start cleaning this up.

[cocowalla]

Regarding FileSink, I just wanted to be clear that this is what you mean:

// Original ctor redirects to new one
public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding = null, bool buffered = false)
	: this(path, textFormatter, fileSizeLimitBytes, encoding, buffered, null)
{
  // Intentionally emtpy
}

// New ctor
public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding = null, bool buffered = false,
	FileLifecycleHooks hooks = null)
{
  // Code here
}

After doing that, existing calls such as new FileSink(path, new JsonFormatter(), null) result in a compiler error due to The call is ambiguous. I can of course change all the calls in the library so they use the new ctor, but I guess this ctor ambiguity will break things for users?

The extension methods are rather fiddly to get right :nerd: but I'll see what I can do!

[nblumhardt]

Usually in the past we've resolved the ambiguity by removing the = x default parameter values from the older version of the constructor. Alternatively, the new parameter can be positioned before the first defaulted one and not given a default value.

[cocowalla]

If adding a new defaulted parameter breaks binary compatibility, I would have though that removing defaults would have too?

2nd option would defo fix it though

[nblumhardt]

From the loader's perspective, only the number/kind/order of parameters is important, it doesn't see the default values at all. So adding a parameter, even if it has a default, changes the signature. Adding/removing/modifying default values doesn't break binary compatibility because they're not considered part of the signature. Fun and games :)

[cocowalla]

Hmm, OK, interesting 😄

So let's say that a user calls this today:

new FileSink(path, textFormatter, 12345);

And then we remove the defaults from the remaining encoding and buffered parameters, and do this:

// Original ctor - defaults removed, and now redirects to new ctor
public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding, bool buffered)
	: this(path, textFormatter, fileSizeLimitBytes, encoding, buffered, null)
{
  // Intentionally emtpy
}

// New ctor
public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding = null, bool buffered = false,
	FileLifecycleHooks hooks = null)
{
  // Code here
}

In this scenario, will the old code continue to work?

I feel there must be some tooling that can check if binary compatibility is maintained - do you use anything like that?

[nblumhardt]

Yes, the old code will be binary compatible here, because the compiler inserts the default values directly into the call site.

So far I haven't found/used any recent tooling for this, but it could be out there. Mostly just painstaking work :-)

[tsimbalar]

https://www.fuget.org/ can be used to compare the API changes between 2 published versions of the same Nuget package. Quite helpful before remove the "pre-release" suffix :)

[nblumhardt]

Hi @cocowalla - hope all's going well! I don't think we've synced up since your last round of changes was pushed - is this ready for another look? Cheers!

[cocowalla]

@nblumhardt it's a bit of a chore going through the overloads, so I confess I've been putting it off 😄

I'd still really like this in Serilog tho, and I believe it should indeed enable #95 too - I'll try to look at it this week!

[nblumhardt]

If you'd like, I can try my hand at sending the last few tweaks through to your branch? I should have some time in the next few days :-D

[cocowalla]

I didn't get time this week, so that would be great :)

If you don't get the time though, I should have some later next week (hopefully!)

[tsimbalar]

(Oops, accidentally pressed the Approve button .... not sure how I can undo this)

[nblumhardt]

Jumping into this now! :-)

[nblumhardt]

I am not sure why encoding was not propagated, here and a few lines above - I think it's a bug (though it's hard to argue with an enforced UTF-8 no-BOM default :-) )

[nblumhardt]

One of the target use cases is writing headers e.g. to CSV/W3C log files, so the name Wrap() might be too specific. I've tried OnOpened() here, with the possibility of adding OnOpening(path) and possibly an OnClosing(stream)/OnClosed(path) pair later on.

If we make this virtual rather than abstract, we won't introduce inconsistencies when future hooks are added (since these must be virtual to avoid breakage).

All sounds sane?

[cocowalla]

Actually, we discussed this a while back in this thread, and it looks like I just forgot to rename this!

virtual reasoning also makes sense.

[cocowalla]

Back when we started discussing this there was the question of streaming vs 'static' extensibility points and this PR solves the streaming part.

Later bolting on OnOpening(path), OnClosing(stream)/OnClosed(path) would solve the 'static' part, enabling scenarios such as cryptographically signing files before they are closed, shipping closed files to WORM media, perform (non-streaming) compression and/or encryption etc 👍

[nblumhardt]

Ha! I actually like the name we were using in the earlier comments a little better, on reflection. I'll rename to OnFileOpened() and merge away! :-)

[nblumhardt]

@merbla I'm following your serilog/serilog example to switch from Travis to AppVeyor for Linux builds. Anything special I need to do to get rid of the Travis integration? I removed the webhook but it's back - guessing the Travis project needs to be deleted?

[nblumhardt]

Okay, I think this is ready to merge.

There are (as you'd expect) a few tricky design issues.

• It would be nice to separate wrapping the stream from initializing it with content, i.e. so that a TextWriter could be passed to a hook rather than the Stream + Encoding, but without access to the underlying stream, code that writes headers can't tell whether or not the stream has data yet.
• Composing hooks is tricky - but this can be punted as the code performing the composition (of say, GZip + Header writer) can itself be a single lifecycle hook that dispatches calls to one or more other hooks.

The AppVeyor build is green; the Travis build is no longer needed.

Any thoughts/comments @cocowalla @tsimbalar , or should we send this through to dev? 😄

[cocowalla]

@nblumhardt looks good to me, thanks!

[nblumhardt]

Thanks @cocowalla 👍

[cocowalla]

@nblumhardt to get things going, I've just published Serilog.Sinks.File.GZip as a NuGet package 👍 Looking forward to the release of 4.1.0!

[nblumhardt]

@cocowalla that's great! I've put the word out on Twitter, keen to see how it all goes 😊

[emiliovmq]

@cocowalla is there any support at Serilog.Sinks.File.Header for DI. Let me explain myself. We have a requirement that dictates that for every log we generate we should add to the beginning of the file the version information of our APIs. As you might expect, I'm reading the version information from the assemblies attributes using reflection (which is a very expensive process). To not incur in this very costly process (for every file event and for my internal requests), I only read the version info once (the first time it is requested), registering it with the DI container and resolve it for every subsequent request or event. l would like to inject the version info into the header writer. Is there any simple workaround for this?

[emiliovmq]

I forgot to thank you and @nblumhardt for this great feature added to Serilog. Thanks

[cocowalla]

Just a note to say there is now another FileLifecycleHooks-based plugin available, serilog-sinks-file-archive, which works with rolling log files, archiving completed log files before they are deleted by Serilog's retention mechanism.