feat: make provider interface "stateless"; SDK maintains provider state by chrfwow · Pull Request #1096 · open-feature/java-sdk

chrfwow · 2024-09-09T14:01:16Z

This PR

Providers no longer maintain their own state: the state for each provider is maintained in the SDK automatically, and updated according to the success/failures of lifecycle methods (init/shutdown) or events emitted from providers spontaneously.

Related Issues

#844

Please remove getState in implementing providers (the interface method has been marked as deprecated).

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow · 2024-09-09T14:19:34Z

+     *
     * @param details The details of the event
     */
    public void emitProviderError(ProviderEventDetails details) {


should there also be a method like emitFatalProviderError?

I would take the approach of just checking if the error is a Fatal error.

How can I do that? The ProviderEventDetails parameter does not have a field containing a potential error code. If I can get it from getEventMetadata(), what key is the error code associated with?

Good point. I think that's another thing we need to add. I might have missed it in the issue, but it's in the spec: https://openfeature.dev/specification/types#event-details

Is it queried like this?

String errorCode = details.getEventMetadata().getString("error code"); if("FATAL".equals(errorCode)){ ... }

I can't see how this should work from the documentation

I'm saying the event details should have a first class member representing the error code (not in the event data), if it's an error event.

chrfwow · 2024-09-09T14:20:32Z

+    @Test void simpleBooleanFlag() throws Exception {
        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
-        api.setProvider(new NoOpProvider());
+        api.setProvider(TestEventsProvider.initialized());


Some of the tests had to be changed, as now an uninitialized provider will not allow the lookup of a value.

It might be better to use setProviderAndWait which will initialize the provider before continuing.

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

toddbaert · 2024-09-09T22:18:06Z

Thanks a lot for this! I will try to review tomorrow! 🙏

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

codecov · 2024-09-10T06:03:13Z

Codecov Report

Attention: Patch coverage is 84.61538% with 20 lines in your changes missing coverage. Please review.

Project coverage is 93.91%. Comparing base (dd8ba81) to head (6d199d9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/java/dev/openfeature/sdk/ProviderRepository.java	76.05%	14 Missing and 3 partials ⚠️
...feature/sdk/providers/memory/InMemoryProvider.java	0.00%	2 Missing ⚠️
...v/openfeature/sdk/FeatureProviderStateManager.java	97.36%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1096      +/-   ##
============================================
- Coverage     95.12%   93.91%   -1.22%     
- Complexity      401      430      +29     
============================================
  Files            39       40       +1     
  Lines           924     1019      +95     
  Branches         56       72      +16     
============================================
+ Hits            879      957      +78     
- Misses           24       39      +15     
- Partials         21       23       +2

Flag	Coverage Δ
unittests	`93.91% <84.61%> (-1.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow · 2024-09-10T08:17:00Z

+            if (ProviderState.FATAL.equals(getState())) {
+                throw new FatalError("provider in fatal error state");
+            }
            throw new GeneralError("unknown error");


The behaviour here is different than that of the OpenFeatureClient, which would not throw this GeneralError. Which bevaviour is the expected one?

See: https://github.com/open-feature/java-sdk/pull/1096/files/98c620f14a81a0fda47207eb9d912826a33ec446#r1752386247

We should be able to delete this code entirely since initialization is tracked in the SDK, but things should also work fine if we don't touch this class at all.

so do you suggest to generate a Second deprecated InMemoryProvider to always ensure backwards compatibility and run the tests for both? or do you think that is overkill?

toddbaert · 2024-09-10T17:25:40Z

+    @Override
+    public final void initialize(EvaluationContext evaluationContext) throws Exception {
+        try {
+            doInitialization(evaluationContext);
+            providerState = ProviderState.READY;
+        } catch (OpenFeatureError openFeatureError) {
+            if (ErrorCode.PROVIDER_FATAL.equals(openFeatureError.getErrorCode())) {
+                providerState = ProviderState.FATAL;
+            } else {
+                providerState = ProviderState.ERROR;
+            }
+            throw openFeatureError;
+        } catch (Exception e) {
+            providerState = ProviderState.ERROR;
+            throw new GeneralError(e);
+        }
+    }
+
+    protected void doInitialization(EvaluationContext evaluationContext) throws Exception {
+        // Intentionally left blank, to be implemented by inheritors
+    }


This implementation is logically correct, but this won't work retroactively with existing implementations (many of which can be found here, though there's others maintained by vendors, etc).

We need to do this in a non-breaking way, without breaking any existing contracts or behvaiors. In C#, I accomplished this by creating a new package-private ProviderState field on the provider which the SDK updates after it has successfully (or unsuccessfully) run the initialize method, and basically ignoring the previous provider state field.

This allows existing providers to work without breaking changes.

@toddbaert I wonder if there can be some additional automated step to see if the sdk-contrib repository providers are not breaking, possibly as warning / non-mandatory. There is the InMemoryProvider on this repository which is a good sanity, maybe checking the other providers as well can help. Or is too much? What do you think ?

toddbaert · 2024-09-10T17:29:23Z

    public Client getClient(String domain, String version) {
-        return new OpenFeatureClient(this,
+        return new OpenFeatureClient(
+                () -> providerRepository.getProvider(domain),


Inside the ProviderRespository, you will have to modify this private method to call the initialize function on the provider if it's not yet initialized or in the process of being initialized, then update a new package-private status member indicating readiness.

toddbaert · 2024-09-10T17:31:04Z

-    /**
-     * Initializes the provider.
-     * @param evaluationContext evaluation context
-     * @throws Exception on error
-     */
-    @Override
-    public void initialize(EvaluationContext evaluationContext) throws Exception {
-        super.initialize(evaluationContext);
-        state = ProviderState.READY;
-        log.debug("finished initializing provider, state: {}", state);
-    }
-


As a proof that nothing is breaking, you should be able to get away with making no changes to this provider.

toddbaert · 2024-09-10T17:39:12Z

+        if (ProviderEvent.PROVIDER_ERROR.equals(event)) {
+            providerState = ProviderState.ERROR;
+        } else if (ProviderEvent.PROVIDER_STALE.equals(event)) {
+            providerState = ProviderState.STALE;
+        } else if (ProviderEvent.PROVIDER_READY.equals(event)) {
+            providerState = ProviderState.READY;
+        }


Can you add a specific test for this? It relates to specification requirement 5.3.5, so you can use our @Specification annotation for tracking compliance.

toddbaert

Really good first pass! I've left some comments. The most important is here.

If you need more help or clarification, please reach out on slack.

liran2000 · 2024-09-10T17:48:13Z

+    @Override
+    public final void shutdown() {
+        providerState = ProviderState.NOT_READY;
+        doShutdown();


why this split is needed ?
without it, provider can implement shutdown() same as today and calling super.shutdown() in it.
providers which were implemented this way, can continue to work as is.
similar with initialize().

liran2000 · 2024-09-10T17:50:42Z


-    public TestEventsProvider(ProviderState initialState) {
-        this.state = initialState;
+    public static TestEventsProvider initialized() throws Exception {


rename to something like newTestEventsProvider() ?

liran2000 · 2024-09-10T17:51:27Z

    transient String flagKey = "mykey";

-    @Test void simpleBooleanFlag() {
+    @Test void simpleBooleanFlag() throws Exception {


can use @SneakyThrows.
also at other places.

^ Apparently, that's someones username 😄

liran2000 · 2024-09-10T17:52:48Z

            // change the provider during a before hook - this should not impact the evaluation in progress
-            public Optional before(HookContext ctx, Map hints) {
-                FeatureProviderTestUtils.setFeatureProvider(new NoOpProvider());
+            public Optional before(HookContext ctx, Map hints)  {


can use @SneakyThrows and remove try/catch ?

liran2000 · 2024-09-10T17:54:19Z

-            public void initialize(EvaluationContext evaluationContext) throws Exception {
-                Awaitility.await().wait(3000);
+            protected void doInitialization(EvaluationContext evaluationContext) throws Exception {
+                Thread.sleep(10000);


why change the implementation and wait that much longer ?

liran2000 · 2024-09-10T17:57:14Z

+        inMemoryProvider.initialize(null);
+        inMemoryProvider.emitProviderError(ProviderEventDetails.builder().build());
+
+        // ErrorCode.GENERAL should be returned when evaluated via the client


why would it return error after provider error event ?
any reference to this behavior ?

any reference to this behavior ?

No, but this is the current behaviour. See #1096 (comment) and the implementation

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

…ke-provider-stateless

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

guidobrei

I have one request for change: I think the @Delegate annotation is a leftover from the Decorator approach and can be removed.

guidobrei · 2024-09-20T10:07:01Z

    @Deprecated() // TODO: eventually we will make this non-public. See issue #872
-    public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String domain, String version) {
+    public OpenFeatureClient(
+            ProviderAccessor providerAccessor,


ProviderAccessor is exposed outside its defined visibility scope. OpenFeatureClient and the constructor are public, but the ProviderAccessor is package privet.

Today I learned... I would have expected the compiler to not allow such constructs.

How should I handle this? Should I make the constructor package private, as it is deprecated anyway?

I would add a package internal accessor to getProviderStateManager(String domain) in OpenFeatureAPI . Then you can access the state manager in the client and you can remove the ´ProviderAccessor` interface completely.

Done: 45e9ba2

guidobrei · 2024-09-20T10:23:45Z

+            }
+            if (ProviderState.FATAL.equals(state)) {
+                throw new FatalError("provider is in an irrecoverable error state");
+            }


We had some effort in parallel to handle evaluation errors without exceptions in #1095.

@toddbaert does this also apply to unexpected Provider states?

In general, how do we handle provider state here? If it's now managed by the SDK, does it only forward the evaluation calls to the provider, if it is in READY state, or in any state except NOT_READY and FATAL?

Currently we only forward calls if the provider is not in error (or fatal), which differs from the implementation of the InMemoryProvider, where they are only forwarded when the provider is READY. Is this what we want?

Currently we only forward calls if the provider is not in error (or fatal), which differs from the implementation of the InMemoryProvider, where they are only forwarded when the provider is READY. Is this what we want?

Yes, this is what we want. Some providers may be in an ERROR state, and still work in some sense (they may send error telemetry, or read from an out-of-date cache, or even try to reconnect before returning). The only states that should short-circuit are FATAL and NOT_READY. In these cases we can be sure the provider hasn't connected at all or is in such a bad state (FATAL) that it's pointless to even use and will never recover (perhaps a bad credential or something like that). This is defined in the spec (and implemented in other SDKs):

https://openfeature.dev/specification/sections/flag-evaluation#requirement-177
https://openfeature.dev/specification/sections/flag-evaluation#requirement-178

As far as throwing errors, yes - it would be better if we could instead return error resolutions but still run the error hooks, as we do in #1095 , to avoid the cost of creating the stack trace and exception.

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

…ke-provider-stateless

guidobrei · 2024-09-20T11:31:18Z

+        return providerRepository.getProviderState(domain);
+    }
+
+    public ProviderState getProviderState(FeatureProvider provider) {


I know I've requested this, but maybe it's better to not expose this on the public API surface unless someone requests it. We could also get rid of the additional code in the ProviderRepository that handles this request again.

Sorry for being too greedy 🙈

This overload would return a null state if the provider is not registered, which is what I would expect (or an exception?), but it's still different to the behavior of other getProviderState() overloads.

Yes I guess the only reason this was done was for this assertion which we can avoid. I agree we should remove these methods.

Done: 45e9ba2

toddbaert · 2024-09-20T14:16:01Z

        FeatureProvider provider = new TestEventsProvider(500);
        OpenFeatureAPI.getInstance().setProviderAndWait(provider);
-        assertThat(api.getProvider().getState()).isEqualTo(ProviderState.READY);
+        assertThat(api.getProviderState()).isEqualTo(ProviderState.READY);


We can instead get a client (even before we set the provider) and then check the Provider state there, so we can remove this public getProviderState as @guidobrei suggests.

Done: 45e9ba2

toddbaert · 2024-09-20T14:18:05Z

@chrfwow I think I agree with @guidobrei 's points. Thanks for being so patient with all these reviews! I think we're almost there but I think the last few changes seem small.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert · 2024-09-23T16:01:01Z

I pushed another changeset to resolve @guidobrei 's suggestions: 45e9ba2

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

sonarqubecloud · 2024-09-23T16:39:05Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

toddbaert · 2024-09-24T12:27:30Z

Thanks @chrfwow for all your help with this! It works great in the contrib repo so far.

chrfwow · 2024-09-24T16:46:35Z

It was a pleasure!

Make provider interface "stateless", SDK maintains provider state

f2e9097

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow requested a review from a team as a code owner September 9, 2024 14:01

chrfwow changed the title ~~Make provider interface "stateless", SDK maintains provider state~~ feat: Make provider interface "stateless", SDK maintains provider state Sep 9, 2024

chrfwow commented Sep 9, 2024

View reviewed changes

Add tests and fix bug

d2b15fe

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow and others added 2 commits September 10, 2024 07:44

Merge branch 'main' into make-provider-stateless

16f57e8

Fix checkstyle errors

b993ee2

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow and others added 2 commits September 10, 2024 08:17

add test

625a404

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

add test

98c620f

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow commented Sep 10, 2024

View reviewed changes

toddbaert requested review from aepfli, beeme1mr, jarebudev, justinabrahms, kinyoklion, liran2000, lukas-reining and toddbaert September 10, 2024 17:10

toddbaert reviewed Sep 10, 2024

View reviewed changes

toddbaert self-requested a review September 10, 2024 17:46

toddbaert requested changes Sep 10, 2024

View reviewed changes

liran2000 reviewed Sep 10, 2024

View reviewed changes

chrfwow and others added 2 commits September 11, 2024 09:06

implement feedback from codereview

d0a2027

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

add wrapper around EventProvider, to manage and hide the provider state

11a0ccb

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow requested a review from liran2000 September 12, 2024 06:42

chrfwow and others added 3 commits September 19, 2024 09:13

Merge remote-tracking branch 'origin/make-provider-stateless' into ma…

e4ea124

…ke-provider-stateless

Merge branch 'main' into make-provider-stateless

28130ba

update readme

975d3ad

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

chrfwow requested review from beeme1mr and toddbaert September 19, 2024 07:20

aepfli approved these changes Sep 19, 2024

View reviewed changes

toddbaert approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'main' into make-provider-stateless

01538c3

guidobrei requested changes Sep 20, 2024

View reviewed changes

chrfwow and others added 2 commits September 20, 2024 12:53

remove unused delegate, update comments

ca8fc16

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>

Merge remote-tracking branch 'origin/make-provider-stateless' into ma…

cd56f25

…ke-provider-stateless

guidobrei reviewed Sep 20, 2024

View reviewed changes

toddbaert reviewed Sep 20, 2024

View reviewed changes

fixup: feedback from guido

45e9ba2

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert requested a review from guidobrei September 23, 2024 16:01

toddbaert added 2 commits September 23, 2024 12:38

fixup: flaky test and spacing

5816732

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Merge branch 'main' into make-provider-stateless

6d199d9

toddbaert reviewed Sep 23, 2024

View reviewed changes

Comment thread src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java

toddbaert changed the title ~~feat: Make provider interface "stateless", SDK maintains provider state~~ feat: make provider interface "stateless"; SDK maintains provider state Sep 23, 2024

toddbaert merged commit 1b1e527 into open-feature:main Sep 23, 2024

github-actions Bot mentioned this pull request Sep 23, 2024

chore(main): release 1.12.0 #1112

Merged

toddbaert mentioned this pull request Sep 23, 2024

feat!: use sdk-maintained state, require 1.12 open-feature/java-sdk-contrib#964

Merged

toddbaert mentioned this pull request Sep 24, 2024

[FEATURE] Make provider interface "stateless", SDK maintains provider state #844

Closed

chrfwow deleted the make-provider-stateless branch September 26, 2024 05:52

chrfwow mentioned this pull request Nov 25, 2024

chore: add chrfwow as member open-feature/community#407

Merged

Conversation

chrfwow commented Sep 9, 2024 • edited by toddbaert Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Related Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddbaert Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddbaert commented Sep 9, 2024

Uh oh!

codecov Bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liran2000 Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddbaert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrfwow Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidobrei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidobrei Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

chrfwow commented Sep 9, 2024 •

edited by toddbaert

Loading

toddbaert Sep 13, 2024 •

edited

Loading

codecov Bot commented Sep 10, 2024 •

edited

Loading

liran2000 Sep 10, 2024 •

edited

Loading

chrfwow Sep 11, 2024 •

edited

Loading

guidobrei Sep 20, 2024 •

edited

Loading

toddbaert Sep 20, 2024 •

edited

Loading

toddbaert Sep 20, 2024 •

edited

Loading