feat: Add OpenFeature provider #111

chris-hoefgen · 2023-10-20T23:09:54Z

Add new OpenFeature Provider to the SDK
Clean up some of the exception handling

src/main/java/com/devcycle/sdk/server/cloud/api/DevCycleCloudClient.java

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleUserFactory.java

src/examples/java/com/devcycle/examples/CloudExample.java

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleProvider.java

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleUserFactory.java

src/main/java/com/devcycle/sdk/server/common/model/DevCycleUser.java

rob-odwyer

Looks solid - just a few questions.

At a high level I'm unsure about whether we want to throw some of the exceptions vs. mimic the DevCycle SDK's strategy of minimizing errors in favor of returning defaults.

OpenFeature.md

README.md

src/main/java/com/devcycle/sdk/server/common/model/DevCycleUser.java

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleProvider.java

liran2000

LGTM from OpenFeature perspective.
Left comments on context wrapping and PROVIDER_NAME.

OpenFeature.md

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleProvider.java

…n unit tests with the local bucketing client

…cal in the provider

…ust ignore the bad value

toddbaert · 2023-11-03T13:40:27Z

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleProvider.java

+                return ProviderEvaluation.<Value>builder()
+                        .value(defaultValue)
+                        .reason(Reason.ERROR.toString())
+                        .errorCode(ErrorCode.GENERAL)
+                        .errorMessage(e.getMessage())
+                        .build();


I would suggest changing this to throw an error. The SDK handles all the defaulting in that case. This is important because in the case of the Java SDK, the error hooks/telemetry only run if an error is thrown during evaluation.

I realize this is confusing because we allow for the errorCode and errorMessage to be set, which gives the impression this is a valid way to communicate errors up the call stack. We're considering changing this to make things clearer.

toddbaert · 2023-11-03T13:40:46Z

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleProvider.java

+                return ProviderEvaluation.<T>builder()
+                        .value(defaultValue)
+                        .reason(Reason.ERROR.toString())
+                        .errorCode(ErrorCode.GENERAL)
+                        .errorMessage(e.getMessage())
+                        .build();


Same as https://github.com/DevCycleHQ/java-server-sdk/pull/111/files#r1381696312

toddbaert · 2023-11-03T13:47:44Z

src/examples/java/com/devcycle/examples/OpenFeatureExample.java

+        DevCycleLocalClient devCycleClient = new DevCycleLocalClient(server_sdk_key, options);
+
+        for (int i = 0; i < 10; i++) {
+            if (devCycleClient.isInitialized()) {
+                break;
+            }
+            Thread.sleep(500);
+        }


It depends on how DevCycle users already use the product, but if you want to avoid something like this in OpenFeature use-cases, you could implement the getState method, and return NOT_READY, then do something like this in the initialize method of the provider, and setting READY and returning when you have initialized.

toddbaert

Again late to the party, but left some suggestions.

chris-hoefgen marked this pull request as draft October 20, 2023 23:10

gitstream-cm bot added the 30 min review label Oct 20, 2023

chris-hoefgen force-pushed the DVC-7734-openfeature-provider branch from 11410e2 to d74590c Compare October 24, 2023 16:52

chris-hoefgen commented Oct 24, 2023

View reviewed changes

src/main/java/com/devcycle/sdk/server/cloud/api/DevCycleCloudClient.java Outdated Show resolved Hide resolved

chris-hoefgen marked this pull request as ready for review October 24, 2023 21:27

chris-hoefgen commented Oct 25, 2023

View reviewed changes

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleUserFactory.java Outdated Show resolved Hide resolved

chris-hoefgen requested a review from a team October 25, 2023 16:23

JamieSinn reviewed Oct 25, 2023

View reviewed changes

src/main/java/com/devcycle/sdk/server/common/model/DevCycleUser.java Outdated Show resolved Hide resolved

rob-odwyer reviewed Oct 27, 2023

View reviewed changes

liran2000 reviewed Oct 29, 2023

View reviewed changes

OpenFeature.md Outdated Show resolved Hide resolved

src/main/java/com/devcycle/sdk/server/openfeature/DevCycleProvider.java Outdated Show resolved Hide resolved

chris-hoefgen added 19 commits October 30, 2023 09:16

initial commit

8ec0bfe

fixing tests

4ba1e0b

formatting and code cleanup

d751c74

tweaked unit test

5cbe419

add docs

c571f96

More code cleanup

d1eaae9

more docs

0fe9a8d

Changed to the new pattern Jamie came up with

5d2e491

unit test fix

4797103

More readme tweaks

caca0ca

Add openfeature example to GHA test

bba7d2b

updated some class references and ran a formatter

40fdc2f

refactored out the user factory

126a8d8

renamed method

c276a6f

Fixed issues with JSON object translations and added a new integratio…

2277a4a

…n unit tests with the local bucketing client

switched to better exception assertions

e9bdc10

Fixed issue with Integer values not being converted properly

3146825

removed a null check

7c08414

Fixing some null data issues and differentiating between Cloud and Lo…

ef5d64a

…cal in the provider

chris-hoefgen added 10 commits October 30, 2023 09:16

Adding nulls as allowable custom data propery

1c9802a

I don't think an exception is necessary here, in the docs we say we j…

5a3b46d

…ust ignore the bad value

docs cleanup

7bbceb4

Added some more examples

0e6dccd

formatter

a7d69c2

feedback

4848f52

feedback bug

ea03e6b

fix some unit tests

5424e87

Applying some OpenFeature feedback

a1aaa0c

added null value to doc

4e21dc9

chris-hoefgen force-pushed the DVC-7734-openfeature-provider branch from b31d6c9 to 4e21dc9 Compare October 30, 2023 16:16

rob-odwyer approved these changes Oct 30, 2023

View reviewed changes

chris-hoefgen merged commit 5475ca3 into main Oct 30, 2023
8 checks passed

toddbaert reviewed Nov 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add OpenFeature provider #111

feat: Add OpenFeature provider #111

chris-hoefgen commented Oct 20, 2023 •

edited

Loading

rob-odwyer left a comment

liran2000 left a comment •

edited

Loading

toddbaert Nov 3, 2023

toddbaert Nov 3, 2023

toddbaert Nov 3, 2023 •

edited

Loading

toddbaert left a comment

feat: Add OpenFeature provider #111

feat: Add OpenFeature provider #111

Conversation

chris-hoefgen commented Oct 20, 2023 • edited Loading

rob-odwyer left a comment

Choose a reason for hiding this comment

liran2000 left a comment • edited Loading

Choose a reason for hiding this comment

toddbaert Nov 3, 2023

Choose a reason for hiding this comment

toddbaert Nov 3, 2023

Choose a reason for hiding this comment

toddbaert Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

toddbaert left a comment

Choose a reason for hiding this comment

chris-hoefgen commented Oct 20, 2023 •

edited

Loading

liran2000 left a comment •

edited

Loading

toddbaert Nov 3, 2023 •

edited

Loading