feat: in-process provider #149

bacherfl · 2024-01-29T13:43:09Z

Part of #139

This PR adds the support for an in-process provider, using the flagd.sync API to connect to a sync stream serving the flag configurations specified by the selector (determined by the FLAGD_SOURCE_SELECTOR environment variable).

Currently only the built-in JSONLogic operators are supported, i.e. the custom operators specified here are yet to be implemented - however, it might be better to implement this in a follow-up PR

Signed-off-by: Florian Bacher <[email protected]>

bacherfl · 2024-01-31T07:51:38Z

This should be ready for a first round of reviews - @toddbaert as discussed, we can leave this PR open and create a separate PR on top of that to implement the custom JSONLogic evaluators, or merge this one without the custom evaluators and deactivate the in-process mode until the additional evaluators have been implemented - whatever you prefer :)

toddbaert · 2024-01-31T19:10:23Z

Thanks @bacherfl ! I will thoroughly review this in the next couple days!

toddbaert · 2024-02-01T20:26:11Z

Hey @bacherfl, I decided a nice way to test this would be to implement the gherkin e2e test suite here for both RPC and in-process settings. I opened a draft PR here, built on your branch (only test projects added). I was able to get the RPC provider working, but not the in-process.

You can run in-process e2e tests on this branch with:

 docker run -p 8013:9090 ghcr.io/open-feature/sync-testbed:latest

and then in a new terminal:

export FLAGD_RESOLVER_TYPE=IN_PROCESS && dotnet test test/OpenFeature.Contrib.Providers.Flagd.E2e.ProcessTest/

If you want, you can run the RPC (which pass) with:

docker run -p 8013:8013 ghcr.io/open-feature/flagd-testbed:latest

and then in a new terminal:

export FLAGD_RESOLVER_TYPE=RPC && dotnet test test/OpenFeature.Contrib.Providers.Flagd.E2e.RpcTest/

I only got the suites working toward the end of the day so it could be something I'm missing with my config, but based on my debugging it seems like the correct port/host/resolver is set to match the testbed, and the provider seems to become ready, but evaluations fail (default).

toddbaert · 2024-02-01T20:28:33Z

src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs

-        private readonly ICache<string, object> _cache;
-        private int _eventStreamRetries;
-        private int _eventStreamRetryBackoff = EventStreamRetryBaseBackoff;
+        private readonly Metadata _providerMetadata = new Metadata(ProviderName);


It's a really good idea to refactor a lot of code out of this file, nice job. There's a lot of unused imports at the top now though.

toddbaert · 2024-02-01T20:32:50Z

src/OpenFeature.Contrib.Providers.Flagd/FlagdConfig.cs

+        internal ResolverType ResolverType
+        {
+            get => _resolverType;
+            set => _resolverType = value;
+        }
+
+        internal string SourceSelector
+        {
+            get { return _sourceSelector; }
+            set { _sourceSelector = value; }
+        }
+


In Kubernmetes, the ENV_VAR based configs make a lot of sense, but I think we need a way to use the in-process provider configured in code code as well, so users who aren't in K8s can use whatever configuration tools they want to configure flagd directly.

I think we should either allow configuration through FlagdConfig (which would mean exposing a public constructor on it, and public fields) or add another constructor on the provider itself which takes a URL and Resolver type. Whatever is easier, I think (though I should point out we also have a public FlagdProvider constructor which takes a FlagdConfig which is sort-of useless now I think.

+1 for exposing a configuration-based constructor. This should be easily done through a builder. And I can point to Java [1] & go flagd [2] providers which allows both env vars and constructor paramters

[1] - https://github.com/open-feature/java-sdk-contrib/tree/main/providers/flagd#in-process-resolver
[2] - https://github.com/open-feature/go-sdk-contrib/tree/main/providers/flagd#in-process-resolver

Kavindu-Dodan · 2024-02-01T22:48:38Z

src/OpenFeature.Contrib.Providers.Flagd/InProcessResolver.cs

+using Grpc.Core;
+using Value = OpenFeature.Model.Value;
+
+namespace OpenFeature.Contrib.Providers.Flagd


minor - may be it's good to namespace in-process vs rpc so that they are proper access modifiers. In the long run this should help in organizing, especially with upcoming json evaluators, offline mode file readers sort of logic.

Kavindu-Dodan · 2024-02-01T23:00:15Z

src/OpenFeature.Contrib.Providers.Flagd/InProcessResolver.cs

+
+        private async void HandleEvents()
+        {
+            while (_eventStreamRetries < _config.MaxEventStreamRetries)


I think this needs to be improved. In-process provider is special as it only have a single flag sync mechanism (currently grpc stream). If we end reconnections after few retries, then flags store will go stale and this is not a desired state. Similar to what we do in other implementations, (for example Java [1]) we should use some exponential back-off (up to a limit) and continuously re-attempt to connect till we get a shutdown request. This should help with debugging as there will be continuous errors if something is wrong with connectivity (ex:- api address change) and reconnect and server fresh flags if problem goes away

[1] - https://github.com/open-feature/java-sdk-contrib/blob/main/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/storage/FlagStore.java#L88

Also worth noting that some gRPC implementations have listener implementations so you can listen to the underlying connection state and restore the stream when the connection is restored. With this model, the stream retry options etc are used to control the parameters of the underlying connection retries, and the stream just reconnects accordingly.

It looks like channel.WaitForStateChangedAsync could perhaps help us here, but its not available in all our targets 😩

@toddbaert yeah, that's a good suggestion. And I hope unlike Java, .Net exposes configurations to control internal grpc reconnect configurations

Kavindu-Dodan · 2024-02-01T23:04:01Z

src/OpenFeature.Contrib.Providers.Flagd/JsonEvaluator.cs

+            var reason = Reason.Default;
+            if (_flags.TryGetValue(flagKey, out var flagConfiguration))
+            {
+                reason = Reason.Static;


We have to check for DISABLED state of the flag. Maybe you can refer Java impl [1] or go impl [2] to see if all checks map here :)

[1] - https://github.com/open-feature/java-sdk-contrib/blob/main/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/InProcessResolver.java#L158

[2] - https://github.com/open-feature/flagd/blob/main/core/pkg/evaluator/json.go#L292

toddbaert · 2024-02-06T19:15:16Z

src/OpenFeature.Contrib.Providers.Flagd/InProcessResolver.cs

+
+        private async void HandleEvents()
+        {
+            while (_eventStreamRetries < _config.MaxEventStreamRetries)


By default while (_eventStreamRetries < _config.MaxEventStreamRetries) evaluates to False, since MaxEventStreamRetries is zero (this means nothing is ever read). Additionally the retries ENV VAR seem to be ignored without the cache.

EDIT: looking into it more, including the Java impl and the flagd spec, I don't think we should use this configuration var at all for the in-process impl. We should infinitely try to reconnect as @Kavindu-Dodan says. I will add this point to the flags spec

toddbaert · 2024-02-06T19:17:32Z

src/OpenFeature.Contrib.Providers.Flagd/InProcessResolver.cs

+        public void Init()
+        {
+            var handleEvents = new Thread(HandleEvents);
+            handleEvents.Start();
+        }


This method needs to block until the connection is established, or else the setProvider returns and the READY event fires before the provider is actually READY (and indeed in might never bet ready). I would use some kind of latch or callback or something to block until we connect.

As a hacky fix you can add a Thread.sleep here for 1000 and things "work".

toddbaert · 2024-02-06T19:21:24Z

src/OpenFeature.Contrib.Providers.Flagd/InProcessResolver.cs

+
+        public void Shutdown()
+        {
+            throw new NotImplementedException();


We need to stop the threads and kill the connections here. As it is, with every SetProvider for this provider we have resource leaks (threads and connections).

We need to do the same thing in the RPC version.

Kavindu-Dodan · 2024-02-08T20:02:14Z

src/OpenFeature.Contrib.Providers.Flagd/Resolver/Rpc/RpcResolver.cs

+            }
+        }
+
+        internal RpcResolver(Service.ServiceClient client, FlagdConfig config, ICache<string, object> cache = null)


Minor - from quick look on the usage, this seems to be not used. If it does however, I think it's best to constructor chain to this constructor from config based constructor :)

good catch - it is used in the unit tests, but i adapted the code to use a constructor chain :)

Kavindu-Dodan · 2024-02-08T20:03:56Z

src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/InProcessResolver.cs

+            _evaluator = new JsonEvaluator(config.SourceSelector);
+        }
+
+        internal InProcessResolver(FlagSyncService.FlagSyncServiceClient client, FlagdConfig config)


Minor - please check if this is used (seems not per my view). And if it is, let's use constructor chaining to avoid duplicate code :)

good catch - it is used in the unit tests, but i adapted the code to use a constructor chain :)

Kavindu-Dodan · 2024-02-08T20:34:14Z

src/OpenFeature.Contrib.Providers.Flagd/JsonEvaluator.cs

+using OpenFeature.Error;
+using OpenFeature.Model;
+
+namespace OpenFeature.Contrib.Providers.Flagd


Minor - packing could be set to Resolver.InProcess

Kavindu-Dodan · 2024-02-08T20:34:33Z

src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/InProcessResolver.cs

+using Grpc.Core;
+using Value = OpenFeature.Model.Value;
+
+namespace OpenFeature.Contrib.Providers.Flagd.RResolver.InProcess


Minor - RResolver could be a typo

Signed-off-by: Florian Bacher <[email protected]>

Signed-off-by: Todd Baert <[email protected]>

toddbaert · 2024-02-09T21:15:31Z

src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/InProcessResolver.cs

+                        var response = call.ResponseStream.Current;
+                        _evaluator.Sync(FlagConfigurationUpdateType.ALL, response.FlagConfiguration);


I think this was a race - we we signaling the latch before we read, which caused some issues.

toddbaert · 2024-02-09T21:17:14Z

src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs

+        public override Task Initialize(EvaluationContext context)
+        {
+            return Task.Run(() =>
+            {
+                this._resolver.Init();
+                this._status = ProviderStatus.Ready;
+
+            }).ContinueWith((t) =>
+            {
+                this._status = ProviderStatus.Error;
+                if (t.IsFaulted) throw t.Exception;
+            });
+        }


@bacherfl @Kavindu-Dodan I added an initialize function, the old provider was built before this was implemented; it's no longer done in the contstructor.

toddbaert

e2e tests are working with some minor noted changes. Thanks @bacherfl

Signed-off-by: Todd Baert <[email protected]>

Kavindu-Dodan · 2024-02-09T22:24:00Z

src/OpenFeature.Contrib.Providers.Flagd/Resolver/Rpc/RpcResolver.cs

+
+        public void Shutdown()
+        {
+            throw new System.NotImplementedException();


I think we should handle this as we now support init and shutdown

I've implemented this now, see commit here.

cc @bacherfl

Kavindu-Dodan · 2024-02-09T22:25:42Z

src/OpenFeature.Contrib.Providers.Flagd/Resolver/Rpc/RpcResolver.cs

+            if (_config.CacheEnabled)
+            {
+                var handleEvents = new Thread(HandleEvents);
+                handleEvents.Start();
+            }


I think this is something interesting to consider for other implementations 🤔

Kavindu-Dodan

Left few improvement comments, but this already looks promising :)

Signed-off-by: Todd Baert <[email protected]>

bacherfl added 6 commits January 10, 2024 13:18

extract rpc resolver into its own class

aa69b8b

Signed-off-by: Florian Bacher <[email protected]>

use resolver in provider

b6f1887

Signed-off-by: Florian Bacher <[email protected]>

implementation of JsonLogic evaluator

12d6b23

Signed-off-by: Florian Bacher <[email protected]>

implemented in-process provider

1f63a85

Signed-off-by: Florian Bacher <[email protected]>

adapted unit tests

9105d7d

Signed-off-by: Florian Bacher <[email protected]>

some minor restructuring, unit tests for in process resolver

0ce3ca4

Signed-off-by: Florian Bacher <[email protected]>

github-actions bot assigned bacherfl and toddbaert Jan 29, 2024

github-actions bot requested a review from toddbaert January 29, 2024 13:44

bacherfl added 5 commits January 29, 2024 14:52

Merge branch 'main' into feat/in-memory-provider

7159e59

Signed-off-by: Florian Bacher <[email protected]>

added ConfigureAwait

8ae2ea9

Signed-off-by: Florian Bacher <[email protected]>

fixed unit test

85e963b

Signed-off-by: Florian Bacher <[email protected]>

fix linting errors

457fcea

Signed-off-by: Florian Bacher <[email protected]>

updated README.md

9ebe097

Signed-off-by: Florian Bacher <[email protected]>

bacherfl marked this pull request as ready for review January 31, 2024 07:51

bacherfl requested a review from a team as a code owner January 31, 2024 07:51

toddbaert requested review from Kavindu-Dodan and austindrenski January 31, 2024 19:11

toddbaert reviewed Feb 1, 2024

View reviewed changes

toddbaert mentioned this pull request Feb 1, 2024

chore: flagd provider e2e tests #150

Merged

Kavindu-Dodan reviewed Feb 1, 2024

View reviewed changes

toddbaert reviewed Feb 6, 2024

View reviewed changes

Kavindu-Dodan reviewed Feb 8, 2024

View reviewed changes

bacherfl and others added 2 commits February 9, 2024 07:56

adapted to PR review

d8eb5f4

Signed-off-by: Florian Bacher <[email protected]>

chore: flagd provider e2e tests (#150)

af51e09

Signed-off-by: Todd Baert <[email protected]>

github-actions bot assigned askpt Feb 9, 2024

github-actions bot requested a review from askpt February 9, 2024 20:23

toddbaert added 6 commits February 9, 2024 15:45

Chore/in memory provider tests (#153)

ed4b4d3

Signed-off-by: Todd Baert <[email protected]>

fixup: services

171d320

Signed-off-by: Todd Baert <[email protected]>

fixup: rm no-build

f5513e9

Signed-off-by: Todd Baert <[email protected]>

fixup: rm no-build

57e4668

Signed-off-by: Todd Baert <[email protected]>

fixup: test setup

1e1524c

Signed-off-by: Todd Baert <[email protected]>

fix: fix race?

a47f34e

Signed-off-by: Todd Baert <[email protected]>

toddbaert reviewed Feb 9, 2024

View reviewed changes

toddbaert self-requested a review February 9, 2024 21:18

toddbaert approved these changes Feb 9, 2024

View reviewed changes

fixup: remove E2E=false from win

7256479

Signed-off-by: Todd Baert <[email protected]>

toddbaert requested a review from Kavindu-Dodan February 9, 2024 21:50

Kavindu-Dodan reviewed Feb 9, 2024

View reviewed changes

Kavindu-Dodan approved these changes Feb 9, 2024

View reviewed changes

toddbaert added 2 commits February 12, 2024 11:03

fixup: shutdown

a0e8666

Signed-off-by: Todd Baert <[email protected]>

fixup: assert-until default

738a5cd

Signed-off-by: Todd Baert <[email protected]>

Kavindu-Dodan approved these changes Feb 12, 2024

View reviewed changes

fixup: init in tests

699cab9

Signed-off-by: Todd Baert <[email protected]>

toddbaert merged commit f7371dc into main Feb 13, 2024
8 checks passed

github-actions bot mentioned this pull request Feb 13, 2024

chore(main): release OpenFeature.Contrib.Providers.Flagd 0.1.9 #157

Merged

This was referenced Apr 30, 2024

chore(main): release OpenFeature.Contrib.Providers.Flagd 0.2.0 #191

Closed

chore(main): release OpenFeature.Contrib.Providers.Flagd 0.1.9 #192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: in-process provider #149

feat: in-process provider #149

bacherfl commented Jan 29, 2024

bacherfl commented Jan 31, 2024 •

edited

Loading

toddbaert commented Jan 31, 2024

toddbaert commented Feb 1, 2024 •

edited

Loading

toddbaert Feb 1, 2024

toddbaert Feb 1, 2024 •

edited

Loading

Kavindu-Dodan Feb 1, 2024

Kavindu-Dodan Feb 1, 2024

Kavindu-Dodan Feb 1, 2024

toddbaert Feb 2, 2024

toddbaert Feb 2, 2024 •

edited

Loading

Kavindu-Dodan Feb 2, 2024

Kavindu-Dodan Feb 1, 2024

toddbaert Feb 6, 2024 •

edited

Loading

toddbaert Feb 6, 2024

toddbaert Feb 6, 2024 •

edited

Loading

Kavindu-Dodan Feb 8, 2024

bacherfl Feb 9, 2024

Kavindu-Dodan Feb 8, 2024

bacherfl Feb 9, 2024

Kavindu-Dodan Feb 8, 2024

Kavindu-Dodan Feb 8, 2024 •

edited

Loading

toddbaert Feb 9, 2024

toddbaert Feb 9, 2024

toddbaert left a comment

Kavindu-Dodan Feb 9, 2024

toddbaert Feb 12, 2024

Kavindu-Dodan Feb 9, 2024

Kavindu-Dodan left a comment

		var response = call.ResponseStream.Current;
		_evaluator.Sync(FlagConfigurationUpdateType.ALL, response.FlagConfiguration);

feat: in-process provider #149

feat: in-process provider #149

Conversation

bacherfl commented Jan 29, 2024

bacherfl commented Jan 31, 2024 • edited Loading

toddbaert commented Jan 31, 2024

toddbaert commented Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

toddbaert Feb 1, 2024 • edited Loading

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

toddbaert Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert Feb 6, 2024 • edited Loading

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

Kavindu-Dodan Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert left a 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

Kavindu-Dodan left a comment

Choose a reason for hiding this comment

bacherfl commented Jan 31, 2024 •

edited

Loading

toddbaert commented Feb 1, 2024 •

edited

Loading

toddbaert Feb 1, 2024 •

edited

Loading

toddbaert Feb 2, 2024 •

edited

Loading

toddbaert Feb 6, 2024 •

edited

Loading

toddbaert Feb 6, 2024 •

edited

Loading

Kavindu-Dodan Feb 8, 2024 •

edited

Loading