-
Notifications
You must be signed in to change notification settings - Fork 21
feat: Add multi-provider support #488
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
base: main
Are you sure you want to change the base?
Conversation
…ure resolution Signed-off-by: André Silva <[email protected]>
…provider feature flag evaluation Signed-off-by: André Silva <[email protected]>
…ng evaluation strategy Signed-off-by: André Silva <[email protected]>
…Strategy classes for feature flag evaluation Signed-off-by: André Silva <[email protected]>
…pecific feature resolution Signed-off-by: André Silva <[email protected]>
…esolution Signed-off-by: André Silva <[email protected]>
…ulti-type feature resolution Signed-off-by: André Silva <[email protected]>
…hod for improved multi-provider support Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…gy to enhance multi-provider support Signed-off-by: André Silva <[email protected]>
…ovider evaluation logic Signed-off-by: André Silva <[email protected]>
…date multi-provider functionality Signed-off-by: André Silva <[email protected]>
…strategy delegation Signed-off-by: André Silva <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #488 +/- ##
==========================================
+ Coverage 88.45% 89.60% +1.15%
==========================================
Files 50 64 +14
Lines 2096 2492 +396
Branches 245 295 +50
==========================================
+ Hits 1854 2233 +379
- Misses 191 199 +8
- Partials 51 60 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: André Silva <[email protected]>
…iders Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Do you have any idea when this will be implemented in the NuGet package? |
@sergheevxo I am actively working on #338, and this feature is next on my list. |
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…h fallback and mismatch handling Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…ed clarity and consistency Signed-off-by: André Silva <[email protected]>
… to OpenFeature.Providers.MultiProvider Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
src/OpenFeature/Providers/MultiProvider/Strategies/ComparisonStrategy.cs
Show resolved
Hide resolved
src/OpenFeature/Providers/MultiProvider/Strategies/FirstMatchStrategy.cs
Show resolved
Hide resolved
src/OpenFeature/Providers/MultiProvider/Strategies/FirstMatchStrategy.cs
Show resolved
Hide resolved
src/OpenFeature/Providers/MultiProvider/Strategies/ComparisonStrategy.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature/Providers/MultiProvider/Strategies/FirstSuccessfulStrategy.cs
Show resolved
Hide resolved
src/OpenFeature/Providers/MultiProvider/Strategies/Models/StrategyEvaluationContext.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature/Providers/MultiProvider/Strategies/Models/StrategyPerProviderContext.cs
Outdated
Show resolved
Hide resolved
…-only accessors Signed-off-by: André Silva <[email protected]>
…ulStrategy to clarify provider evaluation order Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…der strategies Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]> <!-- Please use this template for your pull request. --> <!-- Please use the sections that you need and delete other sections --> ## This PR <!-- add the description of the PR here --> This pull request introduces enhancements to the `Value` class in the `OpenFeature.Model` namespace, ensuring better equality handling, and updates dependencies to include `Microsoft.Bcl.HashCode`. The most significant changes include implementing equality comparison for `Value`, adding hash code generation, and updating project files to include the new dependency. ### Enhancements to `Value` class: * [`src/OpenFeature/Model/Value.cs`](diffhunk://#diff-336aacd3c42458899187108a2064648ae21e439b2b11e6ca7f25b7b7fef00609L9-R9): The `Value` class now implements `IEquatable<Value>` and includes methods for equality comparison (`Equals`, `==`, `!=`), hash code generation (`GetHashCode`), and internal helpers for comparing complex types like structures and lists. This ensures more robust and consistent equality checks. [[1]](diffhunk://#diff-336aacd3c42458899187108a2064648ae21e439b2b11e6ca7f25b7b7fef00609L9-R9) [[2]](diffhunk://#diff-336aacd3c42458899187108a2064648ae21e439b2b11e6ca7f25b7b7fef00609R187-R378) ### Dependency updates: * [`Directory.Packages.props`](diffhunk://#diff-5baf5f9e448ad54ab25a091adee0da05d4d228481c9200518fcb1b53a65d4156L13-R25): Added `Microsoft.Bcl.HashCode` as a dependency for hash code generation. Other package references were reformatted for consistency. * [`src/OpenFeature/OpenFeature.csproj`](diffhunk://#diff-711ea17cbdebe419375c7684c8c39a1423d2bebcf8976ddd7bdd78deaab65b21R11): Included `Microsoft.Bcl.HashCode` for specific target frameworks (`net462` and `netstandard2.0`). ### Notes <!-- any additional notes for this PR --> This implementation is necessary for the comparison in the MultiProvider. See: #488 (comment) --------- Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the premature approval - I need to take a closer look at the changes before this can move forward. I’ll complete my review in the next couple of days
try | ||
{ | ||
await rp.Provider.InitializeAsync(context, cancellationToken).ConfigureAwait(false); | ||
rp.SetStatus(Constant.ProviderStatus.Ready); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this thread safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InitializeAsync
and ShutdownAsync
iterate through _registeredProviders
and update each provider’s status concurrently. Each RegisteredProvider
instance’s Status
property is mutated inside Task.WhenAll
without synchronization
Updating shared mutable state in parallel can lead to data races if the Status
property is not thread‑safe. Either update statuses sequentially, protect them with synchronization (e.g., locks) or redesign RegisteredProvider
to be immutable and return a new instance with the updated status.
To ensure we don’t run into issues here, I would suggest consider adding an integration test that validates the concurrent status updates behave correctly. This will help verify thread-safety and prevent potential data races.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@askpt this was interesting to me so I played around with it a bit.
I’m sharing what I wrote so we don’t lose the code - feel free to improve it and reuse if it’s helpful!
Also, just an idea: it might be useful to add an internal GetRegisteredProviders()
method in MultiProvider
to make testing easier. Maybe even a public one if it makes sense longer term.
Here’s the test code I came up with:
[Fact]
public async Task MultiProvider_ConcurrentInitializationAndShutdown_ShouldMaintainConsistentProviderStatus()
{
// Arrange
const int providerCount = 20;
var random = new Random();
var providerEntries = new List<ProviderEntry>();
for (int i = 0; i < providerCount; i++)
{
var provider = Substitute.For<FeatureProvider>();
provider.InitializeAsync(Arg.Any<EvaluationContext>(), Arg.Any<CancellationToken>())
.Returns(Task.CompletedTask);
provider.ShutdownAsync(Arg.Any<CancellationToken>())
.Returns(Task.CompletedTask);
provider.GetMetadata()
.Returns(new Metadata(name: $"provider-{i}"));
providerEntries.Add(new ProviderEntry(provider));
}
var multiProvider = new MultiProvider(providerEntries);
// Act: simulate concurrent initialization and shutdown with one task each
var initTasks = Enumerable.Range(0, 1).Select(_ =>
Task.Run(() => multiProvider.InitializeAsync(Arg.Any<EvaluationContext>(), CancellationToken.None)));
var shutdownTasks = Enumerable.Range(0, 1).Select(_ =>
Task.Run(() => multiProvider.ShutdownAsync(CancellationToken.None)));
await Task.WhenAll(initTasks.Concat(shutdownTasks));
// Assert: ensure that each provider ends in a valid lifecycle state
var statuses = GetRegisteredStatuses().ToList();
Assert.All(statuses, status =>
{
Assert.True(
status is Constant.ProviderStatus.Ready or Constant.ProviderStatus.NotReady,
$"Unexpected provider status: {status}");
});
// Local helper: uses reflection to access the private '_registeredProviders' field
// and retrieve the current status of each registered provider.
// Consider replacing this with an internal or public method if testing becomes more frequent.
IEnumerable<Constant.ProviderStatus> GetRegisteredStatuses()
{
var field = typeof(MultiProvider).GetField("_registeredProviders", BindingFlags.NonPublic | BindingFlags.Instance);
if (field?.GetValue(multiProvider) is not IEnumerable<object> list)
throw new InvalidOperationException("Could not retrieve registered providers via reflection.");
foreach (var p in list)
{
var statusProperty = p.GetType().GetProperty("Status", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
if (statusProperty == null)
throw new InvalidOperationException($"'Status' property not found on type {p.GetType().Name}.");
if (statusProperty.GetValue(p) is not Constant.ProviderStatus status)
throw new InvalidOperationException("Unable to read status property value.");
yield return status;
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with: 4df6c76
Please let me know if I overdid the implementation of a thread safety for the initialisation and shutdown. I added a lock to the ChildProviderStatus
and a semaphore for the lifecycle methods.
defaultValue, | ||
ErrorType.General, | ||
Reason.Error, | ||
errorMessage: ex.Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use ex.ToString()
?
src/OpenFeature/Providers/MultiProvider/Models/ProviderStatus.cs
Outdated
Show resolved
Hide resolved
try | ||
{ | ||
await rp.Provider.InitializeAsync(context, cancellationToken).ConfigureAwait(false); | ||
rp.SetStatus(Constant.ProviderStatus.Ready); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InitializeAsync
and ShutdownAsync
iterate through _registeredProviders
and update each provider’s status concurrently. Each RegisteredProvider
instance’s Status
property is mutated inside Task.WhenAll
without synchronization
Updating shared mutable state in parallel can lead to data races if the Status
property is not thread‑safe. Either update statuses sequentially, protect them with synchronization (e.g., locks) or redesign RegisteredProvider
to be immutable and return a new instance with the updated status.
To ensure we don’t run into issues here, I would suggest consider adding an integration test that validates the concurrent status updates behave correctly. This will help verify thread-safety and prevent potential data races.
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
e35c0a5
to
78acd86
Compare
src/OpenFeature/Providers/MultiProvider/Models/RegisteredProvider.cs
Outdated
Show resolved
Hide resolved
…r and MultiProvider Signed-off-by: André Silva <[email protected]>
This PR
This pull request introduces the experimental "Multi-Provider" feature to the OpenFeature library, enabling simultaneous use of multiple feature flag providers with configurable evaluation strategies. The changes include updates to documentation, new classes and methods to support multi-provider functionality, and a sample implementation in
AspNetCore
. Below is a summary of the most important changes grouped by theme.Documentation Updates:
README.md
, marking it as experimental.README.md
explaining the usage, evaluation strategies, modes, and limitations of the Multi-Provider feature.Sample Implementation:
samples/AspNetCore/Program.cs
to demonstrate Multi-Provider usage, including flag evaluation and error handling. [1] [2]Core Multi-Provider Functionality:
MultiProvider
class insrc/OpenFeature/Providers/MultiProvider/MultiProvider.cs
, implementing support for multiple providers, evaluation strategies, initialization, and shutdown logic.ProviderEntry
class to represent individual providers in the Multi-Provider configuration.Supporting Models and Extensions:
ProviderStatus
andRegisteredProvider
models to track provider states and metadata. [1] [2]ProviderExtensions
to handle evaluation logic for individual providers.Related Issues
Fixes #487
Notes
I didn't do proper testing for
Hooks
andEvents
. This PR is already quite big, and I would appreciate some feedback on the overall design.Follow-up Tasks
How to test
Open the
samples
app and enjoy!