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: internal provider state, new client events #241

Merged
merged 29 commits into from
Feb 22, 2024
Merged

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Feb 6, 2024

  • moves provider state into SDK (SDK now maintains provider state/lifecycle; provider interface is now "stateless")
  • refine semantics around client state when context is pending/reconciled by using new events/states, instead of STALE
  • add PROVIDER_FATAL error code

Resolves: #238


For background discussion, see #238


This should make provider authorship less error prone, and simpler. Providers essentially become stateless as far as the SDK is concerned. The provider is only obligated to optionally implement the initialize, shutdown and onContextChange functions and maintain is own private state (caches, connection, etc).

This also refines the semantics around context reconciliation, which is particularly important for client SDKs where providers need to reconcile their flags or ruleset(s) when context changes.

I believe these changes can all be implemented by SDKs in a non-breaking fashion.

@toddbaert toddbaert requested a review from beeme1mr February 6, 2024 02:28
specification/sections/01-flag-evaluation.md Outdated Show resolved Hide resolved
specification/sections/01-flag-evaluation.md Outdated Show resolved Hide resolved
specification/sections/01-flag-evaluation.md Outdated Show resolved Hide resolved
specification/sections/02-providers.md Outdated Show resolved Hide resolved
specification/sections/05-events.md Outdated Show resolved Hide resolved
specification/sections/05-events.md Outdated Show resolved Hide resolved
specification/sections/05-events.md Outdated Show resolved Hide resolved
specification/sections/05-events.md Show resolved Hide resolved
specification/sections/05-events.md Outdated Show resolved Hide resolved
specification/types.md Outdated Show resolved Hide resolved
specification/sections/03-evaluation-context.md Outdated Show resolved Hide resolved
specification/sections/05-events.md Outdated Show resolved Hide resolved
specification/sections/05-events.md Show resolved Hide resolved
specification/sections/05-events.md Show resolved Hide resolved
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.

Changes look good to me, minding the open conversations! :)

@beeme1mr
Copy link
Member

beeme1mr commented Feb 7, 2024

I think we can reduce the implementation burden on provider developers and ensure greater consistency by leveraging the provider state in a few additional ways. If we add a FATAL state to the SDK, we could short circuit evaluations within the SDK itself. Hooks would still run, but the underlying provider would not be called. The SDK could return the value specified by the developer within the evaluation itself. The reason returned to the user would be ERROR, the error type would be FATAL, and the message would be whatever the provider defined (e.g. 'invalid auth token`). This would be useful because many existing feature flag SDKs will return a default value on error, which may look like a successful evaluation in OpenFeature unless the provider explicitly handles this scenario. I believe this will lead to inconsistent behavior across implementations unless the SDK handles it.

It may be worth performing the same short circuiting within the SDK if the provider isn't ready yet. This wouldn't only affect providers that have implemented an initialization function that hasn't completed prior to a flag evaluation. This would provide many of the same benefits as described above.

Finally, the error events should also include error code. Currently, only the error message is defined in the spec.

What do you think?
@liran2000 @kinyoklion @lukas-reining @thomaspoignant @fabriziodemaria @toddbaert @jonathannorris

@liran2000
Copy link
Member

I think we can reduce the implementation burden on provider developers and ensure greater consistency by leveraging the provider state in a few additional ways. If we add a FATAL state to the SDK, we could short circuit evaluations within the SDK itself. Hooks would still run, but the underlying provider would not be called. The SDK could return the value specified by the developer within the evaluation itself. The reason returned to the user would be ERROR, the error type would be FATAL, and the message would be whatever the provider defined (e.g. 'invalid auth token`). This would be useful because many existing feature flag SDKs will return a default value on error, which may look like a successful evaluation in OpenFeature unless the provider explicitly handles this scenario. I believe this will lead to inconsistent behavior across implementations unless the SDK handles it.

It may be worth performing the same short circuiting within the SDK if the provider isn't ready yet. This wouldn't only affect providers that have implemented an initialization function that hasn't completed prior to a flag evaluation. This would provide many of the same benefits as described above.

Finally, the error events should also include error code. Currently, only the error message is defined in the spec.

What do you think? @liran2000 @kinyoklion @lukas-reining @thomaspoignant @fabriziodemaria @toddbaert @jonathannorris

Sounds good to me

toddbaert and others added 18 commits February 20, 2024 18:46
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Fabrizio Demaria <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member Author

I'll merge this by EOD today unless I hear objections.

@toddbaert toddbaert merged commit ae286cf into main Feb 22, 2024
6 checks passed
@toddbaert toddbaert deleted the status-changes branch February 22, 2024 00:58
toddbaert added a commit that referenced this pull request Feb 22, 2024
No substantial changes here, just editorial improvements and
corrections.

When implementing #241, I
noticed a few oversights (see comments).

Signed-off-by: Todd Baert <[email protected]>
github-merge-queue bot pushed a commit to open-feature/js-sdk that referenced this pull request Mar 1, 2024
Implements the newest spec changes
[here](open-feature/spec#241).

- Provider state is deprecated (state is now maintained in the SDK
itself). This is non-breaking, we just ignore the provider's own state
now
- add RECONCILING events and state, fire related events with provider's
`on context change`
- add FATAL state

The new tests should help you understand the impact of the changes.

---------

Signed-off-by: Todd Baert <[email protected]>
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.

Proposal to move Provider Status field from provider to SDK, refine semantics around ContextChange events