search

open-feature / python-sdk-contrib

Community contributions for hooks and reference providers in python
https://openfeature.dev
10 stars 11 forks source link

feat: implement grpc protocol version of FlagdProvider #29

Closed federicobond closed 10 months ago

federicobond commented 11 months ago

This PR

Implements a gRPC protocol version of the Flagd Provider. It has been tested manually but not extensively. There are some difference with the way it handles errors for which I couldn't find a definitive answer in the spec.

Notes

Follow-up Tasks

beeme1mr commented 11 months ago

Amazing, thanks! I'll take a look later tonight or tomorrow. @toddbaert should be able to help clarify how we handle generated code in other languages and any other implementation questions.

beeme1mr commented 11 months ago

Hey @federicobond, sorry for the delay. I'll try and answer as many questions as I can. I may need @toddbaert to help with any remaining questions.

federicobond commented 11 months ago

Thank you for the review @beeme1mr!

Specifically what I want to know is whether I should raise an exception or return a FlagEvaluationDetails with each error. Java maps errors to exceptions. If I follow the spec correctly I believe it would be more idiomatic for Python to also raise an exception:

In cases of abnormal execution, the provider MUST indicate an error using the idioms of the implementation language, with an associated error code and optional associated error message.

beeme1mr commented 11 months ago

I would recommend raising an exception but either approach would work. This should only occur when there's an error, so the overhead of an exception is acceptable in my opinion.

codecov[bot] commented 11 months ago

Codecov Report

Merging #29 (ebb42cd) into main (1d893df) will decrease coverage by 23.76%. The diff coverage is 72.09%.

@@             Coverage Diff             @@
##             main      #29       +/-   ##
===========================================
- Coverage   87.20%   63.44%   -23.76%     
===========================================
  Files           8        8               
  Lines         125      238      +113     
===========================================
+ Hits          109      151       +42     
- Misses         16       87       +71
Flag Coverage Δ
unittests 63.44% <72.09%> (-23.76%) :arrow_down:

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

Files Coverage Δ
openfeature/contrib/providers/flagd/provider.py 81.57% <72.09%> (+1.57%) :arrow_up:

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

federicobond commented 11 months ago

Removed generated code, switched to raise exceptions and improved error handling.

beeme1mr commented 10 months ago

I would recommend raising an exception but either approach would work. This should only occur when there's an error, so the overhead of an exception is acceptable in my opinion.

Another thing to consider is how the Python SDK decides to run the error hook. In most SDKs, this happens in a catch block. If that's the case in Python, you should throw.

federicobond commented 10 months ago

Indeed. Python runs error hooks in a catch block so the exception is the most correct.

toddbaert commented 10 months ago

woah this is awesome! I will review today, tomorrow latest.

toddbaert commented 10 months ago

Should we leave generated grpc code under version control or should we regenerate each time?

My recommendation is to generate it each time, as you seem to have done.

I see no issues here, this is great progress and probably worthy of release on its own. In addition to what you've pointed out, there's a few other enhancements that could be made (I would be comfortable helping with them as well, if I can find the time):