Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add flipt provider with PSR-16 caching support #98

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

legoheld
Copy link

This PR

Adds a provider for https://www.flipt.io/ with PSR-16 caching support.

How to test

How to run tests is documented in the README.md

Copy link

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor things, looks good otherwise from a Flipt perspective! thanks @legoheld !!

.vscode/launch.json Outdated Show resolved Hide resolved
providers/Flipt/LICENSE Show resolved Hide resolved
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (13ebc3c) 57.58% compared to head (41f4474) 57.58%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #98   +/-   ##
=======================================
  Coverage   57.58%   57.58%           
=======================================
  Files          28       28           
  Lines         422      422           
=======================================
  Hits          243      243           
  Misses        179      179           

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

Copy link
Member

@tcarrio tcarrio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request! It's looking pretty good so far, mostly have questions around the caching provided thus far.

providers/Flipt/src/Cache.php Outdated Show resolved Hide resolved
providers/Flipt/src/FliptProvider.php Outdated Show resolved Hide resolved
providers/Flipt/src/FliptProvider.php Outdated Show resolved Hide resolved
return json_decode( $result->getVariantAttachment(), true);
case FlagValueType::STRING:
return $result->getVariantKey();
default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never happen, in which case if it did, it should be an error not nullng the value.

Copy link
Author

@legoheld legoheld Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure if this is an issue as I use this only when I get a successfull response from the client:

if( $result->getReason() == ResponseReasons::MATCH_EVALUATION_REASON || $result->getReason() == ResponseReasons::DEFAULT_EVALUATION_REASON ) {
   $result = ResolutionDetailsFactory::fromSuccess( $this->castResult( $result, $flagType ) );
}

And the I suppose a correct value. But you are in a way right if the response value is somewhat invalid I should throw an error. What do you propose here?

Comment on lines 44 to 47
$entries = $this->cache->get( self::CACHE_KEY, []);
$entries[ $key ] = $value;

$this->cache->set( self::CACHE_KEY, $entries );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces race conditions for distributed storage caches like Redis or Memcached. You can atomically interact with the cache per key if you utilize a key prefix instead.

Suggested change
$entries = $this->cache->get( self::CACHE_KEY, []);
$entries[ $key ] = $value;
$this->cache->set( self::CACHE_KEY, $entries );
$this->cache->set($key, $value);

This does not fix the issue I mentioned below about the collisions. To resolve both, you would still need a different solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, utilize a sufficiently large range hash function to avoid collisions. This might be something to consider being a configurable setting, e.g. raw JSON encoded keys or md5 or sha256 would all be valid strategies, to give you the overall safety necessary for the application.

Copy link
Author

@legoheld legoheld Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this is the hard part. With PSR-16 there is no way to delete all keys with a certain cache key prefix.
So I have two ways to solve that either the way you have seen ( all entries under one key ) or I store the keys to delete under a separate cache key. But the later is probably more efficient.

But with both ways you have racing conditions as you mentioned.

I actually dont know how to solve that with PSR-16. Maybe just by documenting that this is not ready to use with a distributed caching storage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dedicated cahce storage -> clear will clear all keys.

providers/Flipt/tests/FliptProviderTest.php Outdated Show resolved Hide resolved
providers/Flipt/README.md Outdated Show resolved Hide resolved
legoheld added 4 commits November 27, 2023 10:40
…vider

fix: Add ResponseReasons constants
fix: proper FliptProvider constructor type
fix: Add ksort for context attributes
…vider

fix: Add ResponseReasons constants
fix: proper FliptProvider constructor type
fix: Add ksort for context attributes

Signed-off-by: legoheld <dreckig@sandkasten>
Signed-off-by: legoheld <dreckig@sandkasten>
@legoheld
Copy link
Author

Thank you @tcarrio for this very detailed feedback!

"open-feature",
"feature flags"
],
"homepage": "https://github.com/legoheld/flipt-provider",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be this repo? or our (Flipt's) website?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the homepage as its optional. We do it like the other providers.

providers/Flipt/composer.json Outdated Show resolved Hide resolved
legoheld and others added 12 commits November 29, 2023 09:00
Signed-off-by: legoheld <dreckig@sandkasten>
…vider

fix: Add ResponseReasons constants
fix: proper FliptProvider constructor type
fix: Add ksort for context attributes

Signed-off-by: legoheld <dreckig@sandkasten>
Signed-off-by: legoheld <[email protected]>
Signed-off-by: legoheld <dreckig@sandkasten>
Signed-off-by: legoheld <[email protected]>
Signed-off-by: legoheld <dreckig@sandkasten>
Signed-off-by: legoheld <[email protected]>
@tcarrio
Copy link
Member

tcarrio commented Dec 8, 2023

A few things I noticed after we talked @legoheld :

  • unit tests exist but do not run in CI. Include the Flipt provider package in the matrix for testing in GH Actions
  • I will need to create the corresponding repository this will "deploy" through before we merge. We gitsplit the packages to individual repos hosted under @open-feauture-php

@tcarrio
Copy link
Member

tcarrio commented Dec 17, 2023

The new repository has been created.

You will need to point the gitsplit config for the new Flipt provider to it in the .gitsplit.yml file

@markphelps
Copy link

👋🏻 Is there anything I can do to help get this across the line? much appreciated @legoheld and @tcarrio !!

@tcarrio
Copy link
Member

tcarrio commented Aug 9, 2024

@legoheld Just reaching out to see if this is something you still wanted to continue with. If so, then the commits would need to be assigned in order to contribute the work.

Let me know! 👋

@legoheld
Copy link
Author

legoheld commented Aug 9, 2024

Thanks for the reminder @tcarrio.

Im quite busy with projects and havent caught up with the flip progress.
@markphelps would you like to take over?

@markphelps
Copy link

Thanks for the reminder @tcarrio.

Im quite busy with projects and havent caught up with the flip progress. @markphelps would you like to take over?

Yes I will take over. Thank you @legoheld !!

@tcarrio
Copy link
Member

tcarrio commented Aug 10, 2024

No worries, that works fine @legoheld. If the work is being continued from your branch then you should sign off the commits for the DCO.

If you have questions @markphelps throw them in the thread and I'll get to it as soon as I can 👋

@tcarrio tcarrio requested a review from a team as a code owner September 4, 2024 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants