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.

Sign up for GitHub

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

Jump to bottom

Add events of the OpenFeature into the provider #45

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Add events of the OpenFeature into the provider #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a509d5
add events of the openfeature into the provider, update non suspendin…
vahidlazio Jul 24, 2023
1fc3c18
fix demo app, integrate with wait for provider being ready
vahidlazio Jul 24, 2023
02afc24
update tests to support provider events
vahidlazio Jul 24, 2023
d770f73
update tests to wait for provider to be ready
vahidlazio Jul 24, 2023
1d98d35
Merge branch 'main' into update-openfeature-events
vahidlazio Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions ConfidenceDemoApp/src/main/java/com/example/confidencedemoapp/MainVm.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import dev.openfeature.sdk.FlagEvaluationDetails
import dev.openfeature.sdk.ImmutableContext
import dev.openfeature.sdk.OpenFeatureAPI
import dev.openfeature.sdk.Value
import dev.openfeature.sdk.async.awaitProviderReady
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes the life of provider devs much easier. but we can always remove it.

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import java.util.UUID

class MainVm(app: Application) : AndroidViewModel(app) {
Expand All @@ -33,20 +36,22 @@ class MainVm(app: Application) : AndroidViewModel(app) {
init {
val start = System.currentTimeMillis()
val clientSecret = ClientSecretProvider.clientSecret()
OpenFeatureAPI.setProvider(
ConfidenceFeatureProvider.create(
app.applicationContext,
clientSecret
),
initialContext = ctx
)
client = OpenFeatureAPI.getClient()
viewModelScope.launch {
OpenFeatureAPI.setProvider(
ConfidenceFeatureProvider.create(
app.applicationContext,
clientSecret
),
initialContext = ctx
)
withContext(Dispatchers.IO) {
awaitProviderReady()
}
Log.d(TAG, "client secret is $clientSecret")
Log.d(TAG, "init took ${System.currentTimeMillis() - start} ms")
refreshUi()
}
client = OpenFeatureAPI.getClient()

}

fun refreshUi() {
Expand Down
2 changes: 1 addition & 1 deletion Provider/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ plugins {
}

object Versions {
const val openFeatureSDK = "v0.1.1"
const val openFeatureSDK = "v0.1.3"
const val okHttp = "4.10.0"
const val kotlinxSerialization = "1.5.1"
const val coroutines = "1.7.1"
Expand Down
50 changes: 36 additions & 14 deletions Provider/src/main/java/dev/openfeature/contrib/providers/ConfidenceFeatureProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,37 +18,57 @@ import dev.openfeature.sdk.ProviderEvaluation
import dev.openfeature.sdk.ProviderMetadata
import dev.openfeature.sdk.Reason
import dev.openfeature.sdk.Value
import dev.openfeature.sdk.events.EventHandler
import dev.openfeature.sdk.events.EventsPublisher
import dev.openfeature.sdk.events.OpenFeatureEvents
import dev.openfeature.sdk.exceptions.OpenFeatureError.FlagNotFoundError
import dev.openfeature.sdk.exceptions.OpenFeatureError.InvalidContextError
import dev.openfeature.sdk.exceptions.OpenFeatureError.ParseError
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.launch

@Suppress(
"TooManyFunctions",
"LongParameterList"
)
class ConfidenceFeatureProvider private constructor(
override val hooks: List<Hook<*>>,
override val metadata: ProviderMetadata,
private val cache: ProviderCache,
private val client: ConfidenceClient,
private val flagApplier: FlagApplier,
private val dispatcher: CoroutineDispatcher
private val eventsPublisher: EventsPublisher,
dispatcher: CoroutineDispatcher
) : FeatureProvider {
override suspend fun initialize(initialContext: EvaluationContext?) {
private val coroutineScope = CoroutineScope(SupervisorJob() + dispatcher)
private val networkExceptionHandler by lazy {
CoroutineExceptionHandler { _, _ ->
// network failed, provider is ready but with default/cache values
eventsPublisher.publish(OpenFeatureEvents.ProviderReady)
Comment on lines +51 to +52
Copy link
Member

@fabriziodemaria fabriziodemaria Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}
override fun initialize(initialContext: EvaluationContext?) {
if (initialContext == null) return
withContext(dispatcher) {
try {
val resolveResponse = client.resolve(listOf(), initialContext)
if (resolveResponse is ResolveResponse.Resolved) {
val (flags, resolveToken) = resolveResponse.flags
cache.refresh(flags.list, resolveToken, initialContext)
}
} catch (_: Throwable) {
// Can't refresh cache at this time. Do we retry at a later time?
coroutineScope.launch(networkExceptionHandler) {
val resolveResponse = client.resolve(listOf(), initialContext)
if (resolveResponse is ResolveResponse.Resolved) {
val (flags, resolveToken) = resolveResponse.flags
cache.refresh(flags.list, resolveToken, initialContext)
eventsPublisher.publish(OpenFeatureEvents.ProviderReady)
}
}
}

override suspend fun onContextSet(
override fun shutdown() {
coroutineScope.cancel()
}

override fun onContextSet(
Copy link
Member

Choose a reason for hiding this comment

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

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.

oldContext: EvaluationContext?,
newContext: EvaluationContext
) {
Expand Down Expand Up @@ -163,7 +183,7 @@ class ConfidenceFeatureProvider private constructor(
private class ConfidenceMetadata(override var name: String? = "confidence") : ProviderMetadata

@Suppress("LongParameterList")
suspend fun create(
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to have a "create" method not suspend 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fun create(
context: Context,
clientSecret: String,
region: ConfidenceRegion = ConfidenceRegion.EUROPE,
Expand All @@ -172,6 +192,7 @@ class ConfidenceFeatureProvider private constructor(
metadata: ProviderMetadata = ConfidenceMetadata(),
cache: ProviderCache? = null,
flagApplier: FlagApplier? = null,
eventsPublisher: EventsPublisher = EventHandler.eventsPublisher(Dispatchers.IO),
dispatcher: CoroutineDispatcher = Dispatchers.IO
): ConfidenceFeatureProvider {
val configuredClient = client ?: ConfidenceRemoteClient(
Expand All @@ -191,6 +212,7 @@ class ConfidenceFeatureProvider private constructor(
cache = cache ?: StorageFileCache.create(context),
client = configuredClient,
flagApplier = flagApplierWithRetries,
eventsPublisher,
dispatcher
)
}
Expand Down
6 changes: 2 additions & 4 deletions Provider/src/main/java/dev/openfeature/contrib/providers/cache/StorageFileCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import dev.openfeature.sdk.EvaluationContext
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import java.io.File
import kotlin.coroutines.resume
import kotlin.coroutines.suspendCoroutine

const val FLAGS_FILE_NAME = "confidence_flags_cache.json"

Expand Down Expand Up @@ -43,10 +41,10 @@ class StorageFileCache private constructor(context: Context) : InMemoryCache() {
}

companion object {
suspend fun create(context: Context): StorageFileCache = suspendCoroutine {
Copy link
Member

Choose a reason for hiding this comment

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

Curious about removing suspend here: reading file could be a slow operation, do we really want to make create blocking on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fun create(context: Context): StorageFileCache {
val storage = StorageFileCache(context)
storage.readFile()
it.resume(storage)
return storage
}
}
}
19 changes: 17 additions & 2 deletions Provider/src/test/java/dev/openfeature/contrib/providers/ConfidenceFeatureProviderTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import dev.openfeature.sdk.ImmutableContext
import dev.openfeature.sdk.ImmutableStructure
import dev.openfeature.sdk.Reason
import dev.openfeature.sdk.Value
import dev.openfeature.sdk.async.awaitProviderReady
import dev.openfeature.sdk.exceptions.OpenFeatureError.FlagNotFoundError
import dev.openfeature.sdk.exceptions.OpenFeatureError.ParseError
import junit.framework.TestCase.assertEquals
Expand Down Expand Up @@ -504,7 +505,13 @@ internal class ConfidenceFeatureProviderTests {
runBlocking {
confidenceFeatureProvider.initialize(ImmutableContext("foo"))
}
val evalRootObject = confidenceFeatureProvider.getObjectEvaluation("fdema-kotlin-flag-1", Value.Structure(mapOf()), ImmutableContext("foo"))
awaitProviderReady()
val evalRootObject = confidenceFeatureProvider
.getObjectEvaluation(
"fdema-kotlin-flag-1",
Value.Structure(mapOf()),
ImmutableContext("foo")
)

assertEquals(resolvedValueAsMap, evalRootObject.value.asStructure())
assertEquals(Reason.TARGETING_MATCH.toString(), evalRootObject.reason)
Expand Down Expand Up @@ -615,7 +622,13 @@ internal class ConfidenceFeatureProviderTests {
confidenceFeatureProvider.initialize(ImmutableContext("user1"))
}

val evalString = confidenceFeatureProvider.getStringEvaluation("fdema-kotlin-flag-1.mystring", "default", ImmutableContext("user1"))
awaitProviderReady()
val evalString = confidenceFeatureProvider
.getStringEvaluation(
"fdema-kotlin-flag-1.mystring",
"default",
ImmutableContext("user1")
)

assertNull(evalString.errorMessage)
assertNull(evalString.errorCode)
Expand Down Expand Up @@ -699,6 +712,7 @@ internal class ConfidenceFeatureProviderTests {
runBlocking {
confidenceFeatureProvider.initialize(ImmutableContext("user2"))
}
awaitProviderReady()
val ex = assertThrows(ParseError::class.java) {
confidenceFeatureProvider.getStringEvaluation(
"fdema-kotlin-flag-1.wrongid",
Expand All @@ -721,6 +735,7 @@ internal class ConfidenceFeatureProviderTests {
runBlocking {
confidenceFeatureProvider.initialize(ImmutableContext("user2"))
}
awaitProviderReady()
val ex = assertThrows(ParseError::class.java) {
confidenceFeatureProvider.getStringEvaluation(
"fdema-kotlin-flag-1.mystring.extrapath",
Expand Down
55 changes: 34 additions & 21 deletions Provider/src/test/java/dev/openfeature/contrib/providers/ConfidenceIntegrationTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import dev.openfeature.sdk.ImmutableContext
import dev.openfeature.sdk.OpenFeatureAPI
import dev.openfeature.sdk.Reason
import dev.openfeature.sdk.Value
import dev.openfeature.sdk.async.awaitProviderReady
import dev.openfeature.sdk.events.EventHandler
import dev.openfeature.sdk.events.OpenFeatureEvents
import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
Expand All @@ -27,26 +30,34 @@ class ConfidenceIntegrationTests {
@Before
fun setup() {
whenever(mockContext.filesDir).thenReturn(Files.createTempDirectory("tmpTests").toFile())
EventHandler.eventsPublisher().publish(OpenFeatureEvents.ProviderShutDown)
}

@Test
fun testSimpleResolveInMemoryCache() {
runBlocking {
OpenFeatureAPI.setProvider(
ConfidenceFeatureProvider.create(mockContext, clientSecret, cache = InMemoryCache()),
ImmutableContext(
targetingKey = UUID.randomUUID().toString(),
attributes = mutableMapOf(
"user" to Value.Structure(
mapOf(
"country" to Value.String("SE")
)
EventHandler.eventsPublisher().publish(OpenFeatureEvents.ProviderShutDown)
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

OpenFeatureAPI.setProvider(
ConfidenceFeatureProvider.create(mockContext, clientSecret, cache = InMemoryCache()),
ImmutableContext(
targetingKey = UUID.randomUUID().toString(),
attributes = mutableMapOf(
"user" to Value.Structure(
mapOf(
"country" to Value.String("SE")
)
)
)
)
)
runBlocking {
awaitProviderReady()
}
val intDetails = OpenFeatureAPI.getClient().getIntegerDetails("test-flag-1.my-integer", 0)

val intDetails = OpenFeatureAPI.getClient()
.getIntegerDetails(
"test-flag-1.my-integer",
0
)
assertNull(intDetails.errorCode)
assertNull(intDetails.errorMessage)
assertNotNull(intDetails.value)
Expand All @@ -59,21 +70,23 @@ class ConfidenceIntegrationTests {
fun testSimpleResolveStoredCache() {
val cacheFile = File(mockContext.filesDir, FLAGS_FILE_NAME)
assertEquals(0L, cacheFile.length())
runBlocking {
OpenFeatureAPI.setProvider(
ConfidenceFeatureProvider.create(mockContext, clientSecret),
ImmutableContext(
targetingKey = UUID.randomUUID().toString(),
attributes = mutableMapOf(
"user" to Value.Structure(
mapOf(
"country" to Value.String("SE")
)
OpenFeatureAPI.setProvider(
ConfidenceFeatureProvider.create(mockContext, clientSecret),
ImmutableContext(
targetingKey = UUID.randomUUID().toString(),
attributes = mutableMapOf(
"user" to Value.Structure(
mapOf(
"country" to Value.String("SE")
)
)
)
)
)
runBlocking {
awaitProviderReady()
}

assertNotEquals(0L, cacheFile.length())
val intDetails = OpenFeatureAPI.getClient().getIntegerDetails("test-flag-1.my-integer", 0)
assertNull(intDetails.errorCode)
Expand Down
3 changes: 2 additions & 1 deletion Provider/src/test/java/dev/openfeature/contrib/providers/StorageFileCacheTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import dev.openfeature.sdk.ImmutableContext
import dev.openfeature.sdk.ImmutableStructure
import dev.openfeature.sdk.Reason
import dev.openfeature.sdk.Value
import dev.openfeature.sdk.async.awaitProviderReady
import junit.framework.TestCase
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.runTest
Expand Down Expand Up @@ -70,8 +71,8 @@ class StorageFileCacheTests {
)
runBlocking {
provider1.initialize(ImmutableContext(targetingKey = "user1"))
awaitProviderReady()
}

// Simulate offline scenario
whenever(mockClient.resolve(eq(listOf()), any())).thenThrow(Error())
// Create new cache to force reading cache data from storage
Expand Down