Enable AIFunctionFactory.Create functions to get DI services, Alternate 1

[stephentoub]

Alternate to #6141. Don't use attributes, just special-case IServiceProvider parameters in AIFunctionFactory.Create, still using AIFunctionArguments.

Microsoft Reviewers: Open in CodeFlow

[SteveSandersonMS]

I think it would be more conventional to retrieve an ILoggerFactory and use that to construct an ILogger<FunctionInvokingChatClient>. Developers won't normally register an ILogger<FunctionInvokingChatClient> explicitly.

[stephentoub]

I think it would be more conventional to retrieve an ILoggerFactory and use that to construct an ILogger

Doesn't DI do that automatically?

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

ServiceCollection c = new();
c.AddLogging(c => c.AddConsole());
IServiceProvider services = c.BuildServiceProvider();

ILogger<MySpecialType> logger = services.GetRequiredService<ILogger<MySpecialType>>();
Console.WriteLine(logger);

class MySpecialType { }

[halter73]

One problem/benefit of taking an ILogger is that whoever constructs the FunctionInvokingChatClient controls the category name of logs emitted by it. That's kind of cool, but very unusual. I now once again lean towards sticking with convention and taking an ILoggerFactory even though I said it was okay to leave as-is before.

You could take an ILogger<FunctionInvokingChatClient> to essentially force the caller to user the right category name, but then there's no real benefit over just taking an ILoggerFactory. It would prevent us from ever logging in a subcategory or something like that in the future.

And while on the topic of sticking with conventions, I don't think we should fall back to the IServiceProvider to get the logger when a null logger is passed in. We talked about this before here. I said it was okay to leave it as-is then, but it still leaves a bad taste in my mouth.

I can't think of any other place where a constructor takes an IServiceProvider and another service in the same signature and uses the IServiceProvider as a fallback when the other service is missing. UserManager doesn't have this type of fallback.

I'd feel better about the fallback behavior if FunctionInvokingChatClient(IChatClient innerClient, IServiceProvider services) was a separate constructor.

[stephentoub]

The point of taking an ILogger was that someone could use this if they already have one.

If we're not going to do that, what's the benefit of taking an ILoggerFactory at all? If you can pass in an IServiceProvider, the implementation could fish the ILoggerFactory or ILogger out of there.

Are there scenarios where you have an ILoggerFactory that doesn't come from DI?

[halter73]

AFAIK, logging to someone else's ILogger isn't supported by any classes we ship. It's always an ILoggerFactory or ILogger<ServiceType>, because it's the type controls the logging category, not whoever constructed an instance of that type. That way you get consistent logging categories and IDs when looking at logs for a random application.

We don't have too many types that straddle the world between being a DI service and being easily constructable on their own, but MemoryCache follows this pattern and takes an ILoggerFactory rather than an ILogger.

I do think that controlling the log category on a per-instance basis might be an interesting feature. I've even thought controlling the log category might be useful for MemoryCache, since it can be used for so many different purposes in the same application, but I don't think it's so important for this API that we should break our standard conventions. We could always add an ILogger overload later if we really need this functionality.

Are there scenarios where you have an ILoggerFactory that doesn't come from DI?

You could use LoggerFactory.Create(). I've noticed the mcpdotnet tests use it.

https://learn.microsoft.com/dotnet/core/extensions/logging

If we're not going to do that, what's the benefit of taking an ILoggerFactory at all? If you can pass in an IServiceProvider, the implementation could fish the ILoggerFactory or ILogger out of there.

We could, but we'd basically be following the "service locator pattern" at that point. It wouldn't be nearly as discoverable that FunctionInvokingChatClient will do its own logging if you provide it a logger.

I like the clean break where the IServiceProvider is for providing arbitrary services to the AIFunction, and any service types we know get used internally by the FunctionInvokingChatClient are provided as separate constructor parameters in the longest overload.

I'm fine with also introducing a shorter overload that takes just the inner IChatClient and an IServiceProvider, and that having that resolve the logger from DI. We could go even further and use the IServiceProvider from the inner IChatClient as I suggest here. But in either case, I still think there should be a longer overload that takes an ILoggerFactory. At least that would make it clear to callers that FunctionInvokingChatClient can emit its own logs.

[stephentoub]

I like the clean break where the IServiceProvider is for providing arbitrary services to the AIFunction, and any service types we know get used internally by the FunctionInvokingChatClient are provided as separate constructor parameters in the longest overload.

OK, so you're viewing the IServiceProvider not as something thee FICC uses itself but that is just an opaque object that's passed through. That's reasonable, though then having other overloads that do query the service provider muddies the waters, no? There are lots of APIs and delegates that pass around an IServiceProvider for resolution purposes... how is a consumer supposed to know when it'll be used in that capacity or not? My assumption is giving something a service provider is giving it access to everything, and it may access everything. Whether it does so explicitly via GetService or instantiaies objects that take a logger and have that populated using ActivatorUtilities, I don't see the difference. Both do logging. Both resolve a logger from the service provider. One just explicitly calls the method and the other does so by calling a method that calls it.

[halter73]

OK, so you're viewing the IServiceProvider not as something thee FICC uses itself but that is just an opaque object that's passed through. That's reasonable, though then having other overloads that do query the service provider muddies the waters, no?

I admit allowing the overload to query the service provider muddies the waters a bit, but that's still where I draw the line. It leaves the door open for us to add new dependencies like IMeterFactory to FunctionInvokingChatClient post 1.0 and use it without being forced to update old code to call the new constructor.

However, in that case, I think we should still add a new constructor that takes IMeterFactory and doesn't fall back to the service provider. If a null meter factory is passed in, there should be no metrics same as with logging. And unlike with logging, I don't think there's a NullMeterFactory.Instance to make it easy to disable metrics otherwise.

I think that if you're calling an overload that takes an optional dependency in the parameter list and you omit it, it should not be used. The fact that you happened to need the IServiceProvider for other reasons doesn't make it a good idea to use the service locator pattern for every other dependency.

I realize that it's a matter of taste, and there's no objectively right answer here though. If I had to rank my preference from most preferred to least, I'd go with:

1. Only use the IServiceProvider as a fallback for dependencies that couldn't be provided directly with the given overload.
2. Don't use the IServiceProvider as a fallback for any dependencies.
3. Use the IServiceProvider anywhere an optional dependency hasn't been provided.

But I think any of these are fine as long as we're consistent.

[stephentoub]

Replaced by #6158