Microsoft.Extensions.Http depends on concrete logging and dependency injection packages

[antoinebj]

Hi,

I was surprised when adding Microsoft.Extensions.Http that I had to accept the licenses for Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Logging instead of just their abstraction counterparts.

IMHO it's counter-intuitive that an HTTP client factory would need access to concrete logging and not its abstractions, and that logging would need access to the concrete DI container implementation package, and not just the abstractions.
That strong coupling appears to defeat the purpose of having abstraction packages.

Unfortunately I don't have time right now to delve inside the source code, can someone explain why it has to be like that? Or whether it can be improved in the future?

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[KalleOlaviNiemitalo]

The LoggingServiceCollectionExtensions.AddLogging call in HttpClientFactoryServiceCollectionExtensions.AddHttpClient causes Microsoft.Extensions.Http to depend on Microsoft.Extensions.Logging:

runtime/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs

Lines 23 to 31 in c5a3f49

	public static IServiceCollection AddHttpClient(this IServiceCollection services)
	{
	if (services == null)
	{
	throw new ArgumentNullException(nameof(services));
	}
	services.AddLogging();
	services.AddOptions();

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggingServiceCollectionExtensions.cs

Line 21 in c5a3f49

public static IServiceCollection AddLogging(this IServiceCollection services)

Microsoft.Extensions.Logging then depends on Microsoft.Extensions.DependencyInjection, even though Microsoft.Extensions.Http itself doesn't.

[KalleOlaviNiemitalo]

Microsoft.Extensions.Logging depends on Microsoft.Extensions.DependencyInjection because LoggerFactory.Create creates a ServiceCollection:

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 93 to 95 in c5a3f49

	public static ILoggerFactory Create(Action<ILoggingBuilder> configure)
	{
	var serviceCollection = new ServiceCollection();

runtime/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceCollection.cs

Line 12 in c5a3f49

public class ServiceCollection : IServiceCollection

LoggerFactory.Create was added to fix dotnet/extensions#615. You can still use other dependency container implementations with Microsoft.Extensions.Logging, just not with LoggerFactory.Create.

[KalleOlaviNiemitalo]

If you register ILoggerFactory and ILogger<> services before you call AddHttpClient, then LoggingServiceCollectionExtensions.AddLogging won't add the concrete types defined in Microsoft.Extensions.Logging. So the dependencies do not prevent you from using other logging implementations; they just require your application to carry some dead weight. I wonder whether the IL linker can be configured to delete those calls.

[antoinebj]

Thanks for your detailed replies with links to source code 👍

Well I feel like I'm digging up decisions from years ago, but I see what happened for logging: dotnet/extensions#640
As noted by @davidfowl in that PR, that was unfortunate.
While I understand that it's convenient to have this turn-key solution to create the factory, I find it also pretty confusing, awkward and worrying to see the concrete DI package brought along. I find it also a bit annoying when using another DI container for my app to see Microsoft's brought along even when I'm not going to use it. And lastly, it could be unfair to other DI libraries, as people might just think that if they're going to have to have that package, might as well use it and drop the other. All those inconveniences for just 1 single convenience method that probably be called from just 1 single line of code in the application. Not sure if it's worth it.
I'd have been tempted to just add an IServiceCollection parameter to Create() and let the developer import the DI package to pass new ServiceCollection(). I know it's not as turn-key, but to paraphrase what was said in the aforementioned PR, it doesn't look like the end of the world either.

Regarding the dependency of M.E.Http on M.E.Logging, is the rationale behind that related to the caller of AddHttpClient() not giving any hint that logging needs to be registered, and therefore to preserve them from surprise? Won't they be even more surprised if they register their logger after the HTTP client and it fails silently to do what it was expected to do? Isn't that worse than getting a runtime exception about the DI container not knowing how to instantiate ILogger<>?

[KalleOlaviNiemitalo]

Microsoft.Extensions.Logging depending on Microsoft.Extensions.DependencyInjection

If an application does not call LoggerFactory.Create, I imagine an IL linker can easily detect that and remove the method and all of Microsoft.Extensions.DependencyInjection. I don't see other ways to remove the dependency now that LoggerFactory.Create has already been in released package versions.

Microsoft.Extensions.Http depending on Microsoft.Extensions.Logging

If an application now uses Microsoft.Extensions.Http but does not otherwise reference the Microsoft.Extensions.Logging.* packages, then I think it doesn't get any ILoggerProvider registered in the dependency injection container, so the logging won't work anyway. Could Microsoft.Extensions.Http make logging optional? Never call LoggingServiceCollectionExtensions.AddLogging, and add alternative constructors or default parameter values so that the dependency injection container can construct Microsoft.Extensions.Http types without ILoggerFactory:

• DefaultHttpClientFactory

  runtime/src/libraries/Microsoft.Extensions.Http/src/DefaultHttpClientFactory.cs

  Lines 61 to 66 in c5a3f49

  	public DefaultHttpClientFactory(
  	IServiceProvider services,
  	IServiceScopeFactory scopeFactory,
  	ILoggerFactory loggerFactory,
  	IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor,
  	IEnumerable<IHttpMessageHandlerBuilderFilter> filters)

• LoggingHttpMessageHandlerBuilderFilter

  runtime/src/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandlerBuilderFilter.cs

  Line 17 in c5a3f49

  public LoggingHttpMessageHandlerBuilderFilter(ILoggerFactory loggerFactory, IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor)

  In this case, LoggingHttpMessageHandlerBuilderFilter.Configure would not create instances of LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler, so the constructors of those types would still be able to require ILogger.

This change would make it possible to remove the Microsoft.Extensions.Logging reference while avoiding runtime exceptions, I think. Would it break anything?

[davidfowl]

I have been following this thread but haven't heard a strong reason behind the why we would do this. I understand the layering concerns but what would you be solving by doing this?

[KalleOlaviNiemitalo]

Personally, I haven't been troubled by these dependencies; but I'd like to understand the best practices.

[antoinebj]

I have been following this thread but haven't heard a strong reason behind the why we would do this. I understand the layering concerns but what would you be solving by doing this?

Imagine if instead of M.E.Logging, it were bringing in Serilog, or NLog. Or if instead of M.E.DI it were bringing in Autofac or Unity. Would it be clearer then that we're bringing in additional potentially redundant "clutter" (no disrespect to those libraries), increasing the number of assemblies for the sake of slightly reducing the complexity of a single line of code in the entire application?
It's a tough sell to say that it will save around 100 KB (71 KB for DI + 33 KB for Logging) of storage and possibly RAM from being used unnecessarily. Honestly the main driver for me to write this issue is that it just feels dirty to have this whole loose coupling design only end up being dependent on a concrete implementation package. You did call it "unfortunate" in the PR, so you must have felt the same at some level.
Another thing, is that once you start accepting dirt somewhere, you'll have to accept it elsewhere. It's basically setting a bad precedent.

I'm raising the issue, but feel free to close it if you don't think it's worth consideration :)

[CarnaViire]

Triage: the design choice was done much earlier in HttpClientFactory. Now the dependency cannot be removed without breaking existing user code (that relies on logging being added automatically). The gain doesn't seem worth of introducing a breaking change, so closing.