Skip to content

feat: introduce OFREP provider #477

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

Merged
merged 12 commits into from
Apr 5, 2024

Conversation

Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan commented Mar 15, 2024

This PR

Fixes #473 and introduce the Go OFREP provider 🎉

Usage

Consider the following code snippet for basic usage,

provider := ofrep.NewProvider(
                     "http://localhost:8016",
                      ofrep.WithBearerToken("TOKEN"))

Implementation

This implementation relies on OFREP single flag evaluation endpoint. And there is no caching layer at the current implementation and flag evaluations always rely on remote evaluation results.

I have introduced modules where appropriate which should allow us to expand the implementation with a caching layer and other OFREP extensions that we plan to introduce

@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/of-ofrep-provider branch from dba701c to 2213f6b Compare March 15, 2024 21:41
@Kavindu-Dodan Kavindu-Dodan changed the title feat: [WIP] introduce OFREP provider feat: introduce OFREP provider Mar 19, 2024
@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/of-ofrep-provider branch from ed7f8a8 to 6b9f13f Compare March 19, 2024 18:43
@Kavindu-Dodan Kavindu-Dodan marked this pull request as ready for review March 19, 2024 18:55
@Kavindu-Dodan Kavindu-Dodan requested a review from a team as a code owner March 19, 2024 18:55
@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/of-ofrep-provider branch from 6b9f13f to d045d9c Compare March 19, 2024 19:12
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

Looks good to me!
All seems as we planned it for JS and I can not find any issues.

@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/of-ofrep-provider branch 3 times, most recently from 27b5173 to d767130 Compare March 27, 2024 16:50
@thomaspoignant
Copy link
Member

Hey @Kavindu-Dodan great work, just one quick question.
I see a lot of similar code, and I was wondering why you did not use generics since you are using go1.21?
It will make the provider maybe a bit easier to understand.

What do you think?

@thomaspoignant
Copy link
Member

I also think that you don't have implement the retry-later header when we receive a 429

@Kavindu-Dodan
Copy link
Contributor Author

Kavindu-Dodan commented Apr 1, 2024

@thomaspoignant thank you for reviewing the PR.

I see a lot of similar code, and I was wondering why you did not use generics

I assume you are referring to flags.go [1]. Yeah, it seems to have repeated logic differed by returned type, but each handler has value conversions and a specific return type of struct. I had a quick look into refactoring them but I couldn't find any good way of doing that.

note - checked JS implementation, which use generic ResolutionDetails. unfortunately, go doesn't use such a generic type

I also think that you don't have implement the retry-later header when we receive a 429

For Go SDK, I focused on the dynamic context paradigm, hence there is no support for bulk evaluation. So to handle 429, I think what we should do is to introduce a request blocker with a timer [2]. I haven't done it in this PR.

[1] - https://github.com/open-feature/go-sdk-contrib/pull/477/files#diff-df3e1ced5c1dbe3274ff2d4cc8a47eaf0aab9475b4f526501c30491cb9d499ca
[2] - https://github.com/open-feature/go-sdk-contrib/pull/477/files#diff-4f6c2e021ab239118a7327dccc6fcab63a9f8480d79e34f57006ad7114fb463eR69-R70

@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/of-ofrep-provider branch from d767130 to 57cd493 Compare April 1, 2024 15:19
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/of-ofrep-provider branch from 69f63d5 to 69283f9 Compare April 1, 2024 17:49
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Copy link
Member

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

👌

@Kavindu-Dodan Kavindu-Dodan requested a review from liran2000 April 3, 2024 16:44
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.

[OFREP] create the initial version of the remote evaluation protocl
6 participants