Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] dependency injection: use of service provider with feature providers #321

Closed
ghelyar opened this issue Nov 18, 2024 · 9 comments · Fixed by #324
Closed

[FEATURE] dependency injection: use of service provider with feature providers #321

ghelyar opened this issue Nov 18, 2024 · 9 comments · Fixed by #324
Labels
enhancement New feature or request

Comments

@ghelyar
Copy link

ghelyar commented Nov 18, 2024

Requirements

I tried to use the new (experimental) dependency injection from 2.1.0 to set up OpenFeature but I found it hard to do this with no access to the service provider from the provider factory.

This causes issues with lifetimes and dependencies.

For example, if you have a client library that is registered as a singleton then you can't re-use that instance from the OpenFeature dependency injection builder. You might want to keep the original client available because it has features that OpenFeature does not have yet, like tracking, but only register a singleton with dependency injection because it keeps a connection to the server and a local cache. You might also want that client cleaned up when the service provider is cleaned up, which doesn't happen if you make it in the factory. If the client library needs access to other services, like an ILogger, you can't access them from the feature provider factory to create the client.

The feature builder has an IServiceCollection but you shouldn't build ephemeral service providers because the lifetime of the services are tied to the lifetime of the service provider, and it can also cause problems with the ordering of the service registrations because it only has access to what is registered at that point.

Just registering a FeatureProvider outside the builder almost works, but fails because HasDefaultProvider is not set, and then it falls back to named but throws because the names are empty (fails because .First() throws on an empty collection)

One solution could be to add an overload of AddOpenFeature that provides access to the IServiceProvider in the callback.

Another possible solution is to change AddProvider<T> to the type of the provider instead of using provider factories, and just register the provider type and let the dependency injection resolve it, so that it can just be given anything it needs in its constructor through normal constructor injection. Basically AddProvider<T> just becomes a convenience method over AddTransient<T> that also sets HasDefaultProvider or something like that, with similar for named/keyed.

Another option is to just let it use whatever is already registered outside the builder.

If I'm just misunderstanding how it should be done then let me know.

cc @arttonoyan

@ghelyar ghelyar added the enhancement New feature or request label Nov 18, 2024
@arttonoyan
Copy link
Contributor

@ghelyar Thank you for your detailed feedback and for pointing out the challenges you encountered with the experimental dependency injection setup for OpenFeature.

I’ll investigate the scenarios you’ve described and consider your suggestions for potential improvements. Once I’ve analyzed the options and evaluated potential changes, I’ll follow up with a detailed response.

@arttonoyan
Copy link
Contributor

@ghelyar
I understand your concerns, and I agree that enhancing the provider registration process could address these issues effectively. One possible solution, as you suggested, could be to allow the provider to be registered as Transient.

Since the provider is primarily used during initialization (e.g., creating a new instance of the provider and setting it to Api.Instance with something like:

await Api.Instance.SetProviderAsync(provider);

the lifetime could then be managed by the Api itself.

This approach ensures that the initialization is straightforward, and the lifecycle of the provider aligns with the Api lifecycle. However, this is just one idea that came to mind. But maybe using ServiceProvider is more flexible; I will investigate deeper and suggest a solution for this.

To address this, I’ll create a ticket and propose improvements to make the process more flexible and intuitive.
Thanks for sharing your insights!

@ghelyar
Copy link
Author

ghelyar commented Nov 20, 2024

Yes, if you allow the service provider to create the feature provider then the lifetime of the feature provider and its dependencies is controlled by the service provider, which allows it to be registered with any lifetime and use any dependencies. Even if the feature provider was transient, if it needed access to singleton services then it could use them through injection.

There might also be a problem with the IFeatureClient being registered as scoped preventing it from being used from singleton services (it would either need to be singleton or transient, probably transient because of the evaluation context it holds), but I haven't got that far yet.

@arttonoyan
Copy link
Contributor

arttonoyan commented Nov 25, 2024

Hi @ghelyar! For clarity, are you wanting to implement something like this and use it?

builder.Services.AddTransient<IFlagConfigurationService, FlagConfigurationService>();
// or
// builder.Services.AddScoped<IFlagConfigurationService, FlagConfigurationService>();
builder.Services.AddOpenFeature(cfg =>
{
    cfg.AddHostedFeatureLifecycle();
    cfg.AddContext(builder => builder.Set("user", "1234"));
    cfg.AddProvider<TestProviderFactory>();
});

#pragma warning disable OFDI001
private class TestProviderFactory : IFeatureProviderFactory
{
    private readonly IFlagConfigurationService _flagConfigurationService;

    public TestProviderFactory(IFlagConfigurationService flagConfigurationService)
    {
        _flagConfigurationService = flagConfigurationService;
    }

    public FeatureProvider Create() => new InMemoryProvider(_flagConfigurationService.GetFlags());
}

internal interface IFlagConfigurationService
{
    Dictionary<string, Flag> GetFlags();
}

public class FlagConfigurationService : IFlagConfigurationService
{
    private readonly IDictionary<string, Flag> _flags;

    public FlagConfigurationService()
    {
        _flags = new Dictionary<string, Flag>
        {
            {
                "feature-a", new Flag<bool>(
                    variants: new Dictionary<string, bool>
                    {
                        { "on", true },
                        { "off", false }
                    },
                    defaultVariant: "on", 
                    context =>
                    {
                        var id = context.GetValue("user").AsString;
                        if (id == null)
                        {
                            return "on"; // Default variant
                        }

                        return id == "123" ? "off" : "on";
                    })
            }
        };
    }

    public Dictionary<string, Flag> GetFlags() => _flags.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
}

Does this align with what you’re aiming to achieve?

@arttonoyan
Copy link
Contributor

@ghelyar If yes, I will create a new PR for this soon. If not, could you please provide more details on how you want to use it? If possible, creating an example would be greatly appreciated.

@ghelyar
Copy link
Author

ghelyar commented Nov 25, 2024

Yes, something like that should work, assuming IFlagConfigurationService could also be registered as a singleton. I would have thought it can but as transient and scoped are called out explicitly I just wanted to check.

Most feature providers just wrap an existing vendor client library that may already be registered with dependency injection, so it's just about getting access to that existing vendor client.

Also the IFeatureClient should work when injected into a constructor parameter of a singleton/scoped/transient service that wants to use it.

@arttonoyan
Copy link
Contributor

arttonoyan commented Nov 25, 2024

Of course, registering it as a singleton would also work.
I will also try to enable registration through the ServiceProvider․

@arttonoyan
Copy link
Contributor

@ghelyar I've submitted a pull request addressing your issue. You can review it here: #324

@ghelyar
Copy link
Author

ghelyar commented Nov 29, 2024

That works well, thank you.

@askpt askpt linked a pull request Dec 2, 2024 that will close this issue
toddbaert pushed a commit that referenced this issue Dec 6, 2024
This update focuses on enhancing the feature provider integration and
testing framework, incorporating improvements to flexibility, usability,
and testing capabilities.
This update addresses [GitHub Issue
#321](#321) in the
OpenFeature .NET SDK repository.

---

### Key Enhancements

1. **Dependency Injection (DI) Enhancements:**
   - Improved lifecycle management for better resource handling.
- Streamlined registration for feature providers, reducing configuration
complexity.
- Introduced the `AddProvider` extension method to simplify and adapt
feature provider integration during service setup.

2. **Simplified Codebase:**
- Removed `FeatureProviderFactory` logic, eliminating unnecessary
complexity and improving usability.

3. **Improved InMemoryProvider:**
- Enhanced the registration process for the `InMemoryProvider`, enabling
smoother and more intuitive usage.

4. **Testing Improvements:**
- Established a dedicated integration test project for comprehensive
validation.
- Improved overall test coverage, ensuring the reliability and
robustness of the framework.

---------

Signed-off-by: Artyom Tonoyan <arttonoyan@gmail.com>
Co-authored-by: chrfwow <christian.lutnik@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants