Add ILoggerFactory-injection to ApiDescriptionGroupCollectionProvider

[captainsafia]

Background and Motivation

To improve the diagnosability of the OpenAPI layer, we'd like to provide logs so that users can understand what components are contributing custom IApiDescriptionProvider implementations to the ApiExplorer layer. In order to support this scenario, we need to support injecting an ILoggerFactory instance to the ApiDescriptionGroupCollectionProvider class that invokes all the discovered IApiDescriptionProvider instances.

Proposed API

- public class ApiDescriptionGroupCollectionProvider : IApiDescriptionGroupCollectionProvider
+ public partial class ApiDescriptionGroupCollectionProvider : IApiDescriptionGroupCollectionProvider
{
+ public ApiDescriptionGroupCollectionProvider(
+       IActionDescriptorCollectionProvider actionDescriptorCollectionProvider,
+       IEnumerable<IApiDescriptionProvider> apiDescriptionProviders,
+       ILoggerFactory loggerFactory)
}

Alternative Designs

• Add a new internal-only implementation of ApiDescriptionGroupCollectionProvider and support ILoggerFactory injection in its constructors.

[dotnet-policy-service]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[BrennanConroy]

Couple comments:

• Should we use ILogger<ApiDescriptionGroupCollectionProvider> instead of ILoggerFactory?
• We could pass IServiceProvider instead so any future additions wouldn't require API additions.
• We could also make a new private implementation that adds the new ctor and is added to DI via our extension methods. This would require 0 new API

[amcasey]

I think I'm probably missing something obvious (and it's probably too late anyway), but why is ApiDescriptionGroupCollectionProvider public? Relatedly, should it be sealed?

[captainsafia]

Should we use ILogger instead of ILoggerFactory?

Good point -- we seem to use ILoggerFactory more frequently in the MVC codepath although I think that's an probably an MVC artifact.

Should we use ILogger instead of ILoggerFactory?

That's an option -- I dunno if we'd need something more in the future and I prefer the straightforwardness of taking an ILogger.

We could also make a new private implementation that adds the new ctor and is added to DI via our extension methods. This would require 0 new API

Yep, that's the alternative design that was proposed above. I'm fine with this approach if we're feelin particularly prickly about introducing new API.

I think I'm probably missing something obvious (and it's probably too late anyway), but why is ApiDescriptionGroupCollectionProvider public? Relatedly, should it be sealed?

It's an old API that was never reviewed so I suspect that there isn't a ton of reason for this. From my knowledge of the area, I don't see a reason to have it be public or extendable (so we can seal it).

[halter73]

One upside of ILoggerFactory is that it's useful if we need to pass it to another subcomponent that logs to a different category. I'm not a big fan of taking an IServiceProvider parameter if we have a good idea of what specific services we need and we don't expect it to change much.

I'm in favor of either approving the API as proposed or making no changes and doing the internal service "Impl" pattern as I like to call it. If this were new API, I think the service would be internal and sealed.

One thing to note about the "Impl" pattern is that AddGrpcSwagger() calls the constructor on the public implementation type from another assembly.

aspnetcore/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.Swagger/GrpcSwaggerServiceExtensions.cs

Lines 36 to 45 in 02e2777

	// Register default description provider in case MVC is not registered
	services.TryAddSingleton<IApiDescriptionGroupCollectionProvider>(serviceProvider =>
	{
	var actionDescriptorCollectionProvider = serviceProvider.GetService<IActionDescriptorCollectionProvider>();
	var apiDescriptionProvider = serviceProvider.GetServices<IApiDescriptionProvider>();
	return new ApiDescriptionGroupCollectionProvider(
	actionDescriptorCollectionProvider ?? new EmptyActionDescriptorCollectionProvider(),
	apiDescriptionProvider);
	});

Assuming we want to add logging in that case as well, we might have to add public constructor. I was going to suggest updating AddGrpcSwagger() to call AddApiExplorer(), but I don't think we want AddGrpcSwagger() to add MVC's DefaultApiDescriptionProvider.

[captainsafia]

Assuming we want to add logging in that case as well, we might have to add public constructor. I was going to suggest updating AddGrpcSwagger() to call AddApiExplorer(), but I don't think we want AddGrpcSwagger() to add MVC's DefaultApiDescriptionProvider.

Yep, I suspect this is the reason the gRPC implementation went the route of registering the ApiExplorer dependencies themselves. Given that this feature helps with diagnosability, I think we should make an effort to enable it for all scenarios where ApiExplorer might be used.

We can't take back the fact that we made this a public API but supporting log for both in-framework and out-of-framework consumes seems like goodness.

[captainsafia]

I've slept on this. I still think making the ILoggerFactory change a public API is the right way to go but I'd advocate for doing this as a .NET 10 change instead of for .NET 9 RC 1. We can ship the logging changes with an internal API for .NET 9 RC 1 and then introduce the public API after we branch.