-
Notifications
You must be signed in to change notification settings - Fork 22
feat: Add events to the multi provider #568
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 events to the multi provider #568
Conversation
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…nagement Signed-off-by: André Silva <[email protected]>
…vider Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…n MultiProvider Signed-off-by: André Silva <[email protected]>
…r unsupported run modes Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
… state handling Signed-off-by: André Silva <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
==========================================
- Coverage 90.07% 89.52% -0.55%
==========================================
Files 77 77
Lines 2881 3084 +203
Branches 327 350 +23
==========================================
+ Hits 2595 2761 +166
- Misses 226 256 +30
- Partials 60 67 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: André Silva <[email protected]>
… error handling Signed-off-by: André Silva <[email protected]>
…sistency 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.
Code Review
This pull request introduces event handling to the MultiProvider, which is a significant and valuable feature. The implementation is well-structured and includes a comprehensive set of tests for the new functionality. I have identified a few areas for improvement to enhance robustness and performance:
- A potentially risky use of
Task.Runin the constructor should be addressed to prevent unobserved exceptions. - The event processing loop can be made more efficient.
- A minor bug in a test helper method could lead to test timeouts.
Overall, this is a solid implementation of a complex feature. Addressing these points will further improve the quality of the code.
test/OpenFeature.Providers.MultiProvider.Tests/MultiProviderEventTests.cs
Outdated
Show resolved
Hide resolved
…mance and clarity Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…utdown Signed-off-by: André Silva <[email protected]>
This PR
This pull request introduces event handling and status management improvements to the
MultiProviderclass in theOpenFeature.Providers.MultiProviderpackage. The main changes include adding infrastructure to listen to events from underlying providers, emitting events based on provider and aggregate status changes, and ensuring proper shutdown of event processing. These enhancements improve observability and reliability of the multi-provider setup.Event Handling Infrastructure
MultiProvider, including a dictionary to track event listening tasks and a cancellation token source for clean shutdown. Event listeners are started for all registered providers during initialization. [1] [2] [3]Event Emission and Status Management
MultiProvider. Includes special handling for configuration change events.Shutdown and Disposal
Project Configuration
OpenFeature.csprojto addInternalsVisibleToforOpenFeature.Providers.MultiProvider, allowing internal access for testing and integration.Related Issues
Fixes #552
Notes