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!: Spec v0.8 adherence #46

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

feat!: Spec v0.8 adherence #46

wants to merge 9 commits into from

Conversation

fabriziodemaria
Copy link
Contributor

@fabriziodemaria fabriziodemaria commented Sep 4, 2024

Spec v0.8 adherence

  • Created "ProviderStatus.swift" according to Specification.
    • ProviderState is now maintained by the SDK, and not by the Provider implementations
  • Align "ProviderEvent.swift" with Specification
    • Basic events like Ready, Error or Fatal are emitted by the SDK and are not expected by the Provider implementations.
  • Makes initialize and onContextChange protocols throwing, allowing for the OpenFeatureAPI layer to detect cases where Error events should be emitted
  • Makes initialize and onContextChange protocols async for an easier Provider implementation (in most cases)

Missing parts

  • From the Specification: It's possible that the on context changed function is invoked simultaneously or in quick succession; in this case the SDK will only run the PROVIDER_CONTEXT_CHANGED handlers after all reentrant invocations have terminated, and the last to terminate was successful (terminated normally)
    • This is not part of this PR: subsequent calls are serialized and executed in sequence, each returning from its execution even if more same functions are queued
  • Handling of spontaneous Provider events missing in this PR

Changes unrelated to Spec v0.8

  • Added setEvaluationContextAndWait to provide the same better ergonomics that setProviderAndWait already offers
  • Evaluation returns early if provider state is not "ready"
  • Refactor afterAll hook to adhere to latest conventions

Notes on backwards compatibility

  • A backwards incompatible aspect of this change is that the initialize() implementation needs to throw in case of errors, rather than emitting the ERROR event. The latter is now responsibility of the SDK. This is also valid for onContextChange.
  • This change in semantics is not enforced with code, and it might be easy for the Provider implementer to make mistakes when adopting this new version of the SDK (e.g. forget to remove ERROR emission from the Provider side)

Example Adoption

The Confidence OpenFeature Provider adopting this version of the SDK is in the works here: spotify/confidence-sdk-swift#184

@fabriziodemaria fabriziodemaria force-pushed the fatal-state branch 3 times, most recently from 203e8da to 40d0812 Compare September 6, 2024 14:41
@fabriziodemaria fabriziodemaria changed the title feat: Add ProviderState feat!: Add ProviderState Sep 6, 2024
@fabriziodemaria fabriziodemaria force-pushed the fatal-state branch 3 times, most recently from 16499c7 to 78a30e5 Compare December 17, 2024 14:53
@fabriziodemaria fabriziodemaria marked this pull request as ready for review December 17, 2024 14:53
@fabriziodemaria fabriziodemaria force-pushed the fatal-state branch 3 times, most recently from ba7c072 to ac15098 Compare December 18, 2024 16:30
@fabriziodemaria fabriziodemaria changed the title feat!: Add ProviderState feat!: Start adoption of Spec v0.8 Dec 18, 2024
@fabriziodemaria fabriziodemaria force-pushed the fatal-state branch 2 times, most recently from 76cd647 to eacbb12 Compare December 19, 2024 09:46
@fabriziodemaria fabriziodemaria changed the title feat!: Start adoption of Spec v0.8 feat!: Spec v0.8 adherence Dec 19, 2024
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Hey @fabriziodemaria, the change looks good from what I can tell (limited experience with Swift). Since this is a breaking change, it may be a good time to update the afterAll stage in the hooks to comply with a recent update to the spec.

https://openfeature.dev/specification/sections/hooks#requirement-438

This will make it easy to support more sophisticated telemetry use cases.

Sources/OpenFeature/OpenFeatureAPI.swift Outdated Show resolved Hide resolved
Sources/OpenFeature/OpenFeatureAPI.swift Outdated Show resolved Hide resolved
Sources/OpenFeature/OpenFeatureAPI.swift Outdated Show resolved Hide resolved
Sources/OpenFeature/OpenFeatureClient.swift Outdated Show resolved Hide resolved
fabriziodemaria and others added 4 commits December 20, 2024 14:25
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Fabrizio Demaria <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Fabrizio Demaria <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Fabrizio Demaria <[email protected]>
Signed-off-by: Fabrizio Demaria <[email protected]>
Comment on lines +21 to +30
/**
Set provider and calls its `initialize` in a background thread.
Readiness can be determined from `getState` or listening for `ready` event.
*/
public func setProvider(provider: FeatureProvider, initialContext: EvaluationContext?) {
queue.async {
Task {
await self.setProviderInternal(provider: provider, initialContext: initialContext)
}
}
Copy link
Contributor Author

@fabriziodemaria fabriziodemaria Dec 20, 2024

Choose a reason for hiding this comment

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

One limitation that came up during manual testing on a sample app

Now the OpenFeature SDK only offers two APIs to set a Provider, and both require await from the Application: await for an async function (i.e. setProviderAndAwait) or await for a .ready event.

If a Provider offers local-resolve or cached flags data, it would be useful to offer a non-await (i.e. synchronous) function to set the Provider and have it "ready" right away: synchronous functions are easier to integrate in the Application (especially at startup, when latency is critical).

One possibility could be to make setProvider blocking until initialize (which is called inside "setProviderInternal") is finished:

    public func setProvider(provider: FeatureProvider, initialContext: EvaluationContext?) {
        let semaphore = DispatchSemaphore(value: 0)
        let _ = queue.sync {
            Task {
                await self.setProviderInternal(provider: provider, initialContext: initialContext)
                semaphore.signal()
            }
        }
        semaphore.wait()
    }

A more robust solution would be to have Providers implement a non-async initialize for the synchronous case, and an async initialize for when network operations are required.

This will require a bit more thinking, and I suggest we refrain from merging until we have a more clear idea on how we want to tackle this

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.

3 participants