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

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

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

arttonoyan
Copy link
Contributor

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:

  • OpenFeature.DependencyInjection Project:

    • Implemented OpenFeatureBuilder, including OpenFeatureBuilderExtensions for seamless integration.
    • Added IFeatureLifecycleManager interface and its implementation.
    • Introduced AddProvider extension method for easy provider configuration.
    • Created OpenFeatureServiceCollectionExtensions for service registration.
  • OpenFeature.Hosting Project:

    • Added HostedFeatureLifecycleService to manage the lifecycle of feature providers in hosted environments.
  • Testing Enhancements:

    • Created unit tests for critical methods, including OpenFeatureBuilderExtensionsTests and OpenFeatureServiceCollectionExtensionsTests.
    • Replicated and tested NoOpFeatureProvider implementation for better test coverage.

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))); 
});

@arttonoyan arttonoyan requested a review from a team as a code owner October 13, 2024 19:12
@beeme1mr beeme1mr changed the title Add Dependency Injection and Hosting support for OpenFeature feat: Add Dependency Injection and Hosting support for OpenFeature Oct 13, 2024
@askpt askpt linked an issue Oct 14, 2024 that may be closed by this pull request
@askpt
Copy link
Member

askpt commented Oct 14, 2024

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
Copy link
Member

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:

Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good. A few comments related to the Logging.

I also encourage you to run dotnet format OpenFeature.sln so it will clear some of the errors that the check-format action is throwing.

@arttonoyan arttonoyan force-pushed the add-dependency-injection branch from 70708ec to 1acb4fa Compare October 16, 2024 10:42
@arttonoyan
Copy link
Contributor Author

arttonoyan commented Oct 16, 2024

@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
Copy link
Member

askpt commented Oct 17, 2024

@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 👍


services.TryAddSingleton(Api.Instance);
services.TryAddSingleton<IFeatureLifecycleManager, FeatureLifecycleManager>();

Copy link

@wwalendz-relativity wwalendz-relativity Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having branching in all places where we would like to consume ExecutionContext we could always register empty instance of ExecutionContext. This will end up with more "pure" methods. In that case IsContextConfigured is not needed.
services.TryAddSingleton(ExecutionContext.Empty);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don’t register the EvaluationContext as a singleton because the client can operate without any context or with null. Under the hood, it checks and sets the Empty context as needed. The purpose of IsContextConfigured is to avoid redundant service resolution and nullability checks. If no context is registered, we bypass fetching the EvaluationContext entirely, preventing unnecessary GetService or GetRequiredService calls.

Also, when the context is registered, I register it as transient. The reason for this is to control its lifecycle within the scope of the class where it’s used—in this case, IFeatureClient. Will be confusing to register the context as transient in one case and Empty as singleton in another. However, this approach ensures proper lifecycle management based on the context's availability and usage.

Additionally, the IsContextConfigured check is only performed once during the service provider building process. In scenarios where context resolution isn’t required, the IFeatureClient can still function without a context, but engineers can always explicitly use ExecutionContext.Empty if needed

{
builder.Services.Configure<FeatureLifecycleStateOptions>(cfg =>
{
cfg.StartState = FeatureStartState.Starting;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those defaults are already set in property initializers. Additionally this code will override any values which user could set before executing AddHostedFeatureLifecycle() (for example bound from appsettings.json) because IConfigureOptions<T> are executed in order of their registration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out! I’ll review the property initializers and check how this affects the configuration flow, especially with values set from appsettings.json. I appreciate the heads-up and will make adjustments accordingly and get back to you soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wwalendz-relativity Are you suggesting replacing the current implementation with something like this:
builder.Services.AddOptions<FeatureLifecycleStateOptions>();?
I’m a bit confused, as I believe user-defined configurations, such as those set via appsettings.json, should still take precedence. Could you provide a code snapshot of how you'd suggest replacing the existing code, along with a real-world scenario? This will help clarify how we avoid overriding any user-set configurations unintentionally.

public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToken = default)
{
_logger.LogInformation("Starting initialization of the feature provider");
var featureProvider = _serviceProvider.GetService<FeatureProvider>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason for not resolving FeatureProvider directly in the ctor beside having custom exception for that situation? In effect it not allow call-site validation to detect that FeatureProvider is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for not resolving FeatureProvider directly in the constructor is that some implementations of FeatureProvider are not fully controlled by us, and they may execute significant logic during construction. For example, certain providers like LaunchDarkly perform extensive initialization within the constructor, which can take time and potentially fail (as seen in LaunchDarkly's Provider Implementation and LdClient Constructor).

By not resolving FeatureProvider in the constructor, we can safely perform this logic asynchronously in a start method, which avoids delays or failures during object instantiation. Additionally, I use GetService to log meaningful messages in case of any issues.

In short, this approach allows us to handle potential failures asynchronously during startup and ensures proper logging in case of errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally providers are doing such initialization in their initialize() method, but we can't control them all, and I see your point. Generally I'd prefer constructor resolution as well but I can understand the advantages you describe.

var api = provider.GetRequiredService<Api>();
var client = api.GetClient();
var context = provider.GetRequiredService<EvaluationContext>();
client.SetContext(context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context can be added at both the global and client level. Global context is most useful for static values (for example, a value representing the timezone of the server or the cloud provider it's running on, or the application version).

We might need multiple contexts setting methods for the different scopes: global, client, and perhaps transaction level once we implement the transaction propagation feature. As it is, I'm not sure it's obvious to a user which one would be used when they do AddContext(...)

{
throw new InvalidOperationException("Feature provider is not registered in the service collection.");
}
await _featureApi.SetProviderAsync(featureProvider).ConfigureAwait(false);
Copy link
Member

@toddbaert toddbaert Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want an optional value for domain, or some other method for working with domain-scoped providers.

Comment on lines 11 to 17
#if NET8_0_OR_GREATER
ArgumentNullException.ThrowIfNull(value, name);
#else
if (value is null)
throw new ArgumentNullException(name);
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think it's better just do use the version in the #else here; using both a "nice" API and a "less nice" API is more complex than just using the "less nice" API, IMO.

Unless there some performance implication here I think this just hurts readability a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@toddbaert
Copy link
Member

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
Copy link
Contributor Author

arttonoyan commented Oct 18, 2024

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).

/// Describes a <see cref="OpenFeatureBuilder"/> backed by an <see cref="IServiceCollection"/>.
/// </summary>
/// <param name="Services">The <see cref="IServiceCollection"/> instance.</param>
public sealed record OpenFeatureBuilder(IServiceCollection Services)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the pattern/reasoning behind using a sealed record and extensions here?
I have not too much experience with .NET so I am mostly wondering why this is the chosen/preferred way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukas-reining Thanks for your review.

Reasoning for using a record in OpenFeatureBuilder and extension methods:

  1. Immutability:
    Using a record in OpenFeatureBuilder ensures immutability, which is crucial in configurations where state changes should not happen after initialization. Records in C# provide an easy way to define immutable types and handle value-based equality. This matches the pattern used in other parts of ASP.NET Core, like the AuthenticationBuilder class, where the object holds configuration data (like IServiceCollection) and doesn’t change its internal state directly.

  2. Following .NET Patterns (e.g., AuthenticationBuilder):
    The AuthenticationBuilder is a great example from ASP.NET Core, where the builder encapsulates IServiceCollection and allows adding services via extension methods. Our approach to OpenFeatureBuilder follows the same design, where extension methods add specific configuration options, making the builder pattern more extensible without altering its core structure. Microsoft actively uses this builder pattern across many of their packages, not just for authentication. It's proven to be a robust way to add functionality in a clean and modular manner.

  3. Extension Methods Simplify Extensibility:
    Like how GoogleExtensions.cs and other authentication schemes are added via extension methods on AuthenticationBuilder, the same idea applies to OpenFeatureBuilder. This allows developers to extend the functionality of OpenFeatureBuilder in a modular way, adding new features without needing to modify or subclass the builder itself. This aligns with Microsoft’s general approach to keep core objects clean and extensible via extensions.

  4. Many Uses Across Microsoft Packages:
    While AuthenticationBuilder is a useful example, this builder and extension pattern is widely used throughout Microsoft’s packages, such as in DI (Dependency Injection), logging, and configuration. It allows developers to write clean, easily maintainable code that is open to extensions without modifying the core framework.

  5. Reconsidering the sealed Keyword:
    In some cases, the use of sealed on a builder might restrict extensibility via inheritance. Although the builder pattern usually focuses on extension methods rather than inheritance, removing sealed can be beneficial if you envision a need for developers to extend OpenFeatureBuilder through inheritance. It opens up the possibility for advanced users to create custom builders while still maintaining the base functionality.


In summary, the use of a record for OpenFeatureBuilder fits well with established patterns in .NET, like AuthenticationBuilder, making it immutable and easily extensible through extension methods. The use of this pattern is not isolated; it’s widespread across Microsoft packages and proves to be a useful approach. The consideration of removing sealed can further enhance the extensibility of the builder, allowing more advanced use cases where inheritance might be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove the sealed modifier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arttonoyan!
I am still not sure why we use the record.

Using a record in OpenFeatureBuilder ensures immutability, which is crucial in configurations where state changes should not happen after initialization.

If I am not looking at the wrong place, we do not have any data.
The AuthenticationBuilder is a class and not a record.

The AuthenticationBuilder also provides the basic functionality directly so maybe we would only need the extensions from src/OpenFeature.Hosting/OpenFeatureBuilderExtensions.cs but we could inline src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs as they are directly defined in the same package.

I have no hard opinion on this, I would just like to understand the reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in our case, there isn't a significant difference between using a class or a record. The main reason I opted for a record is because it allows for a more concise and compact syntax, especially when the class primarily holds immutable data, like the Services property. Functionally, it would be nearly identical to using a class like this:

public class OpenFeatureBuilder
{
    public OpenFeatureBuilder(IServiceCollection services)
    {
        Services = services;
    }

    public IServiceCollection Services { get; }
}

Copy link
Member

@lukas-reining lukas-reining Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially when the class primarily holds immutable data, like the Services property.

In our case we have one immutable property Services and one mutable IsContextConfigured.
This is not too important I guess but I am just wondering why we should do different then e.g. ASP.NET Core in your example.

As you said you removed sealed for extensibility, but I would argue that classes, as for AuthenticationBuilder, would be more idiomatic and extensible than extending a record as "a class can not inherit from a record" and the pattern for extending OpenFeature would be different than for the ASP.NET Core built-ins: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record#inheritance

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the use of record also caused confusion. I had to read a bit to understand why we'd use it in this case.

Personally, since other builders we have are implemented as classes I think I'd prefer that, but it's not a strong preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukas-reining @toddbaert Thank you for your thoughtful feedback! I completely understand your point, especially regarding consistency with other builders being implemented as classes. I can see how using a record in this case might cause some confusion. It’s not a strong preference for me either, I will change with the more familiar pattern of using classes for builders.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add, other reason why we should seal classes is in dotnet 6, we got some small performance improvements: https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-6/#“peanut-butter”

As for the record, I would prefer to start using it by the reasons that @arttonoyan mentioned before. But I am happy to keep as a class until we decide to move it to record.

/// This property is used to determine if specific configurations or services
/// should be initialized based on the presence of an evaluation context.
/// </summary>
internal bool IsContextConfigured { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure: why is this internal?
If our package benefits from IsContextConfigured could it be beneficial for external extensions too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! I initially made it internal because it was primarily used for internal configuration, and the public setter was intended for use within the internal scope. However, you raise a good point. There's no strong reason not to make it public, as it could provide useful information for consumers. We can certainly change it to:

public bool IsContextConfigured { get; internal set; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@toddbaert
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor Author

arttonoyan commented Oct 22, 2024

@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
Copy link
Member

askpt commented Oct 23, 2024

@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
Copy link
Contributor Author

@askpt to clarify, the intent was not to release a separate nuget artifact for these changes (we don't even have the publishing mechanisms in this repo to support that); the idea was just to add new functionality marked as experimental using the [Experimental] attribute to the current SDK. I hope that clarifies some of your namespace concerns.

@arttonoyan You should be able to annotate all the new classes or functions (basically anything a user would touch) like this:

#if !NET462
        [System.Diagnostics.CodeAnalysis.Experimental(Constants.NewDiFeatures)]
#endif

Where Constants.NewDiFeatures is a common string we use to identify all our experimental new DI stuff. Users can then disable the associated warning by ignoring this error for that diagnostic ID (meaning they won't get any warnings for using these new features)

Note that this attribute doesn't exist in .NET framework (462) so you need to add the prepreoceessor directive (#if !NET462) to disable it there; those users won't get the warning but .NET 462 users are probably a minority these days. Please also use the fully qualified attribute name (System.Diagnostics.CodeAnalysis.Experimental) as well (IMO that's a good habit for imports that are used conditionally).

Thanks, @toddbaert! I’m working on it, though it’s impacting quite a few classes. Another approach could be to build pre-release packages and mark the package itself as experimental, which aligns with NuGet's recommendations for experimental features: Pre-release Packages.

To make it clear for users, we could set the version <Version>1.0.0-experimental</Version>, marking it as pre-release on NuGet. Additionally, we could update the NuGet metadata to indicate the experimental status, for example:

<PackageReleaseNotes>This is an experimental version. Features may change without notice.</PackageReleaseNotes>
<Description>Experimental package for testing new features. Not recommended for production use.</Description>

This would help users recognize the package as experimental and avoid unintended use in production. What do you think?

@arttonoyan
Copy link
Contributor Author

@askpt to clarify, the intent was not to release a separate nuget artifact for these changes (we don't even have the publishing mechanisms in this repo to support that); the idea was just to add new functionality marked as experimental using the [Experimental] attribute to the current SDK. I hope that clarifies some of your namespace concerns.
@arttonoyan You should be able to annotate all the new classes or functions (basically anything a user would touch) like this:

#if !NET462
        [System.Diagnostics.CodeAnalysis.Experimental(Constants.NewDiFeatures)]
#endif

Where Constants.NewDiFeatures is a common string we use to identify all our experimental new DI stuff. Users can then disable the associated warning by ignoring this error for that diagnostic ID (meaning they won't get any warnings for using these new features)
Note that this attribute doesn't exist in .NET framework (462) so you need to add the prepreoceessor directive (#if !NET462) to disable it there; those users won't get the warning but .NET 462 users are probably a minority these days. Please also use the fully qualified attribute name (System.Diagnostics.CodeAnalysis.Experimental) as well (IMO that's a good habit for imports that are used conditionally).

Thanks, @toddbaert! I’m working on it, though it’s impacting quite a few classes. Another approach could be to build pre-release packages and mark the package itself as experimental, which aligns with NuGet's recommendations for experimental features: Pre-release Packages.

To make it clear for users, we could set the version <Version>1.0.0-experimental</Version>, marking it as pre-release on NuGet. Additionally, we could update the NuGet metadata to indicate the experimental status, for example:

<PackageReleaseNotes>This is an experimental version. Features may change without notice.</PackageReleaseNotes>
<Description>Experimental package for testing new features. Not recommended for production use.</Description>

This would help users recognize the package as experimental and avoid unintended use in production. What do you think?

@toddbaert I've created a new PR to add the Experimental attribute conditionally for .NET 8 and above, specifically for new DI features.

Key Points:

  • The attribute is applied conditionally using #if NET8_0_OR_GREATER, so it only works for .NET 8 and greater.
  • For .NET Framework 4.6.2 (or other frameworks that don’t match this condition), the attribute will not be applied. The conditional check #if !NET462 does not function as intended for targeting non-.NET 8 frameworks in this scenario.

I’ve created this change in a separate branch, as I think marking the package as experimental is more convenient, as mentioned above. However, the PR is open, and I'm open to feedback – please feel free to decide if this is the best approach for this package.
arttonoyan#2

@toddbaert
Copy link
Member

toddbaert commented Nov 4, 2024

@arttonoyan The problem is we dont want to mark the entire SDK as experimental, and these features will be included in all subsequent releases, so I don't think an experimental version number can help, unless we want to merge these features to a non-main branch and maintain that along side the main branch (I'm not in favor of this for the maintenance burden it creates).

If you can add PackageReleaseNotes to the new .csproj files, that's fine, but I would say "Features may change without notice" and avoid not recommending for production. I think we want to consider this features "production grade"; just changable. I also think that perhaps we should add the Experimental attibute to any of the DI-related classes just in the OpenFeature namespace, since it might not be clear they are coming from the experimental packages... we can avoid it for classes/artifacts in the new experimentally-marked packages/namespaces... does that make sense?

@arttonoyan
Copy link
Contributor Author

@toddbaert, @beeme1mr, @askpt and team!
I’ve submitted a PR to propose a structured approach for Diagnostic Feature Codes in OpenFeature, which aims to standardize the way we track and manage experimental features.
arttonoyan#3

This PR is a starting point and includes a proposed code format [Prefix][Domain][UniqueNumber] (e.g., OFDI001) that could serve as the foundation for future enhancements. I’ve referenced best practices from OpenTelemetry and EF Core, and I’m hoping it sparks a conversation on how we can formalize diagnostics within OpenFeature.

I know it’s a bit lengthy, but I wanted to make sure it’s thoughtful and not just rushed to completion. I’d appreciate it if you could take a few minutes to review the PR and share any thoughts or suggestions. Feedback on the code format, potential improvements, or alignment with OpenFeature’s objectives would be especially helpful.

@beeme1mr
Copy link
Member

beeme1mr commented Nov 5, 2024

Amazing, thanks. I'll take a look ASAP.

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you. As a follow-up, could you please update the readme to include a section on how to configure DI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great addition.

@arttonoyan
Copy link
Contributor Author

@beeme1mr, @toddbaert, @askpt Team, I've updated the README file - please take a look when you have a moment. I've also added a new Experimental type with an icon (open to feedback on this, or we can use a predefined one).

@toddbaert toddbaert self-requested a review November 13, 2024 18:32
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now. The packaging also seems to work well based on the CI test packaging, and since the experimental features here are in their own packages, I have no objections to releasing as-is.

Apart from my approval, you'll have to sign off your commits as described in the failing DCO check here. Alternatively, you can squash all this down into one commit and sign that. Either way is fine with me but this is a policy enforced by our parent organize, the CNCF.

I will merge this in the next couple days unless I hear objections from @kinyoklion @askpt @lukas-reining or @thomaspoignant

/// <summary>
/// This method is used to add a new context to the service collection.
/// </summary>
/// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance.</param>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It would be nice to keep the style of parameter documentation consistent. I think that the prevalent style is currently fragments instead of sentences.

/// </summary>
/// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance.</param>
/// <param name="configure">the desired configuration</param>
/// <returns>The <see cref="OpenFeatureBuilder"/> instance.</returns>
Copy link
Member

@kinyoklion kinyoklion Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have exception documentation for the associated guards? (same comment applies to multiple functions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 46.69421% with 129 lines in your changes missing coverage. Please review.

Project coverage is 85.75%. Comparing base (ccf0250) to head (dd3dc33).

Files with missing lines Patch % Lines
...ependencyInjection/OpenFeatureBuilderExtensions.cs 43.51% 71 Missing and 3 partials ⚠️
...njection/OpenFeatureServiceCollectionExtensions.cs 54.54% 14 Missing and 1 partial ⚠️
...dencyInjection/Internal/FeatureLifecycleManager.cs 62.50% 8 Missing and 1 partial ⚠️
...ction/Providers/Memory/FeatureBuilderExtensions.cs 0.00% 9 Missing ⚠️
...nFeature.DependencyInjection/OpenFeatureOptions.cs 58.82% 6 Missing and 1 partial ⚠️
src/OpenFeature.DependencyInjection/Guard.cs 25.00% 5 Missing and 1 partial ⚠️
...nFeature.DependencyInjection/OpenFeatureBuilder.cs 64.70% 4 Missing and 2 partials ⚠️
...ection/Providers/Memory/InMemoryProviderFactory.cs 0.00% 2 Missing ⚠️
...enFeature.DependencyInjection/PolicyNameOptions.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   94.19%   85.75%   -8.45%     
==========================================
  Files          25       34       +9     
  Lines        1120     1362     +242     
  Branches      123      147      +24     
==========================================
+ Hits         1055     1168     +113     
- Misses         42      162     +120     
- Partials       23       32       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@beeme1mr
Copy link
Member

Hey @arttonoyan, could you please sign off on these commits? It's a requirement from the CNCF.

  • In your local branch, run: git rebase HEAD~139 --signoff
  • Force push your changes to overwrite the branch: git push --force-with-lease origin add-dependency-injection

@arttonoyan
Copy link
Contributor Author

arttonoyan commented Nov 17, 2024

Guys, I believe I may be doing something wrong with the git rebase HEAD~139 --signoff command. I followed the suggested steps, but the result seems to include more changes than I expected. Could someone guide me on what might be going wrong? Any help would be greatly appreciated!

@beeme1mr
Copy link
Member

beeme1mr commented Nov 17, 2024

Guys, I believe I may be doing something wrong with the git rebase HEAD~139 --signoff command. I followed the suggested steps, but the result seems to include more changes than I expected. Could someone guide me on what might be going wrong? Any help would be greatly appreciated!

We can override this check and merge tomorrow. Your comment clearly indicates that you're willing to contribute to the CNCF.

Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
@toddbaert toddbaert force-pushed the add-dependency-injection branch from dd3dc33 to 0054f91 Compare November 18, 2024 14:10
@toddbaert
Copy link
Member

Hey @arttonoyan - I've cleaned up your branch and squashed some commits - I've changed no actual contents since your last changes though. Signoff now looks good. Thanks.

@toddbaert toddbaert merged commit 498c977 into open-feature:main Nov 18, 2024
14 of 18 checks passed
@askpt
Copy link
Member

askpt commented Nov 18, 2024

Hey @arttonoyan - I've cleaned up your branch and squashed some commits - I've changed no actual contents since your last changes though. Signoff now looks good. Thanks.

Thanks, @toddbaert and @arttonoyan, for this effort! Glad to see this out! 👍

@arttonoyan
Copy link
Contributor Author

Hey @arttonoyan - I've cleaned up your branch and squashed some commits - I've changed no actual contents since your last changes though. Signoff now looks good. Thanks.

@toddbaert Thank you for cleaning up the branch! I really appreciate the effort. This has been a valuable experience for me. I'll integrate the new package into our code and, after running some tests, continue working on enhancements to improve it further.

toddbaert pushed a commit that referenced this pull request Nov 18, 2024
)

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

- **OpenFeature.DependencyInjection Project:**
- Implemented `OpenFeatureBuilder`, including
`OpenFeatureBuilderExtensions` for seamless integration.
  - Added `IFeatureLifecycleManager` interface and its implementation.
- Introduced `AddProvider` extension method for easy provider
configuration.
- Created `OpenFeatureServiceCollectionExtensions` for service
registration.

- **OpenFeature.Hosting Project:**
- Added `HostedFeatureLifecycleService` to manage the lifecycle of
feature providers in hosted environments.

- **Testing Enhancements:**
- Created unit tests for critical methods, including
`OpenFeatureBuilderExtensionsTests` and
`OpenFeatureServiceCollectionExtensionsTests`.
- Replicated and tested `NoOpFeatureProvider` implementation for better
test coverage.

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

Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
askpt pushed a commit that referenced this pull request Nov 18, 2024
)

> [!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:

- **OpenFeature.DependencyInjection Project:**
- Implemented `OpenFeatureBuilder`, including
`OpenFeatureBuilderExtensions` for seamless integration.
  - Added `IFeatureLifecycleManager` interface and its implementation.
- Introduced `AddProvider` extension method for easy provider
configuration.
- Created `OpenFeatureServiceCollectionExtensions` for service
registration.

- **OpenFeature.Hosting Project:**
- Added `HostedFeatureLifecycleService` to manage the lifecycle of
feature providers in hosted environments.

- **Testing Enhancements:**
- Created unit tests for critical methods, including
`OpenFeatureBuilderExtensionsTests` and
`OpenFeatureServiceCollectionExtensionsTests`.
- Replicated and tested `NoOpFeatureProvider` implementation for better
test coverage.

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

---

**NuGet Packages for installation:**
```bash
dotnet add package OpenFeature
dotnet add package OpenFeature.DependencyInjection
dotnet add package OpenFeature.Hosting
```

---

**Usage Example:**

```csharp
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)));
});
```

Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>

# Conflicts:
#	Directory.Packages.props
askpt pushed a commit that referenced this pull request Nov 18, 2024
)

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

- **OpenFeature.DependencyInjection Project:**
- Implemented `OpenFeatureBuilder`, including
`OpenFeatureBuilderExtensions` for seamless integration.
  - Added `IFeatureLifecycleManager` interface and its implementation.
- Introduced `AddProvider` extension method for easy provider
configuration.
- Created `OpenFeatureServiceCollectionExtensions` for service
registration.

- **OpenFeature.Hosting Project:**
- Added `HostedFeatureLifecycleService` to manage the lifecycle of
feature providers in hosted environments.

- **Testing Enhancements:**
- Created unit tests for critical methods, including
`OpenFeatureBuilderExtensionsTests` and
`OpenFeatureServiceCollectionExtensionsTests`.
- Replicated and tested `NoOpFeatureProvider` implementation for better
test coverage.

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

Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
toddbaert pushed a commit that referenced this pull request Nov 18, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.1.0](v2.0.0...v2.1.0)
(2024-11-18)


### 🐛 Bug Fixes

* Fix action syntax in workflow configuration
([#315](#315))
([ccf0250](ccf0250))
* Fix unit test clean context
([#313](#313))
([3038142](3038142))


### ✨ New Features

* Add Dependency Injection and Hosting support for OpenFeature
([#310](#310))
([1aaa0ec](1aaa0ec))


### 🧹 Chore

* **deps:** update actions/upload-artifact action to v4.4.3
([#292](#292))
([9b693f7](9b693f7))
* **deps:** update codecov/codecov-action action to v4.6.0
([#306](#306))
([4b92528](4b92528))
* **deps:** update dependency dotnet-sdk to v8.0.401
([#296](#296))
([0bae29d](0bae29d))
* **deps:** update dependency fluentassertions to 6.12.2
([#302](#302))
([bc7e187](bc7e187))
* **deps:** update dependency microsoft.net.test.sdk to 17.11.0
([#297](#297))
([5593e19](5593e19))
* **deps:** update dependency microsoft.net.test.sdk to 17.11.1
([#301](#301))
([5b979d2](5b979d2))
* **deps:** update dependency nsubstitute to 5.3.0
([#311](#311))
([87f9cfa](87f9cfa))
* **deps:** update dependency xunit to 2.9.2
([#303](#303))
([2273948](2273948))
* **deps:** update dotnet monorepo
([#305](#305))
([3955b16](3955b16))
* **deps:** update dotnet monorepo to 8.0.2
([#319](#319))
([94681f3](94681f3))
* update release please config
([#304](#304))
([c471c06](c471c06))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
askpt pushed a commit that referenced this pull request Dec 4, 2024
)

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

- **OpenFeature.DependencyInjection Project:**
- Implemented `OpenFeatureBuilder`, including
`OpenFeatureBuilderExtensions` for seamless integration.
  - Added `IFeatureLifecycleManager` interface and its implementation.
- Introduced `AddProvider` extension method for easy provider
configuration.
- Created `OpenFeatureServiceCollectionExtensions` for service
registration.

- **OpenFeature.Hosting Project:**
- Added `HostedFeatureLifecycleService` to manage the lifecycle of
feature providers in hosted environments.

- **Testing Enhancements:**
- Created unit tests for critical methods, including
`OpenFeatureBuilderExtensionsTests` and
`OpenFeatureServiceCollectionExtensionsTests`.
- Replicated and tested `NoOpFeatureProvider` implementation for better
test coverage.

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

Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
askpt pushed a commit that referenced this pull request Dec 4, 2024
🤖 I have created a release *beep* *boop*
---

##
[2.1.0](v2.0.0...v2.1.0)
(2024-11-18)

### 🐛 Bug Fixes

* Fix action syntax in workflow configuration
([#315](#315))
([ccf0250](ccf0250))
* Fix unit test clean context
([#313](#313))
([3038142](3038142))

### ✨ New Features

* Add Dependency Injection and Hosting support for OpenFeature
([#310](#310))
([1aaa0ec](1aaa0ec))

### 🧹 Chore

* **deps:** update actions/upload-artifact action to v4.4.3
([#292](#292))
([9b693f7](9b693f7))
* **deps:** update codecov/codecov-action action to v4.6.0
([#306](#306))
([4b92528](4b92528))
* **deps:** update dependency dotnet-sdk to v8.0.401
([#296](#296))
([0bae29d](0bae29d))
* **deps:** update dependency fluentassertions to 6.12.2
([#302](#302))
([bc7e187](bc7e187))
* **deps:** update dependency microsoft.net.test.sdk to 17.11.0
([#297](#297))
([5593e19](5593e19))
* **deps:** update dependency microsoft.net.test.sdk to 17.11.1
([#301](#301))
([5b979d2](5b979d2))
* **deps:** update dependency nsubstitute to 5.3.0
([#311](#311))
([87f9cfa](87f9cfa))
* **deps:** update dependency xunit to 2.9.2
([#303](#303))
([2273948](2273948))
* **deps:** update dotnet monorepo
([#305](#305))
([3955b16](3955b16))
* **deps:** update dotnet monorepo to 8.0.2
([#319](#319))
([94681f3](94681f3))
* update release please config
([#304](#304))
([c471c06](c471c06))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce OpenFeature.Extensions.Hosting package
7 participants