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

feat: split event listeners #26

Merged
merged 3 commits into from
Jul 17, 2024
Merged

feat: split event listeners #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09d07d6
feat: fine grained event listeners
gastonfournier Jul 17, 2024
cb5b85c
Cleanup logs
gastonfournier Jul 17, 2024
5a374db
Remove reference to old listeners
gastonfournier Jul 17, 2024
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
4 changes: 2 additions & 2 deletions app/src/main/java/io/getunleash/unleashandroid/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.ViewModel
import io.getunleash.android.Unleash
import io.getunleash.android.events.UnleashEventListener
import io.getunleash.android.events.UnleashStateListener
import java.util.Date
import java.util.Timer
import kotlin.concurrent.timerTask
Expand Down Expand Up @@ -125,7 +125,7 @@ class IsEnabledViewModel(initialFlag: String): ViewModel() {
get() = _isEnabled

fun initialize(unleash: Unleash) {
val listener: UnleashEventListener = object: UnleashEventListener {
val listener: UnleashStateListener = object: UnleashStateListener {
override fun onStateChanged() {
Log.i("MAIN", "Detected refresh event")
val flag = _flagName.value
Expand Down
5 changes: 3 additions & 2 deletions app/src/main/java/io/getunleash/unleashandroid/TestApplication.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import io.getunleash.android.Unleash
import io.getunleash.android.UnleashConfig
import io.getunleash.android.data.DataStrategy
import io.getunleash.android.data.UnleashContext
import io.getunleash.android.events.UnleashEventListener
import io.getunleash.android.events.UnleashReadyListener
import io.getunleash.android.events.UnleashStateListener
import java.util.Date

const val initialFlagValue = "flag-1"
Expand Down Expand Up @@ -35,7 +36,7 @@ class TestApplication: Application() {
),
unleashContext = unleashContext
)
instance.start(listOf(object: UnleashEventListener {
instance.start(listOf(object: UnleashReadyListener, UnleashStateListener {
override fun onReady() {
println("Unleash is ready")
UnleashStats.readySince = Date()
Expand Down
40 changes: 22 additions & 18 deletions unleashandroidsdk/src/main/java/io/getunleash/android/DefaultUnleash.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ import io.getunleash.android.data.Toggle
import io.getunleash.android.data.UnleashContext
import io.getunleash.android.data.UnleashState
import io.getunleash.android.data.Variant
import io.getunleash.android.events.UnleashEventListener
import io.getunleash.android.events.UnleashImpressionEventListener
import io.getunleash.android.events.UnleashListener
import io.getunleash.android.events.UnleashReadyListener
import io.getunleash.android.events.UnleashStateListener
import io.getunleash.android.http.ClientBuilder
import io.getunleash.android.http.NetworkStatusHelper
import io.getunleash.android.metrics.MetricsCollector
Expand All @@ -35,7 +38,6 @@ import kotlinx.coroutines.channels.BufferOverflow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.filter
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.takeWhile
import kotlinx.coroutines.launch
Expand All @@ -58,7 +60,7 @@ class DefaultUnleash(
private val unleashConfig: UnleashConfig,
unleashContext: UnleashContext = UnleashContext(),
cacheImpl: ToggleCache = InMemoryToggleCache(),
eventListeners: List<UnleashEventListener> = emptyList(),
eventListeners: List<UnleashListener> = emptyList(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the ability of sending multiple event listeners instead of just one

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, Both List<UnleashEventListener> and List<UnleashListener>, allow you to send more than one though? They accept a list of listeners. So the real change here is a rename of the Interface?

Copy link
Contributor Author

@gastonfournier gastonfournier Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, previously it was just an event listener but the change from 1 to many was done in a previous PR, this comment is stale :D currently, only changing the interface

private val lifecycle: Lifecycle = getLifecycle(androidContext),
private val coroutineScope: CoroutineScope = unleashScope
) : Unleash {
Expand All @@ -72,11 +74,11 @@ class DefaultUnleash(
private val cache: ObservableToggleCache = ObservableCache(cacheImpl, coroutineScope)
private var started = AtomicBoolean(false)
private var ready = AtomicBoolean(false)
private val readyFlow = MutableStateFlow(false)
private val fetcher: UnleashFetcher?
private val networkStatusHelper = NetworkStatusHelper(androidContext)
private val impressionEventsFlow = MutableSharedFlow<ImpressionEvent>(
extraBufferCapacity = 64,
replay = 1,
extraBufferCapacity = 1000,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is too much, or maybe not. This is just to make sure we don't drop impression events, but we also protect the memory.

Replay turns out to be useful for predictability, because sometimes the listeners don't get to register before the first event happens (same as with isReady flow). Arguably, maybe a replay is not needed for impression events

onBufferOverflow = BufferOverflow.DROP_OLDEST
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may miss some updates if you're not fast enough

)

Expand Down Expand Up @@ -109,14 +111,13 @@ class DefaultUnleash(
}

fun start(
eventListeners: List<UnleashEventListener> = emptyList(),
eventListeners: List<UnleashListener> = emptyList(),
bootstrap: List<Toggle> = emptyList()
) {
if (!started.compareAndSet(false, true)) {
Log.w(TAG, "Unleash already started, ignoring start call")
return
}
eventListeners.forEach { addUnleashEventListener(it) }
networkStatusHelper.registerNetworkListener(taskManager)
if (unleashConfig.localStorageConfig.enabled) {
val localBackup = getLocalBackup()
Expand All @@ -127,6 +128,7 @@ class DefaultUnleash(
cache.subscribeTo(it.getFeaturesReceivedFlow())
}
lifecycle.addObserver(taskManager)
eventListeners.forEach { addUnleashEventListener(it) }
if (bootstrap.isNotEmpty()) {
Log.i(TAG, "Using provided bootstrap toggles")
cache.write(UnleashState(unleashContextState.value, bootstrap.associateBy { it.name }))
Expand Down Expand Up @@ -238,23 +240,25 @@ class DefaultUnleash(
unleashContextState.value = context
}

override fun addUnleashEventListener(listener: UnleashEventListener) {
coroutineScope.launch {
val firstReady = readyFlow.asStateFlow().filter { it }.first()
Log.d(TAG, "Ready state changed to $firstReady, notifying $listener")
override fun addUnleashEventListener(listener: UnleashListener) {

if (listener is UnleashReadyListener) coroutineScope.launch {
cache.getUpdatesFlow().first{
true
}
if (ready.compareAndSet(false, true)) {
Log.d(TAG, "Unleash state changed to ready")
}
Log.d(TAG, "Notifying UnleashReadyListener")
listener.onReady()
}
coroutineScope.launch {
if (listener is UnleashStateListener) coroutineScope.launch {
cache.getUpdatesFlow().collect {
if (ready.compareAndSet(false, true)) {
Log.d(TAG, "Unleash is now ready")
readyFlow.value = true
}
Log.d(TAG, "Cache updated, notifying $listener that state changed")
listener.onStateChanged()
}
}
coroutineScope.launch {

if (listener is UnleashImpressionEventListener) coroutineScope.launch {
impressionEventsFlow.collect { event ->
listener.onImpression(event)
}
Expand Down
4 changes: 2 additions & 2 deletions unleashandroidsdk/src/main/java/io/getunleash/android/Unleash.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package io.getunleash.android

import io.getunleash.android.data.UnleashContext
import io.getunleash.android.data.Variant
import io.getunleash.android.events.UnleashEventListener
import io.getunleash.android.events.UnleashListener
import java.io.Closeable

val disabledVariant = Variant("disabled")
Expand All @@ -27,7 +27,7 @@ interface Unleash: Closeable {
*/
fun setContextAsync(context: UnleashContext)

fun addUnleashEventListener(listener: UnleashEventListener)
fun addUnleashEventListener(listener: UnleashListener)

/**
* This function forces a refresh of the toggles from the server and wait until the refresh is complete or failed.
Expand Down
15 changes: 9 additions & 6 deletions unleashandroidsdk/src/main/java/io/getunleash/android/cache/ObservableCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import io.getunleash.android.data.UnleashState
import io.getunleash.android.unleashScope
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.channels.BufferOverflow
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.asSharedFlow
Expand All @@ -17,7 +18,10 @@ class ObservableCache(private val cache: ToggleCache, private val coroutineScope
private const val TAG = "ObservableCache"
}

private var events = MutableSharedFlow<UnleashState>()
private var newStateEventFlow = MutableSharedFlow<UnleashState>(
replay = 1,
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 cache more predictable. During testing I noticed that the coroutine that was adding a listener to the cache: https://github.com/Unleash/UnleashAndroidSdk/blob/3da55190599b660be58797075410dcc6f69e017b/unleashandroidsdk/src/main/java/io/getunleash/android/DefaultUnleash.kt#L246-L251

was executing after the first new state arrived (this is particularly true when using bootstraped toggles as they are written immediately to the cache). This caused the ready listener to miss the ready event.

With replay = 1 in place, even in this situation, the listener that arrives later will be able to see the first UnleashState received, despite the execution order.

Particularly for state, I believe having the latest always available even for new subscribers is important.

We are also not having a queue of states, just keep the latest and drop the oldest as soon as new states arrive

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me

onBufferOverflow = BufferOverflow.DROP_OLDEST
)
override fun read(): Map<String, Toggle> {
return cache.read()
}
Expand All @@ -28,12 +32,11 @@ class ObservableCache(private val cache: ToggleCache, private val coroutineScope

override fun write(state: UnleashState) {
cache.write(state)
Log.d(TAG, "Done writing cache")
Log.d(TAG, "Done writing cache with ${newStateEventFlow.subscriptionCount.value} subscribers")
coroutineScope.launch {
Log.d(TAG, "Emitting new state with ${state.toggles.size} toggles for ${state.context}")
events.emit(state)
Log.d(TAG, "Emitting new state with ${state.toggles.size} toggles")
newStateEventFlow.emit(state)
}
Log.d(TAG, "Done sending event")
}

override fun subscribeTo(featuresReceived: Flow<UnleashState>) {
Expand All @@ -49,6 +52,6 @@ class ObservableCache(private val cache: ToggleCache, private val coroutineScope
}

override fun getUpdatesFlow(): Flow<UnleashState> {
return events.asSharedFlow()
return newStateEventFlow.asSharedFlow()
}
}
2 changes: 0 additions & 2 deletions unleashandroidsdk/src/main/java/io/getunleash/android/data/FetchResponse.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package io.getunleash.android.data

import java.lang.Exception

/**
* Modelling the fetch action
* @param status Status of the request
Expand Down
16 changes: 16 additions & 0 deletions unleashandroidsdk/src/main/java/io/getunleash/android/events/UnleashAllEventsListener.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package io.getunleash.android.events

import io.getunleash.android.data.ImpressionEvent

interface UnleashListener
interface UnleashReadyListener: UnleashListener {
fun onReady()
}

interface UnleashStateListener: UnleashListener {
fun onStateChanged()
}

interface UnleashImpressionEventListener: UnleashListener {
fun onImpression(event: ImpressionEvent)
}
9 changes: 0 additions & 9 deletions unleashandroidsdk/src/main/java/io/getunleash/android/events/UnleashEventListener.kt
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion unleashandroidsdk/src/main/java/io/getunleash/android/polling/UnleashFetcher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ open class UnleashFetcher(
private val appName = unleashConfig.appName
private var etag: String? = null
private val featuresReceivedFlow = MutableSharedFlow<UnleashState>(
extraBufferCapacity = 1,
replay = 1,
onBufferOverflow = BufferOverflow.DROP_OLDEST
)
private val coroutineContextForContextChange: CoroutineContext = Dispatchers.IO
Expand Down
30 changes: 19 additions & 11 deletions unleashandroidsdk/src/test/java/io/getunleash/android/DefaultUnleashTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import io.getunleash.android.data.ImpressionEvent
import io.getunleash.android.data.Toggle
import io.getunleash.android.data.UnleashContext
import io.getunleash.android.data.UnleashState
import io.getunleash.android.events.UnleashEventListener
import io.getunleash.android.events.UnleashImpressionEventListener
import io.getunleash.android.events.UnleashReadyListener
import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import org.assertj.core.api.Assertions.assertThat
Expand Down Expand Up @@ -95,20 +96,20 @@ class DefaultUnleashTest: BaseTest() {
var onReady3 = false

// adding listeners before start should not be overridden
unleash.addUnleashEventListener(object: UnleashEventListener {
unleash.addUnleashEventListener(object: UnleashReadyListener {
override fun onReady() {
onReady1 = true
}
})

// listeners added at start should also be called
unleash.start(
listOf(object : UnleashEventListener {
listOf(object : UnleashReadyListener {
override fun onReady() {
onReady2 = true
}
},
object : UnleashEventListener {
object : UnleashReadyListener {
override fun onReady() {
onReady3 = true
}
Expand Down Expand Up @@ -157,7 +158,7 @@ class DefaultUnleashTest: BaseTest() {
var ready = false
val impressionEvents = mutableListOf<ImpressionEvent>()
unleash.start(
eventListeners = listOf(object : UnleashEventListener {
eventListeners = listOf(object : UnleashReadyListener, UnleashImpressionEventListener {
override fun onImpression(event: ImpressionEvent) {
impressionEvents.add(event)
}
Expand All @@ -166,21 +167,28 @@ class DefaultUnleashTest: BaseTest() {
ready = true
}
}), bootstrap = listOf(
Toggle(name = "with-impression", enabled = true, impressionData = true),
Toggle(name = "with-impression-1", enabled = true, impressionData = true),
Toggle(name = "with-impression-2", enabled = true, impressionData = true),
Toggle(name = "with-impression-3", enabled = true, impressionData = true),
Toggle(name = "without-impression", enabled = false)
)
)
await().atMost(1, TimeUnit.SECONDS).until { ready }

unleash.isEnabled("with-impression")
unleash.isEnabled("with-impression", true)

unleash.isEnabled("with-impression-1")
unleash.isEnabled("with-impression-2", true)
unleash.isEnabled("without-impression")
unleash.isEnabled("with-impression", false)
unleash.isEnabled("with-impression-3", false)
unleash.isEnabled("non-existing-toggle")

await().atMost(1, TimeUnit.SECONDS).until { impressionEvents.size >= 3 }
assertThat(impressionEvents).hasSize(3)
assertThat(impressionEvents).allMatch { it.featureName == "with-impression" }
assertThat(impressionEvents).allMatch { it.enabled }
for (i in 1 until 4) {
impressionEvents[i - 1].let {
assertThat(it.featureName).isEqualTo("with-impression-$i")
assertThat(it.enabled).isTrue()
}
}
}
}