feat: Client provider guidelines #14

thomaspoignant · 2024-04-30T21:41:18Z

This PR is a first draft for a provider client spec.

Please provide feedback on this one to know if I am heading in the right direction and to know if it can be helpful to create providers.

Those are the guidelines related to the issue #11

markphelps

couple minor typos/grammar suggestions

provider/specs/client.md

beeme1mr

Great start, thanks! I added a few suggestions but I think this will be helpful.

We may want to avoid calling it a spec (at least for now). I would recommend calling it a guideline.

provider/specs/client.md

beeme1mr · 2024-05-01T15:13:10Z

provider/specs/client.md

+The endpoint will return a list of configuration for the provider:
+- `name`: Name of the flag management system, it should be used in the `metadata.name` name (ex: `OFREP Web Provider ${metadata.name}`).
+- `capabilities`: List of capabilities of the flag management system and their associated configuration. *Check the [capabilities section](#capabilities) for the details of each capability.*
+


What's the expected behavior if the call to /ofrep/v1/configuration fails?

Should fallback to the defaults and I think we should define the default behaviour in this spec

Hum good question, we were wondering with @lukas-reining and I think we should put the provider in error and retry later (probably with an exponential backoff).

WDYT?

Will calling the configuration API be an opt-in feature? If it's explicitly enabled by the user, then putting the provider into an error state may make sense. Otherwise, I don't think it should.

If we want to configure it, I will be more in favour of having an opt-out rather than an opt-in.

The reasoning is that this will encourage vendors to provide the information on how it works but also to avoid using the default values, the information of this endpoint is here to help to work better with the flag management system, not using them by default will be a bit weird from my point of view.

This will affect initial page load times, and the config API doesn't provide anything (at least not yet) that couldn't be defined explicitly by the OFREP constructor.

provider/specs/client.md

Kavindu-Dodan

Thanks @thomaspoignant for getting started on this 🙌

I have following general remarks,

Language - Given this is a spec and to be similar to OF Spec, I think we should generilize langauge to avoid "we" usage (ex:- In this specification we will specify.. to This specificaiton specify..)
Spec name & title - Given this is specific to client side (aka static context), a good name for file could be client-provider/ static-context-provider

Otherwise I think this is a great first step

thomaspoignant · 2024-05-02T21:22:57Z

Thanks @thomaspoignant for getting started on this 🙌

I have following general remarks,

* Language - Given this is a spec and to be similar to OF Spec, I think we should generilize langauge to avoid "we" usage (ex:- `In this specification we will specify..` to `This specificaiton specify..`)

* Spec name & title - Given this is specific to client side (aka static context), a good name for file could be `client-provider`/ `static-context-provider`

Otherwise I think this is a great first step

I have renamed the file and stopped using the term specification to avoid any confusion here.
Do you think we should continue to update the language if it is only a guideline?

Kavindu-Dodan · 2024-05-03T21:15:53Z

I have renamed the file and stopped using the term specification to avoid any confusion here.

Thanks :)

Do you think we should continue to update the language if it is only a guideline?

Yeah, can start simple and then convert this to a spec. So let's not spend too much time on this.

Kavindu-Dodan

I am happy with this so we can get started :)

beeme1mr · 2024-05-15T20:30:54Z

guideline/static-context-provider.md

+An implementation of an OFREP client provider should start with an initialization of the provider.
+
+The `initialize()` function should follow those steps:
+1. Make a GET request to the `/ofrep/v1/configuration` endpoint to retrieve the configurations return by the flag management system and store them in memory to be available for all the function of the provider. *(See [Annexe 1](#annexe-1) for the description of the endpoint response)*


Can this step be optional? The provider should be able to work without having to call the configuration API.

This is also my general feeling, that this should be an opt-in feature as @beeme1mr says.

I was more thinking toward something that can be enabled by default and opt-out on demand.
See #14 (comment)

guideline/static-context-provider.md

beeme1mr · 2024-05-15T20:38:15Z

guideline/static-context-provider.md

+##### Polling
+`polling`: define how the provider should do the polling
+
+- `enabled`: if `true` the provider should poll the `/ofrep/v1/evaluate/flags` regularly to check if any flag evaluation has changed in the flag management system.


This isn't the current behavior in the OFREP web provider.

https://github.com/open-feature/js-sdk-contrib/blob/main/libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts#L90

OFREP web provider by default is doing polling.
You have a default value set if no pollingInterval is provided during the initialization.

https://github.com/open-feature/js-sdk-contrib/blob/c00f998227f55ca5a64b01d039ab6ec2aff18f7c/libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts#L78

lukas-reining

Nice start and it is really valuable to have!
I left some comments, and I would generally tent to describe the expected behavior of the provider in third person instead of using first person developer.

lukas-reining · 2024-05-15T21:23:52Z

guideline/static-context-provider.md

+An implementation of an OFREP client provider should start with an initialization of the provider.
+
+The `initialize()` function should follow those steps:
+1. Make a GET request to the `/ofrep/v1/configuration` endpoint to retrieve the configurations return by the flag management system and store them in memory to be available for all the function of the provider. *(See [Annexe 1](#annexe-1) for the description of the endpoint response)*


This is also my general feeling, that this should be an opt-in feature as @beeme1mr says.

lukas-reining · 2024-05-15T21:25:54Z

guideline/static-context-provider.md

+   - If the endpoint returns an error, the `initialize()` function must error and exit.  
+   - If the request is successful, we should store in a local cache all of the flags evaluation results returned by the API in a local cache. We should also store the `ETag` header in the provider to be able to send it back later.


I think it would be better to leave the we out and talk in third person about the provider.

guideline/static-context-provider.md

lukas-reining · 2024-05-15T21:30:17Z

guideline/static-context-provider.md

+The evaluation should not perform any remote API calls.
+
+When calling an evaluation function the provider should check if the associated type is supported, by checking the key `capabilities.flagEvaluation.unsupportedTypes` from the configuration endpoint.
+- If the type is unsupported, we should exit early and directly return an error.


Do we know which error we want to throw here? I think it would be nice to specify here.
Maybe we can even have a new Error type, the case that a system does not support a type happens pretty often. (independently from OFREP)

Yes, this is true, should we add that to the OF spec?

Mh yes I think we should add it to the OF spec.
I could open a PR next days, what do you think @thomaspoignant ?

It would be awesome

lukas-reining · 2024-05-15T21:31:13Z

guideline/static-context-provider.md

+```mermaid
+flowchart TD
+    A[evaluation\nfunction] --> B{Is function type in\nunsupportedTypes?}
+    B --> |YES| C(return an error)
+    B --> |NO| D{Is flag key stored\nin local cache?}
+    D --> |NO| E(return a FlagNotFound error)
+    D --> |YES| F{Is cached evaluation\nresponse in error?} 
+    F --> |YES| G(Map the error as a\nprovider error and return)
+    F --> |NO| H{Is the flag value the\nsame type as the\nevaluation function?}
+    H --> |YES| I(return the evaluation response)
+    H --> |NO| J(return a TypeMismatch error)
+```


Love it! :)

Yes, this is excellent!

guideline/static-context-provider.md

nicklasl · 2024-05-17T07:07:14Z

guideline/static-context-provider.md

+
+In the constructor, the provider should check if the `baseURL` is a valid URL and return an error if the URL is invalid.
+
+## Initialize the provider


I'm a bit torn here wether my thinking is going to deep or the use case is too fringe but bare with me:

Some applications have strict rules or goals of how fast they need to be able to show their initial screen. These apps may not have time (or do not want) to wait for network requests to finish before considering a feature flagging SDK to be initialised.

Obviously there are many ways one can solve this, but we have written mobile providers with disk persistence where we then can pass a FetchingStrategy to the constructor where the app owner can decide wether or not they want the initialize() function to block until fresh data is available or simply load any previous data from disk (and then refresh from the backend in the background).

Firebase Remote Config explains the concepts here.

I like the idea of various initialization strategies and the link to Firebase Remote Config is a great resource.

Perhaps one way to implement that would be to put this section under a "Fetch on load" strategy. We could start with a single strategy (and perhaps make it the default) but that would give us the ability to add additional strategies to support specific use cases.

Interesting point here. I see the value of having multiple fetching strategies but I am not sure how to frame that into this document.

I see 2 options:

We try to define the different possible ways of doing the initialisation for the OFREP client providers, and we add them to the document.

We remove that block since there is some chance that it could be different to initialize something depending on the platform (web/mobile).

Do you have any preference?

I'm thinking that it might be most reasonable to ignore my comment about this and go ahead with a blocking network call as part of the initialize function in order to not block this document.

If we believe that the topic warrants additional discussion we can open an issue to discuss it there and amend the spec or add an appendix. wdyt?

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

.github/workflows/spec-validate.yaml

thomaspoignant requested a review from a team as a code owner April 30, 2024 21:41

thomaspoignant requested review from beeme1mr, Kavindu-Dodan and lukas-reining April 30, 2024 21:45

markphelps reviewed May 1, 2024

View reviewed changes

beeme1mr reviewed May 1, 2024

View reviewed changes

Kavindu-Dodan reviewed May 1, 2024

View reviewed changes

provider/specs/client.md Outdated Show resolved Hide resolved

Kavindu-Dodan reviewed May 1, 2024

View reviewed changes

thomaspoignant force-pushed the client-spec branch 2 times, most recently from 623950c to 5c18eb8 Compare May 2, 2024 21:18

Kavindu-Dodan approved these changes May 13, 2024

View reviewed changes

thomaspoignant changed the title ~~feat: Client provider spec~~ feat: Client provider guidelines May 14, 2024

beeme1mr reviewed May 15, 2024

View reviewed changes

lukas-reining reviewed May 15, 2024

View reviewed changes

nicklasl reviewed May 17, 2024

View reviewed changes

thomaspoignant force-pushed the client-spec branch from 7859893 to a61e2bf Compare May 17, 2024 17:02

thomaspoignant and others added 12 commits May 17, 2024 19:08

feat: Client provider spec

3702d36

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

34f336a

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

dbdd5f4

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

14eaec1

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

667bb96

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

e4fa8a3

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

680d546

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

4a8a859

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

175abb3

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

25d6fbe

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

f01598f

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

7f735b2

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

thomaspoignant and others added 11 commits May 17, 2024 19:08

Update provider/specs/client.md

af3beac

Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

14ac393

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update provider/specs/client.md

4cecfc5

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

update with review comments

87d4238

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Move to guideline folder

0038810

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Replace specification

3008500

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

add OpenAPI spec validator based on redocly cli (#15)

f3ec588

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

chore(deps): update actions/checkout action to v4 (#16)

2522810

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update guideline/static-context-provider.md

ad36231

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update guideline/static-context-provider.md

4a3f70a

Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

Update guideline/static-context-provider.md

961f3c7

Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>

thomaspoignant force-pushed the client-spec branch from ed3ac70 to 961f3c7 Compare May 17, 2024 17:09

Kavindu-Dodan self-requested a review May 17, 2024 21:26

Kavindu-Dodan reviewed May 17, 2024

View reviewed changes

.github/workflows/spec-validate.yaml Outdated Show resolved Hide resolved

Merge branch 'main' into client-spec

69b6f5b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Client provider guidelines #14

feat: Client provider guidelines #14

thomaspoignant commented Apr 30, 2024 •

edited

Loading

markphelps left a comment

beeme1mr left a comment

beeme1mr May 1, 2024

Kavindu-Dodan May 1, 2024

thomaspoignant May 2, 2024

beeme1mr May 3, 2024

thomaspoignant May 15, 2024

beeme1mr May 28, 2024

Kavindu-Dodan left a comment

thomaspoignant commented May 2, 2024

Kavindu-Dodan commented May 3, 2024 •

edited

Loading

Kavindu-Dodan left a comment

beeme1mr May 15, 2024

lukas-reining May 15, 2024

thomaspoignant May 17, 2024

beeme1mr May 15, 2024

thomaspoignant May 17, 2024

lukas-reining left a comment

lukas-reining May 15, 2024

lukas-reining May 15, 2024

lukas-reining May 15, 2024

thomaspoignant May 17, 2024

lukas-reining May 17, 2024

thomaspoignant May 18, 2024

lukas-reining May 15, 2024

nicklasl May 16, 2024

nicklasl May 17, 2024

beeme1mr May 17, 2024

thomaspoignant May 17, 2024

nicklasl May 21, 2024

		- If the endpoint returns an error, the `initialize()` function must error and exit.
		- If the request is successful, we should store in a local cache all of the flags evaluation results returned by the API in a local cache. We should also store the `ETag` header in the provider to be able to send it back later.


		In the constructor, the provider should check if the `baseURL` is a valid URL and return an error if the URL is invalid.

		## Initialize the provider

feat: Client provider guidelines #14

Are you sure you want to change the base?

feat: Client provider guidelines #14

Conversation

thomaspoignant commented Apr 30, 2024 • edited Loading

markphelps left a comment

Choose a reason for hiding this comment

beeme1mr 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

Kavindu-Dodan left a comment

Choose a reason for hiding this comment

thomaspoignant commented May 2, 2024

Kavindu-Dodan commented May 3, 2024 • edited Loading

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

lukas-reining 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

Choose a reason for hiding this comment

thomaspoignant commented Apr 30, 2024 •

edited

Loading

Kavindu-Dodan commented May 3, 2024 •

edited

Loading