fix: move session-specific logic to Timeline #89
Conversation
| @@ -119,6 +119,8 @@ class AmplitudeTest { | |||
| event.eventType = "test event" | |||
| amplitude?.track(event) | |||
| advanceUntilIdle() | |||
| Thread.sleep(100) | |||
There was a problem hiding this comment.
Would this be stable enough for the tests?
There was a problem hiding this comment.
- I can increment it to something like
500but it will increase test duration. - I would use more stable ways (something like
???.await()) but I am not sure what is better/possible to use in Kotlin and do not add notable memory/cpu overhead.
There was a problem hiding this comment.
I don't believe increasing sleep time is a good option. Not sure if @justin-fiedler or @bgiori know some better ways to mock this in tests in Kotlin though. If not, it maybe fine if you run it several times on local and this test always passes.
There was a problem hiding this comment.
yes, I ran the tests with Thread.sleep many times locally and they always pass
| inForeground = true | ||
|
|
||
| val dummySessionStartEvent = BaseEvent() | ||
| dummySessionStartEvent.eventType = START_SESSION_EVENT |
There was a problem hiding this comment.
So we always trigger this and decide later?
There was a problem hiding this comment.
Yes. To avoid any data race issues, all logic is in Timeout now
| private suspend fun processEventMessage(message: EventQueueMessage) { | ||
| val event = message.event | ||
| var sessionEvents: Iterable<BaseEvent>? = null | ||
| val eventTimestamp = event.timestamp!! |
There was a problem hiding this comment.
I believe other event timestamp is bind later in process plugins
There was a problem hiding this comment.
Not sure what you mean. I believe, timestamps should be assigned as soon as possible to have actual live values. Now it is here: https://github.com/amplitude/Amplitude-Kotlin/blob/DXOC-297-DiskReadViolation-in-strict-mode/core/src/main/java/com/amplitude/core/Amplitude.kt#L328.
Other code does fallback assignments like:
event.timestamp ?: let {
event.timestamp = System.currentTimeMillis()
}
There was a problem hiding this comment.
I see, so this is added to cover this. I was worried if this could raiser exceptions.
There was a problem hiding this comment.
processEventMessage is invoked only for items from eventMessageChannel
https://github.com/amplitude/Amplitude-Kotlin/blob/DXOC-297-DiskReadViolation-in-strict-mode/android/src/main/java/com/amplitude/android/Timeline.kt#L25
the only way to add items to eventMessageChannel is process function
https://github.com/amplitude/Amplitude-Kotlin/blob/DXOC-297-DiskReadViolation-in-strict-mode/android/src/main/java/com/amplitude/android/Timeline.kt#L34
here incomingEvent.timestamp = System.currentTimeMillis() exists to be ensure to have non-null timestamp
## [1.4.7](v1.4.6...v1.4.7) (2022-10-28) ### Bug Fixes * move session-specific logic to Timeline ([#89](#89)) ([b353b8c](b353b8c))
|
🎉 This PR is included in version 1.4.7 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary
All session-related logic is extracted to new Timeline class.
Checklist