datalust / serilog-sinks-seq

A Serilog sink that writes events to the Seq structured log server
https://datalust.co/seq
Apache License 2.0
225 stars 50 forks source link

Missing reference to System.Net.Http when updating to v6 #208

Closed rocklan closed 7 months ago

rocklan commented 7 months ago

Not a bug but possible documentation request.

Now that the extension method of Seq() inside SeqLoggerConfigurationExtensions.cs now has an optional parameter HttpMessageHandler? messageHandler = null, this means you must add a reference to System.Net.Http, even if you're not passing a messageHandler. A bit annoying.

I appreciate that v6.0.0 means breaking changes but I couldn't find anywhere where these breaking changes are documented. Maybe worth mentioning somewhere?

(Of course an alternative could be to not use optional params and to overload it instead but I appreciate that might not be something you want to do.)

nblumhardt commented 7 months ago

Thanks for the heads-up, @rocklan.

Which version are you updating from? Just checking v5.0.0 in this tag that parameter is already there (it wasn't a 6.0.0 addition), I'm wondering if we've made some other subtle change (TFM changes perhaps?) that has had an unintended side-effect for you... :thinking:

rocklan commented 7 months ago

That's weird! Upgraded from 5.2.2 to 6.0.0. And you're correct, in the version of our code that uses 5.2.2 it does have that as an optional param there but it doesn't need the reference to System.Net.Http . The actual error is:

error CS0012: The type 'HttpMessageHandler' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Net.Http, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

I just realised that the app itself (asp web framework 4.7.2 app) doesn't directly reference the Serilog.Sinks.Seq package but instead references a project that has the reference to the Serilog.Sinks.Seq package. Dunno if that helps or makes a difference.

rocklan commented 7 months ago

Ok I've managed to reproduce it.

  1. Create dotnet framework 4.7.2 console app
  2. Remove reference to System.Net.Http
  3. Add code in main method: _ = new LoggerConfiguration().WriteTo.Seq("http://example.com");
  4. Create new dotnet standard 2.0 class library project, make sure to use package reference's, not packages.config file
  5. Add Serilog.Sinks.Seq version 5.2.3 to class library
  6. Add code to class library: public void Alert() => Serilog.Log.Warning("Warning");
  7. Compile, note everything is all good
  8. Upgrade nuget package Serilog.Sinks.Seq to version 6.0.0
  9. Recompile, it should fail with message error CS0012: The type 'HttpMessageHandler' is defined in an assembly that is not referenced.

Pretty obscure edge case so completely happy if you just want to close the issue :)

nblumhardt commented 7 months ago

Wowsers! Thanks for tracking it down. I don't think we'd pursue changing the method signature at this stage, but hopefully this ticket will at least provide some context for others who hit this issue :+1: