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.

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

Merged

Conversation

justin-fiedler
Copy link
Contributor

@justin-fiedler justin-fiedler commented Mar 19, 2024

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:

  • No longer migrate session info (sessionId, lastEventTime) - since migration only happens on a new app install, this information can/should be reset.
  • Move session logic into its own class
  • Remove session logic from timeline
  • Process session immediately in constructor, ensure amplitude.sessionId is ready before isBuilt is complete
  • Add sessionId to state
  • Add ObservePlugin.onSessionIdChanged callback
  • support adding plugins to initial config, this is needed to add properties to session events created during initialization
  • centralize time to use SystemTime for easier testing

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: API - no, Functional - yes - session events now fire immediately rather than on the first event being send.

…ng plugins to initial config, centralize time to use SystemTime for easier testing
Copy link
Contributor

@Mercy811 Mercy811 left a 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()
    }
}

@wiz-inc-ef0132150e
Copy link

wiz-inc-ef0132150e bot commented Mar 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link
Contributor

@Mercy811 Mercy811 left a 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.

@justin-fiedler
Copy link
Contributor Author

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.

@justin-fiedler justin-fiedler requested a review from Mercy811 March 21, 2024 00:30
Copy link
Contributor

@Mercy811 Mercy811 left a 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! 🎉

@justin-fiedler justin-fiedler merged commit 7b69897 into main Mar 25, 2024
3 checks passed
@justin-fiedler justin-fiedler deleted the AMP-94783-sessions-not-working-as-expected-in-session-replay branch March 25, 2024 18:59
github-actions bot pushed a commit that referenced this pull request Mar 25, 2024
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants