search

serilog / serilog-sinks-email

A Serilog sink that writes events to SMTP email
Apache License 2.0
70 stars 68 forks source link

Re-implement email sink using IBatchedLogEventSink without breaking anything #81

Closed ericvb closed 4 years ago

ericvb commented 4 years ago

Reimplementation of the email sink to use the interface IBatchedLogEventSink and IDisposable without breaking changes for the user upgrading his nuget package. Like before, it uses the PeriodicBatchingSink implementation. The new option EagerlyEmitFirstEvent from PeriodicBatchingSink is by default false which resolves the annoying two mails if several logging events.

ericvb commented 4 years ago

@nblumhardt Like suggested, a non breaking PR

ericvb commented 4 years ago

Failing check: due to change of SDK to .net core 3? I've only .net core 3 on my machine, so I had to change. The other settings like .netstandard I didn't change, but should we not update?

bcallaghan-et commented 4 years ago

With this implementation, how would users choose a different batching algorithm? It looks like all Email() methods still use PeriodicBatchingSink.

nblumhardt commented 4 years ago

@ericvb great! The breakage will be because the test project is targeting netcoreapp1.0 - this should be updated now so that the tests run on netcoreapp3.1 and a recent .NET Framework version (net47 is probably the least awkward).

nblumhardt commented 4 years ago

@bcallaghan-et thanks for the note. Yes, that's the intent - at the moment, the long-running request has been to avoid the "eager first event" behavior, we don't have a ticket discussing the need for different batching algorithms so that's yet to be considered separately, but would be a great discussion to have, especially how we might do it in a broadly-compatible/non-breaking way :+1:

ericvb commented 4 years ago

@nblumhardt, is there still something I can/must do?

nblumhardt commented 4 years ago

Hi @ericvb! The build's still breaking - updating the test project should fix it. It's usual that PRs aren't merged until checks pass - let me know if you need a hand 👍

ericvb commented 4 years ago

@nblumhardt The window build is passing now together with the tests, but I added also the ubuntu build by analogy with the main serilog build. This ubuntu build is complaining about missing packages for running the tests

A total of 1 test files matched the specified pattern.
Testhost process exited with error: It was not possible to find any compatible framework version
The framework 'Microsoft.NETCore.App', version '2.2.0' was not found.
  - The following frameworks were found:
      3.1.4 at [/home/appveyor/.dotnetcli/shared/Microsoft.NETCore.App]
You can resolve the problem by installing the specified framework and/or SDK.
The specified framework can be found at:
  - https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=2.2.0&arch=x64&rid=ubuntu.18.04-x64
. Please check the diagnostic logs for more information.
Test Run Aborted.
Command exited with code 1

Can these missing frameworks be installed on the build agent? Strange that it doesn't work, because the test projects in the main serilog repository are also targetting netcoreapp2.2

ericvb commented 3 years ago

@nblumhardt Do you have an idea when a new nuget release of the email sink is planned with these latests changes?

nblumhardt commented 3 years ago

@ericvb thanks for the nudge, building #86 now