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: proxy sdk #165

Merged
merged 52 commits into from
Aug 3, 2023
Merged

feat: proxy sdk #165

merged 52 commits into from
Aug 3, 2023

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Jul 12, 2023

This adds implementation of Unleash that talks to the proxy endpoints on Edge. This implements the same external interface as the existing DefaultUnleash but the implementation details of a Proxy SDK are very different behind the scenes, which makes for some significant design decisions. The responses returned from the proxy endpoints do not return enough data for the existing SDK to reuse so I've split this out at the repository layer. I don't believe this is a major problem, the construction of the objects is handled by the existing UnleashBuilder, consumers that want to manually construct instances will need to know what they're doing.

  • The API responses are completely different, toggles in Client response can't be handled independently because of segments, while the Proxy responses can. Because of this, I've used the single toggle responses for the Proxy SDK so that the consumer will only incur the cost of toggles they use (API and cache throughput). This means retrieving from cache is significantly lighter than using the existing DefaultUnleash
  • Registration is not handled by Proxy SDKs directly, this is handled by the Proxy or Edge and so registration is a no-op in the DefaultProxyUnleash class
  • The DefaultProxyUnleash class reuses the existing builder, context, caching layers, configuration, and metrics handlers. It does not reuse the existing UnleashRepository or Feature classes. This is intentional for a few reasons: behind the scenes retrieving the relevant feature toggle for the Proxy SDK requires the actual context object to make the decision on how to resolve the feature (the resolution itself is handled by Edge/Proxy). So the method signature looks like this:
    public function findFeatureByContext(string $featureName, ?Context $context = null): ?ProxyFeature;

  • The above is pretty much unavoidable, since that's how the Edge/Proxy works. The returned ProxyFeature is a very stripped down version of whats contained in the existing Feature class, but more to the point, the fields mean vastly different things. While it's probably possible to force the DefaultProxyFeature and DefaultFeature classes to share an interface, I don't believe this is correct, since doing so will signal to consumers that they're interchangeable, when they very much are not.
  • Because the variant returned from Edge/Proxy contains only the fields that are useful for the consumer and not things like weight or stickiness I've created a new interface and class called ResolvedVariant/DefaultResolvedVariant which is now returned from the high level public interfaces, containing only the relevant fields. The existing code now uses the existing Variant/DefaultVariant for internal calculations but no longer exposes it through the high level interfaces. This is a breaking change, albeit a small one

@sighphyre sighphyre force-pushed the feat/proxy-sdk branch 4 times, most recently from b849bf9 to 02f17e1 Compare July 13, 2023 08:00
@sighphyre sighphyre marked this pull request as ready for review July 27, 2023 10:54
Copy link

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

I think most of my comments are details, I'm more than happy to review again after you go over them

examples/09-cached-proxy.php Show resolved Hide resolved
src/DefaultUnleash.php Outdated Show resolved Hide resolved
examples/09-cached-proxy.php Show resolved Hide resolved
src/Repository/DefaultUnleashProxyRepository.php Outdated Show resolved Hide resolved
src/Repository/DefaultUnleashProxyRepository.php Outdated Show resolved Hide resolved
src/Repository/DefaultUnleashProxyRepository.php Outdated Show resolved Hide resolved
sighphyre and others added 2 commits July 27, 2023 15:43
Co-authored-by: Gastón Fournier <gastonfournier@gmail.com>
Copy link
Collaborator

@RikudouSage RikudouSage left a comment

Choose a reason for hiding this comment

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

Overall it looks good, just leaving some comments.

src/Configuration/UnleashConfiguration.php Outdated Show resolved Hide resolved
src/DTO/DefaultProxyFeature.php Outdated Show resolved Hide resolved
src/DefaultProxyUnleash.php Outdated Show resolved Hide resolved
src/DefaultUnleash.php Outdated Show resolved Hide resolved
src/UnleashBuilder.php Outdated Show resolved Hide resolved
src/UnleashBuilder.php Outdated Show resolved Hide resolved
src/UnleashBuilder.php Outdated Show resolved Hide resolved
tests/UnleashBuilderTest.php Outdated Show resolved Hide resolved
tests/UnleashBuilderTest.php Outdated Show resolved Hide resolved
sighphyre and others added 10 commits August 2, 2023 08:44
Co-authored-by: Dominik Chrástecký <dominik@chrastecky.cz>
Co-authored-by: Dominik Chrástecký <dominik@chrastecky.cz>
Co-authored-by: Dominik Chrástecký <dominik@chrastecky.cz>
Co-authored-by: Dominik Chrástecký <dominik@chrastecky.cz>
@RikudouSage
Copy link
Collaborator

Everything looks good to me now, let me know if you're done with all the changes and I'll be happy to merge.

@sighphyre
Copy link
Member Author

Everything looks good to me now, let me know if you're done with all the changes and I'll be happy to merge.

Yes! I'm all happy, shall we merge and release this?

@RikudouSage RikudouSage merged commit ecfa74d into main Aug 3, 2023
11 checks passed
@RikudouSage RikudouSage deleted the feat/proxy-sdk branch August 3, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants