Should CoreLogging be constrained to netcoreapp?

[jrsearles]

I'm not sure why this constraint is needed. You can use the Microsoft.Extensions.Logging and abstractions in code that targets dotnetstandard and full framework. I'm not sure about net452, but we have apps that target472 and use these abstractions.

[eli-darkly]

Well, our thought process was basically:

1. In .NET Framework in particularly, dependency version resolution can be quite a hassle. Therefore it's desirable for our main SDK libraries not to bring in any dependencies that they don't actually require, especially if those have transitive dependencies. An API that's actually built into the runtime platform (as M.E.L is built into .NET Core) can't have that issue.
2. When the LaunchDarkly .NET SDK moved away from M.E.L to Common.Logging a couple of years ago, the reason was specifically that .NET Framework developers were complaining that they didn't want to bring in any .NET Core dependencies. The issue there is not that the M.E.L package itself had a lot of dependencies, but that actually using it to do logging (i.e. having a logging provider implementation, rather than just the facade) required packages that would bring in a fair amount of .NET Core support code.

I think probably number 2 is no longer relevant at this point. In the context of LaunchDarkly.Logging, we would not be requiring .NET Framework developers to have a M.E.L provider implementation; we would only be referencing its API package. That's not as bad... but, it still means we would be importing a specific version of that assembly and of its transitive dependencies, some of which are very likely to be referenced from other things, which in .NET Framework means you're likely to need more binding dependencies.

Still, it's possible that this would be worth doing for the .NET Framework 4.6.1 target. I don't think it's possible in 4.5.2— the M.E.L package simply isn't supported for that target.

As for .NET Standard 2.0, I'm reluctant. We are trying to keep the dependencies even more stripped-down there, because that target can be used for mobile platforms like Xamarin where a dependency like M.E.L is not really useful and increases both the application size and the number of potentially incompatible transitive dependencies. And code that's targeted to .NET Standard is likely to be library code, or portable support code; such code is less likely to be responsible for things like configuring logging, and more likely to be receiving a reference to an already-configured logger to do things with. The host application, which is targeted to a real runtime platform, would be more likely to be configuring logging.

[eli-darkly]

Of course, it would still be possible to do exactly what Logs.CoreLogging is doing from within application code on any platform where Microsoft.Extensions.Logging is available. The implementation in NetCoreLogging.cs is fairly trivial— it's only provided as a convenience.

[jrsearles]

Yeah, i was going to suggest perhaps adding a target for net461. I guess ideally it'd be a separate library specific to the MS abstractions and then you wouldn't have the same concerns about dependencies. It seems that most libraries these days have support for the MS abstractions.

I do see that the implementation is straightforward, so i don't mind pulling that in directly to our code if you're not comfortable making changes here.

[eli-darkly]

I think probably what we should do is just add a subproject in https://github.com/launchdarkly/dotnet-logging-adapters, where we've already put other optional adapters for things like log4net, each of which is published as a separate NuGet package so no one gets any unwanted dependencies. The only reason we didn't do that in the first place is because we were thinking of this as something that only .NET Core developers would be likely to use, and so it seemed simplest to just put it directly into that one target. We can still leave Logs.CoreLogging where it is as well, with a note to explain where the extra package is for other targets.

[eli-darkly]

And thanks for the feedback. We had created this package to support the upcoming 6.0 release of our .NET SDK, and we hadn't been able to get as much feedback as we were hoping for during the beta period on design issues like this; platform compatibility in particular is something we wanted to make sure we were getting right, as there are so many ways to build .NET apps. And logging had been a particular sore spot in the past— older versions of our SDK did use Microsoft.Extensions.Logging, but .NET Framework developers didn't want that dependency, so we settled on Common.Logging as a compromise but that had its own issues which led to this latest approach.

Anyway, the GA release was also delayed by general busyness on other things, but we were finally getting close to wrapping it up and this is a simple enough change that it'd make sense to take care of it now as part of that.

[eli-darkly]

One thing I'm slightly uncertain about in terms of creating this package. Until recently the stable version of Microsoft.Extensions.Logging in NuGet was 5.0.0. There's now a preview of 6.0.0— not in GA yet, but presumably soon. If we update our adapter to use 6.0.0 in the near future, that's a breaking change but all of the subprojects in https://github.com/launchdarkly/dotnet-logging-adapters are versioned together so it'd be a bit awkward to bump them all to 2.x. Having them together in one repo is much easier to maintain but maybe it is better to break them up.

[jrsearles]

Thanks - it does make sense to keep a separate library. I'm not sure what version it'd make sense to pin it to, but ASP.Net Core 2.1 is still in support so you'd want to make sure it's compatible with that version.

FWIW - i've been using the 6.0-rc1 (not in production yet, but haven't noticed issues.) I wouldn't want this to complicate or delay that release. We can work around it.

[eli-darkly]

Good to know. I don't think this will really affect the release either way.

[eli-darkly]

Hmm... I think maybe some of my comments have been based on remembering something incorrectly. I was talking as if the Microsoft.Extensions.Logging classes are built into the .NET Core runtime but that's not the case, is it. It is always a package dependency. So I'm not really sure why we went the way we did originally with Logs.CoreLogging, but in any case I think the solution is still to have a separate package.

[eli-darkly]

I guess one thing that is different in .NET Core compared to .NET Framework is that at least in .NET Core there's automatic dependency version resolution without assembly bindings, so it's not quite as disruptive to bring in a transitive dependency.

[eli-darkly]

@jrsearles - Please let us know if the 2.0.0 release of LaunchDarkly.Logging.Microsoft works for you. The only difference in usage is that the factory method is now LaunchDarkly.Logging.LdMicrosoftLogging.Adapter rather than LaunchDarkly.Logging.Logs.CoreLogging.

[jrsearles]

I was able to use this library and it has resolved my issue. Thanks a lot!