open-feature / dotnet-sdk

.NET implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
68 stars 19 forks source link

feat: Add Dependency Injection and Hosting support for OpenFeature #310

Open arttonoyan opened 2 weeks ago

arttonoyan commented 2 weeks ago

[!NOTE] This initial version of OpenFeature.DependencyInjection and OpenFeature.Hosting introduces basic provider management and lifecycle support for .NET Dependency Injection environments. While I haven't included extensive tests, particularly for the Hosting project, if this approach is approved and the scope is considered sufficient for the first DI and Hosting release, I will expand the test coverage to include both unit and integration tests. Additionally, I'll provide a sample application to demonstrate the usage.

This pull request introduces key features and improvements to the OpenFeature project, focusing on Dependency Injection and Hosting support:

These changes significantly improve OpenFeature's extensibility and lifecycle management for feature providers within Dependency Injection (DI) and hosted environments.


NuGet Packages for installation:

dotnet add package OpenFeature
dotnet add package OpenFeature.DependencyInjection
dotnet add package OpenFeature.Hosting

Usage Example:

builder.Services.AddOpenFeature(featureBuilder => {
    featureBuilder
        .AddHostedFeatureLifecycle() // From Hosting package
        .AddContext((context, serviceProvider) => {
            // Context settings are applied here. Each feature flag evaluation will
            // automatically have access to these context parameters.
            // Do something with the service provider.
            context
                .Set("kind", "tenant")
                .Set("key", "<some key>");
        })
        // Example of a feature provider configuration
        // .AddLaunchDarkly(builder.Configuration["LaunchDarkly:SdkKey"], cfg => cfg.StartWaitTime(TimeSpan.FromSeconds(10))); 
});
askpt commented 2 weeks ago

Hey @arttonoyan! The build is failing because of compliance. Could you please follow these steps? https://github.com/open-feature/dotnet-sdk/pull/310/checks?check_run_id=31479940982

beeme1mr commented 1 week ago

Hey @arttonoyan, thanks for the PR. I'll let the .NET experts provide more implementation-specific feedback. I just wanted to say that it looks great from a general OpenFeature perspective.

Could you please sign off your commits? It's a requirement from the CNCF that's basically a way to indicate that you're willing and able to donate the code to the CNCF.

This can be done by:

arttonoyan commented 1 week ago

@askpt Thanks for your review and comments! Feel free to adjust the formatting or suggest any improvements you think would help. I’ve copied some of code from your PR )). I’m aiming to wrap up this package as soon as possible and start brainstorming new features. In the meantime, I’m working on fixing the DCO error.

askpt commented 1 week ago

@askpt Thanks for your review and comments! Feel free to adjust the formatting or suggest any improvements you think would help. I’ve copied some of code from your PR )). I’m aiming to wrap up this package as soon as possible and start brainstorming new features. In the meantime, I’m working on fixing the DCO error.

@arttonoyan I just pushed the dotnet format fixes 👍

toddbaert commented 1 week ago

I think this is on the right track. Could you provide a usage guide in the README as well?

I'd also like to know your thoughts about how we could add some additional features; mostly because I'd like to prevent breaking changes: should we support the configuration of hooks? Request scoped injection of clients? Request scoped evaluation of flags?

arttonoyan commented 1 week ago

I think this is on the right track. Could you provide a usage guide in the README as well?

I'd also like to know your thoughts about how we could add some additional features; mostly because I'd like to prevent breaking changes: should we support the configuration of hooks? Request scoped injection of clients? Request scoped evaluation of flags?

Regarding the README – Absolutely, I will provide a usage guide.

Regarding hooks – This requires some careful consideration. This is my first experience using feature flags with this standard, and I don’t have a solid example yet to test and determine the best approach.

Regarding scoped services – While request-scoped configurations may seem beneficial, they introduce several challenges:

  1. You cannot use singletons in classes.
  2. There are significant issues when integrating with HTTP client’s message handlers. Specifically, we’ve encountered cases where the context was lost (see: Understanding Scopes with IHttpClientFactory). This happens because message handlers are long-lived and don’t adhere to scoped lifetimes. For these reasons, features like HttpContextAccessor are often registered as singletons to avoid such issues.

I personally prefer having a singleton version, but it is a bit challenging to develop, and we should be very careful with it and conduct thorough testing.

The code I have written is already being used by several teams in our company, and there is real testing happening in these environments. I would prefer to have a preview version that allows me to replace most of our current code with this new package while continuing to work on the next version in parallel. This approach will enable me to test the changes in our projects first before proposing modifications here.

One potential path forward could be to create an initial package with these considerations and mark it as a preview. We can then develop a second version that registers IFeatureClient as a singleton, while providing IFeatureClientSnapshot for scoped registrations. This aligns with good practices that Microsoft also follows, for instance, in the use of IFeatureManagerSnapshot in the Feature Management library (reference).

toddbaert commented 5 days ago

@arttonoyan @askpt @lukas-reining @kinyoklion What do you think about using Experimental on some of these classes/interfaces to make clear they may change? That might be easier than creating a preview branch (which is what I think we'd have to do to release a separate artifact).

That way, people would get a clear warning they are using a subset of our feature which are subject to change?

lukas-reining commented 5 days ago

What do you think about using Experimental on some of these classes/interfaces to make clear they may change? That might be easier than creating a preview branch (which is what I think we'd have to do to release a separate artifact).

Makes sense to me. We did this for the TransactionContext Propagation in JS too. And as this is available from .NET out-of-the-box I think this is a good way to get this tested.

arttonoyan commented 5 days ago

@toddbaert @askpt @lukas-reining @kinyoklion let's try to speed up the pace a bit. I think marking it as experimental is a solid approach. We can mark it as experimental, resolve all the conversations, and incorporate the other suggestions to have a first experimental version. Afterward, I can replace my changes in our company package with this version, which will allow for natural testing.

Additionally, I will work on improving it further, focusing on making IFeatureClient a singleton and adding a new IFeatureClientSnapshot interface as scoped. In our package, I've used a strategy pattern for the context because we often deal with a few common strategies. For example, many teams are using TenantContext, where the information always comes from the request, and we utilize OperationContextAccessor to resolve that information. Here's an example of how it looks:

builder.Services.AddOpenFeature(featureBuilder =>
{
    featureBuilder
        .AddHostedFeatureLifecycle()
        .AddTenantContext(cfg => cfg.UseOperationContext())
        .AddLaunchDarkly(builder.Configuration["LaunchDarkly:SdkKey"]);
});

In this setup, AddTenantContext adds some tenant-specific context definitions, while UseOperationContext pulls data from the operation context - it's a concrete strategy that we are using in our system (Note, the Strategy is just an idea that needs further discussion)

I'll continue refining this as we move forward.

askpt commented 5 days ago

@toddbaert @askpt @lukas-reining @kinyoklion let's try to speed up the pace a bit. I think marking it as experimental is a solid approach. We can mark it as experimental, resolve all the conversations, and incorporate the other suggestions to have a first experimental version. Afterward, I can replace my changes in our company package with this version, which will allow for natural testing.

Additionally, I will work on improving it further, focusing on making IFeatureClient a singleton and adding a new IFeatureClientSnapshot interface as scoped. In our package, I've used a strategy pattern for the context because we often deal with a few common strategies. For example, many teams are using TenantContext, where the information always comes from the request, and we utilize OperationContextAccessor to resolve that information. Here's an example of how it looks:


builder.Services.AddOpenFeature(featureBuilder =>

{

    featureBuilder

        .AddHostedFeatureLifecycle()

        .AddTenantContext(cfg => cfg.UseOperationContext())

        .AddLaunchDarkly(builder.Configuration["LaunchDarkly:SdkKey"]);

});

In this setup, AddTenantContext adds some tenant-specific context definitions, while UseOperationContext pulls data from the operation context - it's a concrete strategy that we are using in our system (Note, the Strategy is just an idea that needs further discussion)

I'll continue refining this as we move forward.

@arttonoyan I agree with you at 100%. As per the Experimental attribute, I suggest we have a section in the readme that explains why is experimental and use the URL property to point to that section.

If you need any help with the release let me know and we can sync.

arttonoyan commented 5 days ago

@toddbaert @askpt @lukas-reining @kinyoklion let's try to speed up the pace a bit. I think marking it as experimental is a solid approach. We can mark it as experimental, resolve all the conversations, and incorporate the other suggestions to have a first experimental version. Afterward, I can replace my changes in our company package with this version, which will allow for natural testing. Additionally, I will work on improving it further, focusing on making IFeatureClient a singleton and adding a new IFeatureClientSnapshot interface as scoped. In our package, I've used a strategy pattern for the context because we often deal with a few common strategies. For example, many teams are using TenantContext, where the information always comes from the request, and we utilize OperationContextAccessor to resolve that information. Here's an example of how it looks:

builder.Services.AddOpenFeature(featureBuilder =>

{

    featureBuilder

        .AddHostedFeatureLifecycle()

        .AddTenantContext(cfg => cfg.UseOperationContext())

        .AddLaunchDarkly(builder.Configuration["LaunchDarkly:SdkKey"]);

});

In this setup, AddTenantContext adds some tenant-specific context definitions, while UseOperationContext pulls data from the operation context - it's a concrete strategy that we are using in our system (Note, the Strategy is just an idea that needs further discussion) I'll continue refining this as we move forward.

@arttonoyan I agree with you at 100%. As per the Experimental attribute, I suggest we have a section in the readme that explains why is experimental and use the URL property to point to that section.

If you need any help with the release let me know and we can sync.

Thanks, @askpt! I will go ahead and complete the README section, as suggested. I'll also work on fixing the DCO issue again, and once that's sorted, we can proceed with the release. It would be great to have your help with the release process - let's sync up when we're ready to move forward.

arttonoyan commented 1 day ago

@arttonoyan @askpt @lukas-reining I support releasing this as-is if we mark all new APIs as experimental and do something about this; I think we need to clarify which context level is being set here, preferably with the method name or at least with docs. Please let me know if I'm missing something on that.

I'll approve once those are resolved.

I would prefer to see this resolved as well, but it's not a blocker for me.

@toddbaert and team,

I wanted to update you on the task you mentioned – I’m actively working on it, and while it’s a bit complex, I’ve completed most of the code. I’m also running thorough tests to ensure everything works as expected.

I’m working in a separate branch, and within the next few days, I’ll open a new PR for review. The process has taken a bit longer as I explored multiple solutions to find the best approach.

arttonoyan commented 22 hours ago

@arttonoyan @askpt @lukas-reining I support releasing this as-is if we mark all new APIs as experimental and do something about this; I think we need to clarify which context level is being set here, preferably with the method name or at least with docs. Please let me know if I'm missing something on that. I'll approve once those are resolved. I would prefer to see this resolved as well, but it's not a blocker for me.

@toddbaert and team,

I wanted to update you on the task you mentioned – I’m actively working on it, and while it’s a bit complex, I’ve completed most of the code. I’m also running thorough tests to ensure everything works as expected.

I’m working in a separate branch, and within the next few days, I’ll open a new PR for review. The process has taken a bit longer as I explored multiple solutions to find the best approach.

@toddbaert, @askpt and team, guys! I've completed the Domain-Scoped Providers Configuration logic to support multiple providers. However, I created my changes in a new branch off this one, and now I'm trying to open a new PR back to it, but I'm only seeing the option to merge into the main branch. Could someone help me figure this out?

beeme1mr commented 21 hours ago

I'll take a look in a bit.

askpt commented 19 hours ago

@arttonoyan it seems you created a new branch in your fork. So you will need to create a PR from di-domain-scoped-providers to add-dependency-injection. You then can share the PR which will have the diff from di-domain-scoped-providers against the PR you are proposing to the main repo. After it is merged, the changes will be visible in this PR. Makes sense?

arttonoyan commented 19 hours ago

@arttonoyan it seems you created a new branch in your fork. So you will need to create a PR from di-domain-scoped-providers to add-dependency-injection. You then can share the PR which will have the diff from di-domain-scoped-providers against the PR you are proposing to the main repo. After it is merged, the changes will be visible in this PR. Makes sense?

Thanks, @askpt ! You’re right; I created the branch from my fork. It's a bit late for me now, but I’ve opened the PR on my end and would love to share it with you for review. This way, you’ll still have a chance to take a look. Thanks in advance! https://github.com/arttonoyan/dotnet-sdk/pull/1

arttonoyan commented 13 hours ago

dependency

@arttonoyan it seems you created a new branch in your fork. So you will need to create a PR from di-domain-scoped-providers to add-dependency-injection. You then can share the PR which will have the diff from di-domain-scoped-providers against the PR you are proposing to the main repo. After it is merged, the changes will be visible in this PR. Makes sense?

Thanks, @askpt ! You’re right; I created the branch from my fork. It's a bit late for me now, but I’ve opened the PR on my end and would love to share it with you for review. This way, you’ll still have a chance to take a look. Thanks in advance! arttonoyan#1

Guys, if you’re on board with my approach to developing multi-provider support, let me know. I can merge it into my current branch so we can continue our discussion on the main PR.