feat: add grpc sync flag store #84

colebaileygit · 2024-04-27T10:00:09Z

This PR

adds gRPC sync source for flagd in-process evaluator

Related Issues

Fixes #83

Notes

Follow-up Tasks

How to test

Signed-off-by: Cole Bailey <[email protected]>

codecov · 2024-04-27T10:00:59Z

Codecov Report

Attention: Patch coverage is 97.12919% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.21%. Comparing base (d8e10c7) to head (e9cd2ed).
Report is 33 commits behind head on main.

Files	Patch %	Lines
.../flagd/resolvers/process/connector/file_watcher.py	95.52%	3 Missing ⚠️
...src/openfeature/contrib/provider/flagd/provider.py	83.33%	1 Missing ⚠️
...ure/contrib/provider/flagd/resolvers/in_process.py	94.73%	1 Missing ⚠️
.../contrib/provider/flagd/resolvers/process/flags.py	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
+ Coverage   90.55%   93.21%   +2.65%     
==========================================
  Files           8       15       +7     
  Lines         180      560     +380     
==========================================
+ Hits          163      522     +359     
- Misses         17       38      +21

Flag	Coverage Δ
unittests	`?`

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: Cole Bailey <[email protected]>

federicobond

Awesome work @colebaileygit! I left a few comments and questions merely to help push this forward, without being exhaustive. I know this is still in draft.

federicobond · 2024-04-29T02:01:03Z

providers/openfeature-provider-flagd/docker-compose.yaml

+      context: test-harness
+      dockerfile: sync/Dockerfile.unstable
+    ports:
+      - 9091:9090


Let's make sure this file is excluded from published package. We can do so from the pyproject.toml

Good point. Checking this now it also includes tests and test-harness etc. Should we simplify to only include src?

[tool.hatch.build.targets.sdist] include = [ "src", ]

I'd have to check the generated build to confirm. If you can please make this change in a separate PR.

providers/openfeature-provider-flagd/pyproject.toml

federicobond · 2024-04-29T02:05:55Z

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/grpc_watcher.py

+from openfeature.provider.provider import AbstractProvider
+
+from ....config import Config
+from ....proto.flagd.sync.v1 import sync_pb2, sync_pb2_grpc


For more than one level up, an absolute import is much easier to read and to update if necessary.

I was getting some mypy errors when using absolute import, need to check how to fix those:

providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/grpc_watcher.py:10: error: Cannot find implementation or library stub for module named "openfeature.contrib.provider.flagd.proto.flagd.sync.v1" [import-not-found] from openfeature.contrib.provider.flagd.proto.flagd.sync.v1 import ( ^

Couldn't figure this out in a simple way, seems related to using pre-commit at root of a "mono-repo" but I don't see any clear resources of how to structure it better. Maybe this is a follow-up issue?

This approach that may work as long as the number of packages remains manageable.

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/grpc_watcher.py

Signed-off-by: Cole Bailey <[email protected]>

.github/workflows/build.yml

providers/openfeature-provider-flagd/README.md

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py

Kavindu-Dodan

overal looks good 🙌 nice work and I left few comments, check them when you can

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py

...rs/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py

Signed-off-by: Cole Bailey <[email protected]>

federicobond · 2024-05-06T03:40:05Z

providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/config.py

@@ -42,6 +44,14 @@ def __init__(  # noqa: PLR0913
            env_or_default("FLAGD_TLS", False, cast=str_to_bool) if tls is None else tls
        )
        self.timeout = 5 if timeout is None else timeout
+        self.retry_backoff_seconds: float = (
+            float(env_or_default("FLAGD_RETRY_BACKOFF_SECONDS", 2.0))


Should we do cast=float here for consistency?

Yes, but then it gives me this:

After fighting with mypy a bit I realised that pulling the cast outside of the function is the cleanest way to have clean typing. I would instead suggest refactoring to remove this helper function everywhere and just use the python libraries directly. This abstraction doesn't add too much if you ask me:

self.retry_backoff_seconds: float = ( env_or_default("FLAGD_RETRY_BACKOFF_SECONDS", 2.0, cast=float) if retry_backoff_seconds is None else retry_backoff_seconds )

vs

self.retry_backoff_seconds: float = ( float(os.environ.get("FLAGD_RETRY_BACKOFF_SECONDS", 2.0)) if retry_backoff_seconds is None else retry_backoff_seconds )

federicobond · 2024-05-06T03:41:33Z

...lagd/src/openfeature/contrib/provider/flagd/proto/flagd/evaluation/v1/evaluation_pb2_grpc.py

@@ -2,7 +2,7 @@
 """Client and server classes corresponding to protobuf-defined services."""
 import grpc

-from flagd.evaluation.v1 import evaluation_pb2 as flagd_dot_evaluation_dot_v1_dot_evaluation__pb2
+from . import evaluation_pb2 as flagd_dot_evaluation_dot_v1_dot_evaluation__pb2


Are these changes automated after grpc code generation?

Nope, I actually didn't notice that script. When I try to run it I get this file not found error, does it need to be fixed and maybe documented in the contributing guide?

Failure: could not read file schemas/protobuf/buf.gen.python.yaml: open schemas/protobuf/buf.gen.python.yaml: no such file or directory

providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/provider.py

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py

federicobond · 2024-05-06T04:00:56Z

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py

+
+    def initialize(self, evaluation_context: EvaluationContext) -> None:
+        self.active = True
+        self.thread = threading.Thread(target=self.refresh_file, daemon=True)


Why is daemon set to true in this thread?

Let's add a name to all child threads for easier debugging. This could be FlagdFileWatcherWorkerThread

Basically this:

By setting them as daemon threads, you can let them run and forget about them, and when your program quits, any daemon threads are killed automatically.

https://stackoverflow.com/questions/190010/daemon-threads-explanation

...openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/flags.py

federicobond · 2024-05-06T04:12:28Z

...openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/flags.py

+                transposed = re.sub(
+                    rf"{{\s*\"\$ref\":\s*\"{name}\"\s*}}", json.dumps(rule), transposed
+                )
+            flags = json.loads(transposed)


We should include unit tests for this parsing.

federicobond · 2024-05-06T04:13:25Z

...openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/flags.py

+            flag = cls(key=key, **data)
+            return flag
+        except Exception as err:
+            raise ParseError from err


Add unit tests for this method please.

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/grpc_watcher.py

federicobond · 2024-05-06T04:25:21Z

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py

+            raise ProviderNotReadyError from err
+
+    def shutdown(self) -> None:
+        self.active = False


Shutting down and then initializing again can leave more than one thread running here, because active might not be checked until after the sleep expires.

We have the same issue in both connector implementations. The current thread handling logic is a bit fragile, would it make sense to build a cancellable scheduled task manager and concentrate our efforts on making sure its implementation is robust? I'm happy to discuss how to approach it.

Sounds sensible. Would this be a simple refactor/enhancement we can add directly to this PR? It might be easier for me to review and test a first reference implementation if you can help provide that. I'm definitely not an expert in python thread handling

federicobond · 2024-05-06T05:13:18Z

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py

+
+    def initialize(self, evaluation_context: EvaluationContext) -> None:
+        self.active = True
+        self.thread = threading.Thread(target=self.refresh_file, daemon=True)


Let's add a name to all child threads for easier debugging. This could be FlagdFileWatcherWorkerThread

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/grpc_watcher.py

beeme1mr · 2024-06-10T18:39:37Z

Hey @colebaileygit, could you please resolve all the conversations that have been addressed? It would be great to merge this soon.

colebaileygit · 2024-06-11T06:59:39Z

Hey @colebaileygit, could you please resolve all the conversations that have been addressed? It would be great to merge this soon.

Sorry for disappearing, had a European style vacation and getting back into things now. Will take some time to revisit the PR this week

Signed-off-by: Cole Bailey <[email protected]>

beeme1mr · 2024-07-01T20:53:00Z

Sorry for disappearing, had a European style vacation and getting back into things now. Will take some time to revisit the PR this week

No worries. I hope you had a relaxing vacation. A European-style vacation sure sounds nice😄 At any rate, can you resolve all the addressed threads and provide a short summary of what's remaining? It looks like it's close, and I would love to help get it across the finish line.

colebaileygit · 2024-07-02T21:24:02Z

At any rate, can you resolve all the addressed threads and provide a short summary of what's remaining? It looks like it's close, and I would love to help get it across the finish line.

@beeme1mr I've closed some threads where I provided a best effort fix, feel free to reopen @federicobond if you think they are important!

Main points still open:

building out a better threading solution: feat: add grpc sync flag store #84 (comment)
adding some tests: feat: add grpc sync flag store #84 (comment), feat: add grpc sync flag store #84 (comment)

Otherwise I'd say remaining topics can be follow-up tasks that don't need to block this PR:

fixing grpc code generation
fixing env_or_default cast usage
fixing mypy and absolute imports
fixing files embedded in published package

feat: add grpc sync flag store

1899571

Signed-off-by: Cole Bailey <[email protected]>

fix: float/int type casting

2bfffe6

Signed-off-by: Cole Bailey <[email protected]>

federicobond reviewed Apr 29, 2024

View reviewed changes

beeme1mr mentioned this pull request Apr 30, 2024

docs: add python, update .net provider docs open-feature/flagd#1305

Merged

colebaileygit added 8 commits May 1, 2024 10:28

add: test cases and reconnect functionality

fd8fe11

Signed-off-by: Cole Bailey <[email protected]>

fix: pyproject settings

17e8ce8

Signed-off-by: Cole Bailey <[email protected]>

fix: event assertions

378ccc9

Signed-off-by: Cole Bailey <[email protected]>

add: thread shutdown and cleaner flag store / connector logic

054e5af

Signed-off-by: Cole Bailey <[email protected]>

Merge branch 'main' into feat/grpc-sync

b899d82

add: selector and sync init timeout

19a29e5

Signed-off-by: Cole Bailey <[email protected]>

fix: noisy test due to insufficient wait time

cc14759

Signed-off-by: Cole Bailey <[email protected]>

docs: configuration options in readme

8088a6f

Signed-off-by: Cole Bailey <[email protected]>

colebaileygit marked this pull request as ready for review May 1, 2024 14:29

colebaileygit requested a review from a team as a code owner May 1, 2024 14:29

tests: edge cases for grpc response parsing

e86984b

Signed-off-by: Cole Bailey <[email protected]>

beeme1mr reviewed May 2, 2024

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

providers/openfeature-provider-flagd/README.md Outdated Show resolved Hide resolved

Kavindu-Dodan reviewed May 2, 2024

View reviewed changes

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py Outdated Show resolved Hide resolved

Kavindu-Dodan reviewed May 2, 2024

View reviewed changes

...der-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/connector/file_watcher.py Outdated Show resolved Hide resolved

...rs/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py Outdated Show resolved Hide resolved

colebaileygit added 3 commits May 3, 2024 09:16

Add env var configs to readme

b8fa382

Signed-off-by: Cole Bailey <[email protected]>

fix: improve naming of emit flag

80419a7

Signed-off-by: Cole Bailey <[email protected]>

docs: update CONTRIBUTING details

5782c09

Signed-off-by: Cole Bailey <[email protected]>

federicobond reviewed May 6, 2024

View reviewed changes

Merge branch 'main' into feat/grpc-sync

1f82462

colebaileygit force-pushed the feat/grpc-sync branch from 41ec424 to 217dcf7 Compare June 20, 2024 19:40

colebaileygit added 2 commits June 20, 2024 22:20

add: worker thread names

1b25540

Signed-off-by: Cole Bailey <[email protected]>

fix: remove hasattr usage

f5e89c0

Signed-off-by: Cole Bailey <[email protected]>

colebaileygit added 2 commits June 20, 2024 22:20

fix: circular dependency

45e3739

Signed-off-by: Cole Bailey <[email protected]>

fix: AbstractProvider dependency everywhere

e9cd2ed

Signed-off-by: Cole Bailey <[email protected]>

colebaileygit force-pushed the feat/grpc-sync branch from 4b34fad to e9cd2ed Compare June 20, 2024 20:20

beeme1mr mentioned this pull request Oct 21, 2024

Clarification on provider init #99

Closed

aepfli mentioned this pull request Nov 17, 2024

[bug][flagd-rpc] We can not retrieve object flags due to a type mismatch error #102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add grpc sync flag store #84

feat: add grpc sync flag store #84

colebaileygit commented Apr 27, 2024

codecov bot commented Apr 27, 2024 •

edited

Loading

federicobond left a comment

federicobond Apr 29, 2024

colebaileygit May 1, 2024

federicobond May 1, 2024

federicobond Apr 29, 2024

colebaileygit May 1, 2024

colebaileygit May 2, 2024

federicobond May 2, 2024

Kavindu-Dodan left a comment

federicobond May 6, 2024

colebaileygit Jun 20, 2024

federicobond May 6, 2024

colebaileygit Jun 20, 2024

federicobond May 6, 2024

federicobond May 6, 2024

colebaileygit Jun 20, 2024

federicobond May 6, 2024

federicobond May 6, 2024

federicobond May 6, 2024

federicobond May 6, 2024

colebaileygit Jun 12, 2024 •

edited

Loading

federicobond May 6, 2024

beeme1mr commented Jun 10, 2024

colebaileygit commented Jun 11, 2024

beeme1mr commented Jul 1, 2024

colebaileygit commented Jul 2, 2024

feat: add grpc sync flag store #84

Are you sure you want to change the base?

feat: add grpc sync flag store #84

Conversation

colebaileygit commented Apr 27, 2024

This PR

Related Issues

Notes

Follow-up Tasks

How to test

codecov bot commented Apr 27, 2024 • edited Loading

Codecov Report

federicobond 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

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

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

Choose a reason for hiding this comment

colebaileygit Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeme1mr commented Jun 10, 2024

colebaileygit commented Jun 11, 2024

beeme1mr commented Jul 1, 2024

colebaileygit commented Jul 2, 2024

codecov bot commented Apr 27, 2024 •

edited

Loading

colebaileygit Jun 12, 2024 •

edited

Loading