Add events of the OpenFeature into the provider #45
Conversation
vahidlazio
commented
Jul 24, 2023
vahidlazio
commented
- Add events of the OpenFeature into the provider
- Update non suspending provider
vahidlazio
force-pushed
the
update-openfeature-events
branch
from
f141ced
to
d770f73
Compare
| coroutineScope.cancel() | ||
| } | ||
|
|
||
| override fun onContextSet( |
There was a problem hiding this comment.
I wonder if we should emit "ProviderStale" if newContext != oldContext, and until the flags for the newContext are fetched. But this is something we can think about in a future PR.
| @@ -163,7 +183,7 @@ class ConfidenceFeatureProvider private constructor( | |||
| private class ConfidenceMetadata(override var name: String? = "confidence") : ProviderMetadata | |||
|
|
|||
| @Suppress("LongParameterList") | |||
| suspend fun create( | |||
There was a problem hiding this comment.
Sounds good to have a "create" method not suspend 👍
There was a problem hiding this comment.
the problem with this however is that the cache is reading from file in the caller thread now. we should probably make the read from file in the cache explicit. we can discuss this offline.
| // network failed, provider is ready but with default/cache values | ||
| eventsPublisher.publish(OpenFeatureEvents.ProviderReady) |
There was a problem hiding this comment.
I believe in some cases we will want to emit ProviderError here. I think we can merge this as it is for now, but we should perhaps take another look the overall OpenFeature design of Events and make sure they are useful for users to really understand what is happening in the Provider at any time
There was a problem hiding this comment.
this depends on the strategy of the provider. maybe we are fine with the network to fail. if user doesn't have internet they should still be able to use a working provider with flags from the cache. and provider is ready.
| @@ -43,10 +41,10 @@ class StorageFileCache private constructor(context: Context) : InMemoryCache() { | |||
| } | |||
|
|
|||
| companion object { | |||
| suspend fun create(context: Context): StorageFileCache = suspendCoroutine { | |||
There was a problem hiding this comment.
Curious about removing suspend here: reading file could be a slow operation, do we really want to make create blocking on that?
There was a problem hiding this comment.
explained in another comment. we can and i believe we should explicitly call read from file in the provider initialization on a background thread instead of doing it in the init of the cache.
| mapOf( | ||
| "country" to Value.String("SE") | ||
| ) | ||
| EventHandler.eventsPublisher().publish(OpenFeatureEvents.ProviderShutDown) |
There was a problem hiding this comment.
tests are running on the same machine, if they run together and one after another, the next one will fail because provider was already ready in the previous test. we need to clean up the events for the new test case.
| @@ -10,7 +10,10 @@ import androidx.lifecycle.MutableLiveData | |||
| import androidx.lifecycle.viewModelScope | |||
| import dev.openfeature.contrib.providers.ConfidenceFeatureProvider | |||
| import dev.openfeature.sdk.* | |||
| import dev.openfeature.sdk.async.awaitProviderReady | |||
There was a problem hiding this comment.
I am sill concerned about exposing this non-spec-standard API from the base SDK: we should use the Events API directly or try add this API in the OpenFeature Sepc. But this comment is not related to this PR under review
There was a problem hiding this comment.
this makes the life of provider devs much easier. but we can always remove it.
vahidlazio
force-pushed
the
update-openfeature-events
branch
from
53da1f4
to
1d98d35
Compare
