-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
fix: start tracking sessions at init for session replay #186
fix: start tracking sessions at init for session replay #186
Conversation
…ng plugins to initial config, centralize time to use SystemTime for easier testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Justin for fixing this.
I can see why sessionId
is not guaranteed to be set after isBuilt
as it's part of timeline. To fix this, instead of adding a session
property in Amplitude
, how about creating timeline in build()
? So that we don't need to support adding plugins to initial config which is needed to add properties to session events created during initialization.
// core Amplitude
class Amplitude{
init {
require(configuration.isValid()) { "invalid configuration" }
// Stop creating timeline in init
// timeline = this.createTimeline()
logger = configuration.loggerProvider.getLogger(this)
isBuilt = this.build()
isBuilt.start()
}
protected open fun build(): Deferred<Boolean> {
// create timeline here
timeline = this.createTimeline()
}
}
android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/amplitude/android/utilities/Session.kt
Outdated
Show resolved
Hide resolved
android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt
Outdated
Show resolved
Hide resolved
android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt
Outdated
Show resolved
Hide resolved
android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt
Outdated
Show resolved
Hide resolved
Wiz Scan Summary
|
Thanks @Mercy811, I tried moving the session creation into the Timeline.start() but then I need to pass in all the dependencies from Amplitude (configuration, storage, store). I think this implementation is a little cleaner given that. wdyt? |
Not sure why the one test is failing. It passes on my local. Debugging it now. |
android/src/main/java/com/amplitude/android/utilities/Session.kt
Outdated
Show resolved
Hide resolved
android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving the session creation into the Timeline.start() but then I need to pass in all the dependencies from Amplitude (configuration, storage, store). I think this implementation is a little cleaner given that. wdyt?
Hi @justin-fiedler, I read the updated code. To not pass all the dependencies Amplitude (configuration, storage, store), how about accessing them via amplitude
as core timeline already has amplitude as its class property.
// core Timline
open class Timeline {
lateinit var amplitude: Amplitude // core Amplitude
}
// android Timeline
class Timeline : Timeline() {
// should be android Amplitude to enable access `configuration.defaultTracking.sessions` in android Configuration.
// Kotlin requires exact type, I did a quick search and it seems to have workaround to use subclass of the original type
override lateinit var amplitude: Amplitude
}
The biggest concern for me is that right now we have session
class property in both Amplitude and Timeline and I'd imagine it would be hard to maintain. I feel it's ok to keep session
in either one of them to have a cleaner logic.
Thanks @Mercy811 that made it a lot cleaner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Justin, this is a huge improvement! 🎉
## [1.16.3](v1.16.2...v1.16.3) (2024-03-25) ### Bug Fixes * start tracking sessions at init for session replay ([#186](#186)) ([7b69897](7b69897))
Summary
Sessions are not working as expected with the Session Replay functionality.
The main issue I am trying to solve is ensuring that
amplitude.sessionId
is set afterisBuilt
which was not always the case until now.Additionally customers have asked for a way to listen to session changes without requiring session events. For this I updated the
ObservePlugin
to haveonSessionIdChanged()
callback that fires regardless of Session event tracking. Future versions of the Session Replay plugin can use this listener instead of listening for session events.Since Session events can now be fired on instantiation e.g.
val amp = Amplitude(...)
I also addedval amp = Amplitude(Configuration(plugins = listOf(..plugins that run on any events during init))
. This can be used to attach params to session start and other potential DET events.Full list of changes:
amplitude.sessionId
is ready beforeisBuilt
is completeObservePlugin.onSessionIdChanged
callbackChecklist