amplitude / Amplitude-Kotlin

Amplitude Kotlin SDK
MIT License
27 stars 10 forks source link

fix: start tracking sessions at init for session replay #186

Closed justin-fiedler closed 3 months ago

justin-fiedler commented 4 months ago

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 after isBuilt 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 have onSessionIdChanged() 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 added val 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:

Checklist

wiz-inc-ef0132150e[bot] commented 4 months ago

Wiz Scan Summary

IaC Misconfigurations 0C 0H 0M 0L 0I
Vulnerabilities 0C 0H 0M 0L 0I
Sensitive Data 0C 0H 0M 0L 0I
Total 0C 0H 0M 0L 0I
Secrets 0🔑
justin-fiedler commented 4 months ago

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.

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?

justin-fiedler commented 4 months ago

Not sure why the one test is failing. It passes on my local. Debugging it now.

justin-fiedler commented 4 months ago

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.

Thanks @Mercy811 that made it a lot cleaner.