feat: Add initial dependency injection support for LaunchDarkly provider #49
Conversation
Introduce a new project for LaunchDarkly OpenFeature dependency injection, including configuration for target frameworks, versioning, and package references. Add extension methods to `OpenFeatureBuilder` for registering the LaunchDarkly provider as both a default and domain-scoped feature provider, enhancing flexibility in configuration.
Updated the solution to Visual Studio 17 and added two new projects for dependency injection and its tests. Introduced integration tests for the `AddLaunchDarkly` method, ensuring correct behavior in various scenarios. Created a project file for the new tests with necessary dependencies. Added unit tests to validate edge cases for the provider registration.
Updated project and test files to rename methods from `AddLaunchDarkly` to `UseLaunchDarkly` for clarity and improved configuration handling. Added new properties in the project file for editor support and debug type. Cleaned up dependencies in the test project and adjusted tests to validate the new method signatures and behaviors.
Refactor `DependencyInjectionIntegrationTests.cs` to replace instances of `FeatureClient` with `IFeatureClient`. This change improves adherence to dependency inversion principles, enhancing abstraction and flexibility in testing and implementation.
…tegration - Renamed `OpenFeatureBuilderExtensions` to `LaunchDarklyOpenFeatureBuilderExtensions` for clarity. - Updated XML documentation to better describe method functionalities and parameters. - Modified `UseLaunchDarkly` method signatures to accept a `Configuration` object, streamlining configuration. - Added overloads for domain-scoped configurations to improve feature flag management. - Refined internal methods for clarity and ensured configuration validation occurs before registration. - Removed `EnsureValidConfiguration` method, integrating its functionality into the configuration creation process. - Enhanced `CreateConfiguration` method to accept existing `Configuration` instances for improved flexibility.
The comments in the `RegisterLaunchDarklyProvider` method were updated to better explain the purpose of early configuration validation, emphasizing its role in preventing runtime failures by ensuring correct provider construction during setup. Additionally, comments were revised in the domain-scoped configuration registration to highlight that the same early validation strategy applies, allowing for quick identification of misconfigurations.
Removed `DependencyInjectionIntegrationTests.cs` and `OpenFeatureBuilderExtensionsTests.cs`. Added `LaunchDarklyDependencyInjectionIntegrationTests.cs` and `LaunchDarklyOpenFeatureBuilderExtensionsTests.cs` to enhance test coverage and organization for LaunchDarkly integration with OpenFeature.
The entire `LaunchDarklyDependencyInjectionIntegrationTests.cs` file has been removed, including all its using directives, namespace declaration, and the class definition along with all its methods and tests.
The project file `LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.csproj` now targets only the `net8.0` framework, removing support for `netstandard2.0`, `net471`, and `net6.0`. The version range for the `OpenFeature.DependencyInjection` package reference was changed to `[2.2.0, 3.0.0)`. Conditional compilation directives were added in `LaunchDarklyOpenFeatureBuilderExtensions.cs` for .NET 8.0 or greater. The test project `LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.Tests.csproj` was also updated to target `net8.0`, with the addition of `GenerateMSBuildEditorConfigFile` set to `false`. Tests in `LaunchDarklyOpenFeatureBuilderExtensionsTests.cs` were refactored to use `Theory` and `InlineData` for parameterized testing, improving exception handling and validation logic. Overall, these changes streamline the codebase and enhance test robustness.
Add conditional compilation directives for .NET 8.0 or greater in the LaunchDarklyOpenFeatureBuilderExtensions and corresponding test files. This ensures that specific class definitions and methods are included only when targeting .NET 8.0 or higher.
Renamed test methods for consistency and clarity in the `LaunchDarklyOpenFeatureBuilderExtensionsTests` class. Added new tests to verify behavior of the `UseLaunchDarkly` method with various configurations, including singleton registration and handling of null or whitespace SDK keys. Improved overall structure for better maintainability.
- Added `Microsoft.Extensions.Options` namespace for options pattern support. - Updated comments for clarity on early configuration validation. - Simplified singleton configuration registration by passing the configuration object directly. - Enhanced clarity and consistency in domain-scoped configuration registration. - Introduced a default configuration provider for flexible resolution based on name selection policy. - Overall improvements to robustness and clarity in OpenFeature integration with LaunchDarkly.
Updated `LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.Tests.csproj` to include new package references for `Microsoft.Extensions.Logging` and `Microsoft.Extensions.DependencyInjection` at version `8.0.1`. Added a new test class `LaunchDarklyIntegrationTests` in `LaunchDarklyIntegrationTests.cs` with multiple integration tests covering configuration overloads, SDK key overloads, multi-provider setups, and resource management. Introduced helper methods to enhance test structure and reusability.
Removed the block that adds context to the OpenFeature builder, specifically the targeting key setup. This change may impact how targeting is handled in the application.
Refactor methods for configuring the LaunchDarkly feature provider in the OpenFeature library. Updated method names for clarity, replacing `CreateServiceProviderWithDefaultLaunchDarklyAsync` with `ConfigureLaunchDarklyAsync`. Changed return types from `Task` to `ValueTask` for performance improvements. Enhanced comments and XML documentation for better understanding. Updated tests to ensure they validate the new configurations correctly.
…tion Update test cases to include `validateScopes: true` in `BuildServiceProvider()` calls. This change ensures early validation of service configurations, allowing configuration issues to be caught immediately during registration. Enhances the robustness of the tests across multiple methods.
Refactor the `registerWithThrowing` function from a lambda expression to a method declaration for improved clarity. The change maintains the same functionality of asserting that an `InvalidOperationException` is thrown during the registration process with the `OpenFeatureBuilder` instance.
|
Thanks for submitting this @arttonoyan, I will try and make some time in the next week or two to review this. |
Refactor the `UseLaunchDarkly` methods to include null checks for the `configuration` parameter, throwing an `ArgumentNullException` when it is null. Remove the unnecessary `CreateConfiguration` method. Update related tests to reflect the change in exception type from `NullReferenceException` to `ArgumentNullException`.
|
Hi @jsonbailey, just following up on this PR. I saw your note about reviewing when you had time - whenever you get a chance, could you take another look? Thanks in advance for your time and feedback! |
|
@arttonoyan I haven't forgot about it and its still in my task list, a couple items took longer than expected. I will try to get to it this week or early next week. |
...y.OpenFeature.ServerProvider.DependencyInjection/LaunchDarklyOpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
…n/LaunchDarklyOpenFeatureBuilderExtensions.cs Co-authored-by: Waldek <[email protected]>
Renamed the parameter `stdKey` to `sdkKey` in multiple method signatures and XML documentation comments to clarify its purpose and align with LaunchDarkly SDK terminology. Affected methods include `UseLaunchDarkly` for both default and domain-scoped providers, as well as the `CreateConfiguration` method. The internal logic remains unchanged, preserving functionality while improving code clarity.
Renamed `RegisterLaunchDarklyProviderForDomain` to `RegisterLaunchDarklyProvider`. Updated its usage in `UseLaunchDarkly` and null configuration checks. The method parameters now include a `Func<Configuration>` for creating configurations and a `Func<IServiceProvider, object, Configuration>` for resolving them, enhancing configuration management flexibility.
Refactor the `UseLaunchDarkly` method to format the parameters of the `RegisterLaunchDarklyProvider` call on separate lines. This change enhances readability while maintaining the existing functionality.
| var config = createConfiguration(); | ||
| builder.Services.TryAddSingleton(config); | ||
|
|
||
| return builder.AddProvider(serviceProvider => new Provider(resolveConfiguration(serviceProvider))); |
There was a problem hiding this comment.
AddProvider registers a transient factory method. I don't believe we should be resolving transient instances of Provider here. Please correct me if I am wrong, but my understanding is that LaunchDarkly strongly recommends instances of LdClient to be singletons (for a given sdk-key/environment).
Provider encapsulates the construction of the LdClient within its own constructor and thus, its lifetime should follow the same guideline. I believe a more correct approach would be to register Provider as a singleton and resolve it using DI as part of the factory method. e.g.
builder.Services.AddSingleton<Provider>(serviceProvider => {
...
return new Provider(config);
});
builder.AddProvider(serviceProvider => serviceProvider.GetRequiredService<Provider>());
I'm assuming OpenFeature uses a transient factory method so that they don't dictate lifetime to provider implementations. Instead, each provider can be registered per their own guidance.
There was a problem hiding this comment.
You’re absolutely right that LdClient is recommended to be used as a singleton. However, in this case, the factory method is registered as transient because its lifecycle is ultimately governed by the client that consumes it.
In OpenFeature, the initialization happens here, and the _featureApi is registered as a singleton. Consumers don’t interact with providers directly - they use IFeatureClient, which goes through this API (singleton). Given that flow, the effective usage aligns with the singleton recommendation.
Registering the provider as transient also leaves flexibility: consumers can create and manage their own provider instance if they have specific scenarios requiring different lifetimes. This avoids OpenFeature itself enforcing a single pattern and instead allows SDK consumers to decide. In the standard usage (via OpenFeature and IFeatureClient), it will still behave as a singleton, which follows LaunchDarkly’s guidance.
That said, if the LaunchDarkly team concludes that Provider must always be a singleton for correctness, we can adapt by layering a singleton registration on top of the transient one. For example:
builder.AddProvider(serviceProvider => new Provider(resolveConfiguration(serviceProvider)));
builder.Services.AddSingleton(serviceProvider => new Provider(resolveConfiguration(serviceProvider)));Here AddProvider wires up the necessary OpenFeature services internally (via TryAdd), and the explicit AddSingleton overrides the provider’s registration.
My recommendation is to keep it as-is (registered as transient), for the reasons I outlined above. That said, this is an excellent highlight, and I fully agree it’s an important consideration that we should call out explicitly in the documentation.
There was a problem hiding this comment.
@dgioulakis what do you think?
There was a problem hiding this comment.
Hey @arttonoyan, sorry for the delay. I was out for a bit, and then came down with covid and was out some more.
I see what you're saying, although I'm not a maintainer of this repository. It would be best to get someone from LD to review. I think your second AddSingleton will not overwrite as you intend unless you specify the service type e.g. builder.Services.AddSingleton<FeatureProvider>(serviceProvider => new Provider(...)).
There was a problem hiding this comment.
The LaunchDarkly Client should be a singleton but I think the approach to let OpenFeature dictate the provider lifetime here is okay. Especially since it in effect becomes a singleton.
There was a problem hiding this comment.
I noticed that OpenFeature has deprecated their DI library in favor of the Hosting library which contains the DI functionality. We should update the PR to target the new package. If you'd prefer, I can also update the PR to target the new package but I wanted to give you a chance first since its your PR.
| var config = createConfiguration(); | ||
| builder.Services.TryAddSingleton(config); | ||
|
|
||
| return builder.AddProvider(serviceProvider => new Provider(resolveConfiguration(serviceProvider))); |
There was a problem hiding this comment.
The LaunchDarkly Client should be a singleton but I think the approach to let OpenFeature dictate the provider lifetime here is okay. Especially since it in effect becomes a singleton.
| /// <param name="createConfiguration">A delegate that returns a domain-specific <see cref="Configuration"/> instance.</param> | ||
| /// <param name="resolveConfiguration">A delegate that resolves the domain-scoped <see cref="Configuration"/> from the service provider.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| private static OpenFeatureBuilder RegisterLaunchDarklyProvider( |
There was a problem hiding this comment.
I don't follow the benefit of adding the config to the ServiceCollection. If we remove that, this could be simplified. Can you share the intent behind storing it?
| private static OpenFeatureBuilder RegisterLaunchDarklyProvider( | |
| private static OpenFeatureBuilder RegisterLaunchDarklyProvider( | |
| OpenFeatureBuilder builder, | |
| string domain, | |
| Func<Configuration> createConfiguration) | |
| { | |
| // Still do early validation to fail fast | |
| var config = createConfiguration(); | |
| // Just register the provider directly with its config | |
| return builder.AddProvider(domain, (_, __) => new Provider(config)); | |
| } |
There was a problem hiding this comment.
I'm not sure why the "Apply suggestion" messed up, it should be a replacement for the whole method, not just added in. I mainly wanted to include it as a discussion point about storing the config.
Thanks for the note - you’re right, That said, from a provider’s point of view (e.g., LaunchDarkly), pulling in In the meantime, I’m fine proceeding either way:
I’ll start a discussion upstream and report back here with any guidance we get. |
|
You mentioned having a working solution internally, do you want to hold off on this until we here back from the community before we switch this to the hosting package? Also, do you have any thoughts about the config being added to the service collection? It's possible I'm just not understanding the intent but I wanted to discuss it before you or I make any changes. |
Yes - let’s hold off. I’ve opened the PR for discussion #587 and pinged the OpenFeature team in Slack. I’d like to wait a couple of days to gather community feedback before switching this to the Hosting package. On the config being added to the service collection: my initial view is that registering config via DI improves discoverability, testability, and alignment with the Options pattern, especially for scenarios like multi-provider setups, named clients, and environment overrides. I’ll review a few edge cases and come back with a concrete proposal shortly. |
@jsonbailey I’ve discussed this in Slack, and the OpenFeature team supports the approach. I’ll take ownership and implement it shortly. Let’s hold this PR until the new abstractions are ready; I’ll then return and update this PR accordingly. |
|
@jsonbailey Here’s the open PR to track progress: open-feature/dotnet-sdk#596 |
4 participants
Requirements
Related issues
Describe the solution you've provided
This PR implements initial dependency injection (DI) support for the LaunchDarkly provider in the OpenFeature .NET SDK:
Adds
.UseLaunchDarkly(...)extension to enable DI registration in .NET 8.0.Registers
ConfigurationandIFeatureClientas scoped services, consistent with OpenFeature’s default behavior - when OpenFeature changes its default service lifetime, it will automatically apply here, preserving correct DI semantics.Supports both default and domain-scoped provider configurations, with domain-scoped registrations via keyed DI.
Introduces comprehensive tests verifying:
.Offline(true)) behavior.IFeatureClient.Describe alternatives you've considered
Additional context