.Net: First round of MEVD DI changes

[roji]

• Align to the .NET (and MEAI) standard of having AddQdrant() (no service key) and AddKeyedQdrant() (non-optional service key).
• Rename serviceId to the more standard serviceKey naming, and change its type from string to object.
• Add ServiceLifetime parameters to all methods.
• Obsolete the KernelBuilderExtensions, in preparation for #10855

Contributes to #10549

/cc @westey-m @adamsitnik @dmytrostruk

[adamsitnik]

The code LGTM, but IMO if we want to introduce any breaking changes we should introduce more changes (remove most of the overloads, accept factory methods that take ServiceProvider as an argument). PTAL at my comments and let me know what is your opinion.

[adamsitnik]

Why Transient is the default? I would hope Azure AI Search SDK client to be the same as other Azure SDK clients:

https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients/#client-lifetime

[roji]

I agree - this is specifically the thing I want to change in the follow-up to this PR, just didn't want to put everything in at once.

[roji]

Yep, I agree - see #10549 (comment) and below for the discussion. I'm mainly saving this for a later PR as it's a more important behavioral change, and I didn't want to do it all in one PR.

[adamsitnik]

why is the objectKey nullable?

[roji]

Shouldn't be, thanks.

[adamsitnik]

I know that all these overloads already existed, but I just want to share my experience from HealthChecks project. You can read Xabaril/AspNetCore.Diagnostics.HealthChecks#2040, but to tell the long story short:

• all Azure SDK clients should be registered as singletons by default (they are thread safe)
• users may need to access IServiceProvider when creating the option bags (to for example read sth from the config system)
• each client may provide various ctors that accept various arguments. To simplify the API we can just allow the users to provide a factory method that returns the client rather than having a dedicated overload for each client ctor (like here with Url, TokenCredential etc)

Having said that, the API shape could be following:

public static IServiceCollection AddAzureAISearchVectorStore(
    this IServiceCollection serviceCollection,
    Func<IServiceProvider, SearchIndexClient>? clientFactory = default, // when not provided, we just resolve it from the DI
    Func<IServiceProvider, AzureAISearchVectorStoreOptions>? optionsFactory = default,
    ServiceLifetime lifetime = ServiceLifetime.Singleton)
    
public static IServiceCollection AddKeyedAzureAISearchVectorStore(
    this IServiceCollection serviceCollection,
    object serviceKey // mandatory to provide,
    Func<IServiceProvider, SearchIndexClient>? clientFactory = default, // when not provided, we just resolve it from the DI
    Func<IServiceProvider, AzureAISearchVectorStoreOptions>? optionsFactory = default,
    ServiceLifetime lifetime = ServiceLifetime.Singleton)

[roji]

all Azure SDK clients should be registered as singletons by default (they are thread safe)

Yep, see above.

users may need to access IServiceProvider when creating the option bags (to for example read sth from the config system)

Good point - I overlooked that, thanks! I'm thinking to add an optional Func<IServiceProvider, SearchIndexClient> parameter to the existing APIs; if it's provided, it's used, otherwise we attempt to resolve the SearchIndexClient from DI. How does that sound?

each client may provide various ctors that accept various arguments. To simplify the API we can just allow the users to provide a factory method that returns the client rather than having a dedicated overload for each client ctor (like here with Url, TokenCredential etc)

Sure, though I think that it's a bit nicer for the easy/default path to just have such sugar on it, so a single UseAzureAISearchVectorStore() call is enough to set everything up. Of course we shouldn't go overboard with it and add many of these.

[adamsitnik]

Is my understanding correct that it's going to work only for a single collection? As soon as the user tries to register two, we are going to fail?

I wonder if we should simply always use Keyed DI for collections, were the key would be the collection name.

[roji]

I think the idea here is that the single-collection scenario is common enough for this to make sense as dedicated sugar... Users do have AddKeyedAzureAISearchVectorStoreRecordCollection it they want to work with multiple collections. Does that make sense?