SDK Review

[toddbaert]

I've opened this issue as a place to centralize review of the Kotlin SDK built by Spotify. Thanks to all that have worked on this. The community is excited to have this contribution!

I've gone through the code as best I can (as a Kotlin novice) and here are my observations so far:

Spec compliance

These are the things that seem to not yet be implemented from a specification perspective. We may not need to implement all of them for a 1.0. I also could be misreading (again, Kotlin novice) so please correct me if I've missed things.

FlagMetadata

• FlagMetadata is missing on the FlagEvaluationDetails and ProviderEvaluation (this was probably merged after work on the SDK was started, it was a small and recent change). It is pretty easy to implement. See related point below.

Multiple provider support

• This is not implemented, see here, here. There's some additional background here.

Provider initialization/Shutdown

• This is partially implemented.
  • The provider interface needs a "shutdown" function to clean up timers, threads, connections, etc. See here, and here
  • The API needs a shutdown function to shut down all providers gracefully when the application terminates, see here.
  • This gets a bit more complex with multiple providers

Named provider (aka multi-provider) support

• This is not implemented, see here, here. There's some additional background here.

Events

• This is not implemented, see here.
• Combined with named provider (aka multi-provider) support, this can be quite tricky to implement, and in fact it's only yet implemented fully by the JS SDK, though the Go and Java SDKs have PRs open to add this feature.
• Events allow applications to react to flag changes without user-action. This can be important in web use-cases, I'm interested in whether or others (especially contributors to this SDK) think it's useful for mobile application use-cases.

General recommendations and observations

These are things that aren't really about spec compliance, but general points I've noticed:

Value

• This looks similar to the Java and .NET impls, which is good, but I'm wondering if there's a "simpler" alternative for representing arbitrary structured data in a type-safe, serializable way in Kotlin. For example, in TS/JS this is just a plain object with the type restrictions. In Go it's a map[string][interface{}]. I would recommend whatever the "standard" way of representing a JSON object is in Kotlin (in Java, there's a myriad of ways to do this, none of which are standard to the runtime, which is why we made the choice we did).

EvaluationContext, MutableContext, MutableStructure

• You may want to consider making these immutable. I think in Java, we regretted a bit making them mutable. Especially in a multi-threaded context, it adds some difficulty.

Metadata

• I recommend calling this ProviderMetadata or something else. We regretted this naming in other SDKs, since we found number use-cases for a generic Metadata object that carries arbitrary information (FlagMetadata, and EventMetadata, for instance).

OpenFeatureClient:

• Something the .NET and JS implementations do is to use a method ref or delegate function to remove the switch case from the flag evaluation. It also might have with the casting issue.

setProvider is a "suspend function"

• I'm not too familiar with suspend, but I suspect that if events are implemented, this might not be ideal. In other SDK implementations, the the function returns immediately and is not async or awaitible. To deal with the fact that provider initialization that must happen inside is asynchronous, clients and the API can have PROVIDER_READY event handlers added to them in order to know when the provider is ready to be used. See points here.

Proposal

Overall the SDK looks really good. :partying_face: I think we need to decide which of the above things we want to address before a release (especially to avoid breaking changes). Once we agree on a what would constitute a first release, we can create issues to implement that, and then move the code into the OpenFeature org and do the implementation there.

I would love to hear input from the contributors from Spotify and the OpenFeature community in general on my proposal above.

[toddbaert]

@alina-v1 @Calibretto @fabriziodemaria @nicklasl

[fabriziodemaria]

Thanks for the thorough review!

Regarding the Spec Compliance section, I agree there is still a lot of work to be done in terms of missing functionalities. However, my understanding is that none of the listed points is a requirement for the initial release.

On the General Observation part:

• I opened two PRs to address data structures immutability (PR) and Metadata naming (PR)
• On the Value implementation, I am going to involve someone more expert than me in Kotlin to look into it from our side
• Regarding the setProvider point: if we assume we won't have proper Events support for the initial release, we are left with limited options: either keep initialize async, or asking the Providers to expose their "status" in some way that doesn't rely on Events. How can we align on the right approach for this?
• Finally, the OpenFeatureClient suggestion seems like an internal detail that we can improve on after the release without breaking changes for consumers

[toddbaert]

Regarding the Spec Compliance section, I agree there is still a lot of work to be done in terms of missing functionalities. However, my understanding is that none of the listed points is a requirement for the initial release.

Yes. However:

Regarding the setProvider point: if we assume we won't have proper Events support for the initial release, we are left with limited options: either keep initialize async, or asking the Providers to expose their "status" in some way that doesn't rely on Events. How can we align on the right approach for this?

This is my only real concern about releasing this "as-is", because if we want to be consistent with the other SDKs, when we eventually release events, setProvider will go from async to sync (returning immediately), which would be breaking IIUC.

I think we have to find some plan for this. I'll think about this a bit more.

[toddbaert]

After more consideration, I feel that we could release this as is, as a sub-1.0 version, especially since it looks like this week we will merge the static/dynamic context spec PR.

It would be my preference to have some kind of solution for the last point in my write-up (setProvider). A couple ideas:

• implement some limited event functionality
• have setProviderAsync or something like that, which would behave like the current implementation - basically an alternative async API, as well as a setProvider method that immediately returns
• expose the provider's status some other way

[vahidlazio]

@toddbaert the suspending provider doesn't launch any coroutines on its own and it blocks the execution of the next line. suspending here means the other threads can go on if something in the background needs IO. we can also make it not suspending with minimal effort but we have to initialize the parent block with a coroutine which has the same effect.

if we want to solve this properly without having stale values for flags, we need to introduce limited events stream functionality for storage.

[toddbaert]

@toddbaert the suspending provider doesn't launch any coroutines on its own and it blocks the execution of the next line. suspending here means the other threads can go on if something in the background needs IO. we can also make it not suspending with minimal effort but we have to initialize the parent block with a coroutine which has the same effect.

Yes, I think I understand the nature of suspend though I've only been introduced to it recently, so I could be off. What I was trying to say, is that it may be possible to expose 2 versions of setProvider (though they may need different names) one that suspends, and one that does not. The one that does not would be more consistent with the other SDKs. I'm not sure this is a great solution though, as it might only add to confusion in the end.

Before the OpenFeature specification had initialization and shutdown features, to ensure the providers we "ready to go" and any I/O had been done, users simply initialized their providers manually, and then used setProvider. In other words, we expected providers to be ready (each provider did that differently). This wasn't ideal, and it's why we added the initialization/shutdown logic, but it was "good enough". We could use the same pattern in the Kotlin SDK for now, as another option. I think that would be fine for a pre-1.0 version, especially since it's how OpenFeature worked in the past.

if we want to solve this properly without having stale values for flags, we need to introduce limited events stream functionality for storage.

Yes, I think a minimal events implementation would probably be optimal, that way setProvider can return immediately, and users can listen for the PROVIDER_READY event to know when to start evaluating flags... but there's some additional work associated with going that route.

[vahidlazio]

@toddbaert We discussed about a proposal for how openfeature should handle events and came up with this draft. currently there are 2 events here but more events can be added. like ContextChanged etc. there is also an utility wrapper & extension created so the consumers can use to get updates of flags.

[toddbaert]

@vahidlazio apologies since I didn't link this before; OpenFeature already has an events specification, see here. I was proposing a partial implementation of this to solve our problem.

Put very simply, this allows you to subscribe to things like the provider being initialized (PROVIDER_READY) so that users can be sure they don't evaluate flags before the provider is in a usable state. It also allows application authors to subscribe to changes in flags (if the provider supports it). It's very similar to paradigms from many popular vendor feature flag SDKs.

Here are some simple pseudo-code examples:

// don't evaluate flags until provider is ready
OpenFeature.setProvider(new MyProvider()); // sets a provider (without waiting for it to initialize), returns immiediately
client = OpenFeature.getClient();
// attaches an event handler for when the provider is ready (after initialize() completes)
client.on('PROVIDER_READY', () => {

  // provider is ready now, do something!

}); 

// do something when flags change
client.on('PROVIDER_CONFIGURATION_CHANGED', (flagsChanged) => {

  // some flag values changed!

}); 

If we could support just PROVIDER_READY for example, it might solve this problem.

[vahidlazio]

thanks for linking the specifications, I'v read them already. one point that we are improving with this draft proposal is you are using handlers in the specifications and callbacks but we are using kotlin coroutines approach to make things more kotlin/android/compose friendly. the provider_ready is in the draft proposal and linking with what we have in the other PR about removing the suspending initialization will move things very close to what's expected imo.

[toddbaert]

As discussed in our meeting, I'm in favor of moving forward as is if we agree that the suspend approach doesn't fundamentally conflict with the later addition of the events specification. I'm not familiar enough with Kotlin paradigms to weigh-in on that, so I'll defer to others on it!

[nickybondarenko]

Hi everyone! As discussed, updating a README to align with Open Feature template is our next step: PR can be found here. I've omitted some of the parts that we haven't implemented yet (like support multi-provider). Posting it here for visibility.

[nickybondarenko]

Hello again! I wanted to let you all know that the README PR has been merged 💪 We would love to have some feedback on it and see whether the SDK looks good for us to move to Open Feature repo - WDYT?

[beeme1mr]

Hey @nickybondarenko, thanks for the update. I'll check it out later today.

By the way, I'm working on improving the readme template. Here's a draft PR showing the proposal. If this new format is approved, I'll update the template and help update all the SDK readmes. If you can, please check out the proposal and let me know what you think.

[beeme1mr]

Hey @nickybondarenko, the readme looks good! I think we can move the project to the OpenFeature org. @toddbaert any concerns?

[toddbaert]

I've reviewed the README and I agree. It looks great. I've also been looking into a few more Kotlin APIs and I think this strikes a good balance between being "Kotlin-ish" and feeling consistent with other OpenFeature SDKs.

I propose moving the contents into this repo next week, and providing maintainer access to the appropriate contributors... I'll be relying on @nickybondarenko and @fabriziodemaria to know who all those people are, but it's all configured in the community repo.

[fabriziodemaria]

@toddbaert that sounds great! Can we get some help in configuring what is necessary in the community repo? This kotlin-sdk repo is not mentioned there yet, and I currently can't fork it or push to it; once we figure this part out, I am happy to open a PR here with the forklifted code form the Spotify organization.

Regarding the contributors, we can start with the accounts mentioned here.

[toddbaert]

@toddbaert that sounds great! Can we get some help in configuring what is necessary in the community repo? This kotlin-sdk repo is not mentioned there yet, and I currently can't fork it or push to it; once we figure this part out, I am happy to open a PR here with the forklifted code form the Spotify organization.

Regarding the contributors, we can start with the accounts mentioned here.

Yep! I will create the PRs to add what we need there, and you can amend them with the personnel we need. I'll ping you on the PR.

[toddbaert]

See PR here.

[toddbaert]

The management PR is merged, and the repo permissions look good.

Maintainers can feel free to merge the SDK into that repo whenever. Just be careful not to unintentionally release anything you don't mean to ! :sweat_smile: If you have issues or need anything from org admins (secrets, config changes, etc, please ping me, or @beeme1mr or anyone else on the TC/GC)

I'll be creating issues in the upcoming days/weeks for some housekeeping items, and maintainers should feel free to do the same.

Thanks again to all who helped with this! We can also close this issue whenever if it's no longer useful.