feat!: Add support for provider shutdown and status. #158

kinyoklion · 2023-10-31T20:09:18Z

This PR

Adds support for provider initialization, shutdown, and status.

This PR does not add support for events, but does put several items in place to facilitate their implementation.

Related Issues

Partially Addresses #126

Notes

Follow-up Tasks

How to test

codecov · 2023-10-31T20:10:55Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (9c42d4a) 93.91% compared to head (8108e63) 94.30%.
Report is 7 commits behind head on main.

Files	Patch %	Lines
src/OpenFeature/FeatureProvider.cs	57.14%	3 Missing ⚠️
src/OpenFeature/ProviderRepository.cs	99.15%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   93.91%   94.30%   +0.39%     
==========================================
  Files          20       21       +1     
  Lines         542      650     +108     
  Branches       39       54      +15     
==========================================
+ Hits          509      613     +104     
- Misses         20       23       +3     
- Partials       13       14       +1

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

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion · 2023-10-31T20:14:56Z

src/OpenFeature/Api.cs

        /// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param>
-        public void SetProvider(FeatureProvider featureProvider)
+        public async Task SetProvider(FeatureProvider featureProvider)


So, returning a task here is a breaking API change. This seems like the most appropriate action. Await the method if you care that it finishes initialization, don't await if you do not.

Other options would be adding another method, but this method would just be async without a way to know it finished, versus being sync.

This all applies to named providers as well.

Yea I've been thinking about this myself its quite common to offer bother sync and async libraries especially when working on a old legacy project in full framework. Lets just leave it for now and see if anyone raise concern before going ahead an introduce a sync based methods.

So, returning a task here is a breaking API change. This seems like the most appropriate action. Await the method if you care that it finishes initialization, don't await if you do not.

Other options would be adding another method, but this method would just be async without a way to know it finished, versus being sync.

"events" are the mechanism by which you can call this synchronously and "know" the outcome (your READY/ERROR event handlers will run). I know they don't exist on this SDK yet, but they will eventually.

Other options would be adding another method

This is what's been done in Java and JS, and what the spec recommends: https://openfeature.dev/specification/sections/flag-evaluation/#requirement-118

So, the spec makes a lot of sense for Java. Complete-able futures are not super convenient, and you would probably need something like kotlin on top to make a useful API out of it.

Dotnet though has reasonably proper async, and the general means would be a method you either await, or don't await, or synchronously wait.

The language in the spec doesn't really work, and I am not sure what would.

Usually if you have a sync and async method you would have.

doTheThing and doTheThingAsync. But, doTheThing would be synchronous, not async without completion.

The spec recommends setProviderAndWait, which implies sync, but I don't think we actually want to call .Wait on behalf of the user. They can make the choice to asynchronously await, not await or wait, or to synchronously wait, and we don't have to worry about quirks in their thread execution model that could lead to things like a deadlock.

Either approach will result in an API that is not intuitive for the ecosystem. It can be done, it is just unfortunate.

(To elaborate AndWait or Wait in general have been used to provide sync APIs where the base API is async, so it would be a confusing name)

I think, if the API is fully async, then we don't need to worry about the Async suffix. It really is a case of libraries that implemented things synchronously, and then needed async methods as well.

The interface for this API is almost all Async, and it happened that there was this sync method, so making it async would provide a consistent async interface.

It may be worth checking the rest of the API to see if other items should move to async with this.

(I guess the client is really where things are async, and the "Api" doesn't have that many methods that are async, or would conceivably be async.)

I'm fine with keeping it all async and not providing a sync api. I think since we aren't 1.x.x yet its the best time to make a breaking change to SetProvider, i think we just need to call it out in the release notes.

This SDK is already version 1.3.1 but we can release a version 2.0 if it makes sense. I think it does make sense in this case since it simplifies waiting for a provider to initialize and the migration process should be straightforward.

I'm fine with the breaking change, especially if we agree we'll do the same breaking change in JS, which has the same async/await semantics.

kinyoklion · 2023-10-31T20:15:34Z

src/OpenFeature/Api.cs

-            {
-                this._featureProviderLock.ExitWriteLock();
-            }
+            await this._repository.SetProvider(featureProvider, this.GetContext()).ConfigureAwait(false);


I refactored the collection management out of the Api and moved it into the ProviderRepository. This is similar to the Java SDK.

kinyoklion · 2023-10-31T20:16:38Z

test/OpenFeature.Tests/OpenFeatureTests.cs

@@ -20,6 +21,74 @@ public void OpenFeature_Should_Be_Singleton()
            openFeature.Should().BeSameAs(openFeature2);
        }

+        [Fact]


I added some basic tests here, but the tests for the registry are much more comprehensive.

kinyoklion · 2023-10-31T20:19:46Z

src/OpenFeature/ProviderRepository.cs

+        public async Task SetProvider(
+            FeatureProvider featureProvider,
+            EvaluationContext context,
+            Action<FeatureProvider> afterSet = null,


All these "actions" can be used to facilitate implementing events support. I didn't want to put too much stuff together, so this should be a nice middle ground.

beeme1mr · 2023-11-13T16:00:30Z

Sorry for the delayed response. Now that we have the ability to perform initialization from within the provider, it makes sense to make this an async call. Unfortunately, that means it will be a breaking change, but it should be easy for users to absorb this change. I did bring this up during the last community call, and no one had any concerns. I think I'll also do something similar in the JS SDK.

Before we release a 2.0 version, could the rest of the SDK be audited to make sure there are no other breaking changes we should consider?

FYI @toddbaert @kinyoklion @benjiro

test/OpenFeature.Tests/ProviderRepositoryTests.cs

toddbaert · 2023-11-13T18:56:53Z

src/OpenFeature/ProviderRepository.cs

+                this._providersLock.ExitWriteLock();
+            }
+
+            await InitProvider(featureProvider, context, afterInitialization, afterError).ConfigureAwait(false);


At first I thought it was necessary to put this in the locked block, but I don't think it matters, since the important thing is to maintain thread safety of the provider dict, not make any guarantees about provider initialization order.

toddbaert

Testing looks good. I'm OK with the breaking change as discussed, but in that case I think we should probably wait for events and release both of these changes together.

Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion · 2023-11-13T23:17:21Z

Testing looks good. I'm OK with the breaking change as discussed, but in that case I think we should probably wait for events and release both of these changes together.

Should I re-target this PR to a feature branch?

benjiro · 2023-11-14T08:12:32Z

Testing looks good. I'm OK with the breaking change as discussed, but in that case I think we should probably wait for events and release both of these changes together.

Should I re-target this PR to a feature branch?

I would think it's fine to merge we just don't release/publish the next version until events are done.

toddbaert · 2023-11-14T14:57:40Z

Testing looks good. I'm OK with the breaking change as discussed, but in that case I think we should probably wait for events and release both of these changes together.

Should I re-target this PR to a feature branch?

No no, we'll just refrain from releasing if possible until events. If we need to release for some reason, it won't be a huge problem to release this on its own. The worse case would be we have another breaking change for events (not likely, it didn't happen in other SDKs) and we release a v3.

kinyoklion changed the title ~~ffeat: Add support for provider shutdown and status.~~ feat: Add support for provider shutdown and status. Oct 31, 2023

feat: Add support for provider shutdown and status.

22b07b0

Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion force-pushed the rlamb/add-shutdown-support branch from 73c15d9 to 22b07b0 Compare October 31, 2023 20:12

kinyoklion commented Oct 31, 2023

View reviewed changes

kinyoklion marked this pull request as ready for review October 31, 2023 20:18

kinyoklion requested a review from a team as a code owner October 31, 2023 20:18

kinyoklion commented Oct 31, 2023

View reviewed changes

beeme1mr requested review from bacherfl, benjiro and toddbaert October 31, 2023 20:23

benjiro approved these changes Nov 5, 2023

View reviewed changes

beeme1mr changed the title ~~feat: Add support for provider shutdown and status.~~ feat!: Add support for provider shutdown and status. Nov 13, 2023

beeme1mr added the breaking-change Changes in exposed interfaces or behaviour label Nov 13, 2023

toddbaert reviewed Nov 13, 2023

View reviewed changes

test/OpenFeature.Tests/ProviderRepositoryTests.cs Outdated Show resolved Hide resolved

toddbaert reviewed Nov 13, 2023

View reviewed changes

toddbaert approved these changes Nov 13, 2023

View reviewed changes

Update test/OpenFeature.Tests/ProviderRepositoryTests.cs

8108e63

Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

kinyoklion merged commit 24c3441 into main Nov 14, 2023
11 checks passed

github-actions bot mentioned this pull request Nov 14, 2023

chore(main): release 1.4.0 #154

Merged

beeme1mr deleted the rlamb/add-shutdown-support branch November 14, 2023 16:56

toddbaert mentioned this pull request Nov 21, 2023

Changes to support spec v0.6.0, v0.7.0 compliance #126

Closed

9 tasks

toddbaert mentioned this pull request Nov 28, 2023

[FEATURE] Update setProvider to be async and remove setProviderAndWait open-feature/js-sdk#695

Open

adams85 mentioned this pull request Jan 4, 2024

feat: Add ConfigCat provider open-feature/dotnet-sdk-contrib#119

Merged

toddbaert mentioned this pull request Jan 17, 2024

chore: revert breaking setProvider #190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Add support for provider shutdown and status. #158

feat!: Add support for provider shutdown and status. #158

kinyoklion commented Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

kinyoklion Oct 31, 2023

benjiro Nov 1, 2023

toddbaert Nov 1, 2023

kinyoklion Nov 1, 2023 •

edited

kinyoklion Nov 1, 2023

kinyoklion Nov 3, 2023

kinyoklion Nov 3, 2023

benjiro Nov 5, 2023

beeme1mr Nov 13, 2023

toddbaert Nov 13, 2023

kinyoklion Oct 31, 2023

kinyoklion Oct 31, 2023

kinyoklion Oct 31, 2023

beeme1mr commented Nov 13, 2023

toddbaert Nov 13, 2023

toddbaert left a comment

kinyoklion commented Nov 13, 2023

benjiro commented Nov 14, 2023

toddbaert commented Nov 14, 2023

feat!: Add support for provider shutdown and status. #158

feat!: Add support for provider shutdown and status. #158

Conversation

kinyoklion commented Oct 31, 2023

This PR

Related Issues

Notes

Follow-up Tasks

How to test

codecov bot commented Oct 31, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinyoklion Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeme1mr commented Nov 13, 2023

Choose a reason for hiding this comment

toddbaert left a comment

Choose a reason for hiding this comment

kinyoklion commented Nov 13, 2023

benjiro commented Nov 14, 2023

toddbaert commented Nov 14, 2023

codecov bot commented Oct 31, 2023 •

edited

kinyoklion Nov 1, 2023 •

edited