Coroutines: add event flow #91

wzieba · 2023-11-08T08:48:15Z

Description

This PR introduces Kotlin Coroutines for "add an event" flow. It covers

adding an event to the in-memory buffer
storing event in local storage (file)

This PR also:

removes logic of QueueManager as discussed internally p1699018266041689-slack-C0533SEJ82H
makes save to file operation thread-safe of the main thread

How to test

Run the test app
Tap on "Track URL" multiple times. Count how many times you tapped.
Observe logs: you'll notice a new log Persisting n events, where n is the number of events added in 1-second time frame
Move app to the background
Assert that number m of events in Sending request with m events log equals the number you counted in step 2

to `LocalStorageRepository`

The logic of validating queue is no longer relevant as discussed internally. This commits removes `QueueManager` completely, and replaces it with a thread-safe `InMemoryBuffer`, which holds newly recorded events and, as soon as possible, adds them to local persistence. The new event not added to persistence right away, because the local storage file might be occupied/locked by, e.g. process/coroutine of sending events. We do not want to make consumer of the library to wait for adding a new event ever. Hence, the `InMemoryRepository#buffer`.

By using mutual exclusion (`Mutex`). Also, remove the `persistEvent` method, as it's no longer needed actually.

Also, add a name for easier identification while debugging

To reduce overhead of constant loop, the `InMemoryBuffer` will check `buffer` list every second.

As `inMemoryBuffer` triggers asynchronous operation, this PR adds a listener that asserts starting flush queue after adding an event.

50 events batches are no longer a thing since we removed `QueueManager`

wzieba · 2023-11-08T15:36:09Z

parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt

+    private val onEventAddedListener: () -> Unit,
+) {
+
+    private val mutex = Mutex()


To test if this mutual exclusion works correctly, comment this, usages in init and add and then run stressTest. It should fail with unexpected result - sometimes recorded events are smaller, sometimes bigger.

Thanks for the instructions! 🥇

FYI: I run that stressTest 3 times after removing mutex and everything was still working as expected on my side (testing on actual device). I then also triggered the whole ./gradlew connectedDebugAndroidTest from CLI and it still worked as expected, everything was successful. 🤷

UPDATE: I then also removed the mutex on the repository level and run the stressTest 3 times, along with running the whole ./gradlew connectedDebugAndroidTest from CLI. Running the stressTest succeeded every time, but running the whole ./gradlew connectedDebugAndroidTest from CLI indeed failed (both times I tried):

com.parsely.parselyandroid.FunctionalTests > stressTest[Pixel 4 - 13] FAILED java.lang.AssertionError: Expected size: 500 but was: 1000 in: Tests on Pixel 4 - 13 failed: There was 1 failure(s).

PS:

I then uncommented the mutex on the repository level and run whole ./gradlew connectedDebugAndroidTest from CLI again and it started failing as well. 👍

I then, uncommented the mutex on the buffer level and run whole ./gradlew connectedDebugAndroidTest from CLI again and it now succeeded. ✅

I guess there exist some flakiness but it seems that everything is working as expected after all. Just wanted to document my testing here, just in case it somehow helps you.

Thanks for sharing! I think for some reason, they're more likely to fail without mutex on emulator. In any case, one can also remove the delay to the InMemoryBuffer#init - this would make the test fail more certain.

I think for some reason, they're more likely to fail without mutex on emulator.

Yea, maybe! 🤷

In any case, one can also remove the delay to the InMemoryBuffer#init - this would make the test fail more certain.

True, good tip! 💯

parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt

codecov · 2023-11-08T15:52:39Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (5e590c2) 53.00% compared to head (8c4ce70) 55.85%.

Additional details and impacted files

@@              Coverage Diff               @@
##           coroutines      #91      +/-   ##
==============================================
+ Coverage       53.00%   55.85%   +2.84%     
==============================================
  Files              12       12              
  Lines             366      376      +10     
  Branches           57       57              
==============================================
+ Hits              194      210      +16     
+ Misses            155      148       -7     
- Partials           17       18       +1

Files	Coverage Δ
...m/parsely/parselyandroid/LocalStorageRepository.kt	`76.47% <100.00%> (+0.60%)`	⬆️
...om/parsely/parselyandroid/ParselyCoroutineScope.kt	`100.00% <100.00%> (ø)`
.../java/com/parsely/parselyandroid/InMemoryBuffer.kt	`95.65% <95.65%> (ø)`
...ava/com/parsely/parselyandroid/ParselyTracker.java	`11.81% <27.27%> (+0.69%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

All events are now add to `InMemoryBuffer` and then from there to local storage which is our SSOT for events that are about to be sent.

ParaskP7

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, everything works as expected, awesome job with InMemoryBuffer and removing the eventQueue, lots of complexity is now actually gone! 🌟 🎉 🚀

I have left a questions (❓) and a couple of suggestions (💡) for you to consider. I am going to approve this PR anyway, since none is blocking (ish). I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

.github/workflows/readme.yml

parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt

ParaskP7 · 2023-11-09T11:25:14Z

parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt

-        ParselyTracker.PLog("Persisting event queue")
-        persistObject((inMemoryQueue + getStoredQueue()).distinct())
+    open suspend fun insertEvents(toInsert: List<Map<String, Any?>?>) = mutex.withLock {
+        ParselyTracker.PLog("Persisting ${toInsert.size} events")


Suggestion (💡): Just like it is done on InMemoryBuffer.add(...), maybe this log can be moved to InMemoryBuffer.init { ... } as well and make the logging closer to source and more consistent, at least with each other. Wdyt? 🤔

parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java

ParaskP7 · 2023-11-09T11:27:59Z

parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt

+    private val repository = FakeLocalStorageRepository()
+
+    @Test
+    fun `when adding a new event, then save it to local storage and run onEventAdded listener`() = runTest {


Suggestion (💡): Consider splitting this test into two:

when adding a new event, then save it to local storage

given a listener, when adding a new event, then run listener

ParaskP7 · 2023-11-09T11:32:58Z

parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt

+    private val onEventAddedListener: () -> Unit,
+) {
+
+    private val mutex = Mutex()


Thanks for the instructions! 🥇

FYI: I run that stressTest 3 times after removing mutex and everything was still working as expected on my side (testing on actual device). I then also triggered the whole ./gradlew connectedDebugAndroidTest from CLI and it still worked as expected, everything was successful. 🤷

ParaskP7 · 2023-11-09T11:41:29Z

parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt

+    private val onEventAddedListener: () -> Unit,
+) {
+
+    private val mutex = Mutex()


UPDATE: I then also removed the mutex on the repository level and run the stressTest 3 times, along with running the whole ./gradlew connectedDebugAndroidTest from CLI. Running the stressTest succeeded every time, but running the whole ./gradlew connectedDebugAndroidTest from CLI indeed failed (both times I tried):

com.parsely.parselyandroid.FunctionalTests > stressTest[Pixel 4 - 13] FAILED java.lang.AssertionError: Expected size: 500 but was: 1000 in: Tests on Pixel 4 - 13 failed: There was 1 failure(s).

PS:

I then uncommented the mutex on the repository level and run whole ./gradlew connectedDebugAndroidTest from CLI again and it started failing as well. 👍

I then, uncommented the mutex on the buffer level and run whole ./gradlew connectedDebugAndroidTest from CLI again and it now succeeded. ✅

I guess there exist some flakiness but it seems that everything is working as expected after all. Just wanted to document my testing here, just in case it somehow helps you.

in `InMemoryBuffer`

wzieba added 14 commits November 7, 2023 14:34

tests: add functional stress test

0013445

refactor: replace AsyncTask with Kotlin Coroutines in QueueManager

cbae82d

fix: close file streams when finished using

d3b7726

feat: add remove and persistEvent methods

e9d88d6

to `LocalStorageRepository`

feat: make LocalStorageRepository#insertEvents thread safe.

0487719

By using mutual exclusion (`Mutex`). Also, remove the `persistEvent` method, as it's no longer needed actually.

feat: make SDK's CoroutineScope internal

1905ffc

Also, add a name for easier identification while debugging

feat: check the buffer every second

8a518ce

To reduce overhead of constant loop, the `InMemoryBuffer` will check `buffer` list every second.

tests: add unit tests for InMemoryBuffer

01503d6

feat: introduce listener after adding an event

f963a0e

As `inMemoryBuffer` triggers asynchronous operation, this PR adds a listener that asserts starting flush queue after adding an event.

ci: add androidTest-results to CI job artifacts

54e66a9

tests: update stress test to not verify removed logic

52c7a43

50 events batches are no longer a thing since we removed `QueueManager`

tests: update basic tracks events scenario

3e127ba

50 events batches are no longer a thing since we removed `QueueManager`

style: improve log messages

e1234ac

wzieba mentioned this pull request Nov 8, 2023

Migrate threading model to Coroutines - long running branch #89

Merged

wzieba commented Nov 8, 2023

View reviewed changes

wzieba requested a review from ParaskP7 November 8, 2023 15:37

wzieba marked this pull request as ready for review November 8, 2023 15:37

Merge branch 'coroutines' into coroutines-queue-manager

e14615f

style: remove in-memory eventQueue

9efd59f

All events are now add to `InMemoryBuffer` and then from there to local storage which is our SSOT for events that are about to be sent.

ParaskP7 approved these changes Nov 9, 2023

View reviewed changes

wzieba added 3 commits November 9, 2023 20:06

style: move log out of

591a72d

tests: split adding new event tests

86ac0eb

in `InMemoryBuffer`

style: fix test name

8c4ce70

wzieba merged commit 845947b into coroutines Nov 9, 2023
2 checks passed

wzieba deleted the coroutines-queue-manager branch November 9, 2023 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coroutines: add event flow #91

Coroutines: add event flow #91

wzieba commented Nov 8, 2023 •

edited

Loading

wzieba Nov 8, 2023

ParaskP7 Nov 9, 2023

ParaskP7 Nov 9, 2023

wzieba Nov 9, 2023

ParaskP7 Nov 10, 2023

codecov bot commented Nov 8, 2023 •

edited

Loading

ParaskP7 left a comment

ParaskP7 Nov 9, 2023

ParaskP7 Nov 9, 2023

ParaskP7 Nov 9, 2023

ParaskP7 Nov 9, 2023

Coroutines: add event flow #91

Coroutines: add event flow #91

Conversation

wzieba commented Nov 8, 2023 • edited Loading

Description

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2023 • edited Loading

Codecov Report

ParaskP7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzieba commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading