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

feat: manual start/stop session recording #276

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ioannisj
Copy link
Contributor

@ioannisj ioannisj commented Dec 19, 2024

💡 Motivation and Context

Closes #262 and #227

Docs PR: PostHog/posthog.com#10235

Added the following:

  • startSessionRecording(resumeCurrent: Bool)
  • stopSessionRecording()

Session Management

Related discussion: https://posthog.slack.com/archives/C03PB072FMJ/p1733146678787689?thread_ts=1732810962.491699&cid=C03PB072FMJ

Based on the discussion above, I refactored how we manager and rotate replay sessions. Quite a big change but the main points of the change are:

  • Internal getSessionId changes form and is now responsible on running validation rules and rotates or clears the session if needed before returning
  • getSessionId can be called with readOnly to skip validation and just return current session id. In both cases, a new id will be created if it doesn't exist but only when the app is foregrounded
  • Validation rules are:
    • Rotates session after 30 minutes of inactivity
    • Clears session after 30 minutes of inactivity (when app is backgrounded at the time of calling this method)
    • Enforces a maximum session duration of 24 hours
    • All validations can be run against a given timestamp, otherwise .now is used. For replay, we capture the timestamp of the event as soon as possible, so we need to run session validation checks on that timestamp to account for delayed captures
  • $snapshot events add their own $session_id property which is acquired as early as possible and capture() method will respect that $session_id
  • $snapshot events always call capture() with a given timestamp, acquired as early as possible
  • Added a reason on hedgeLog messages related to session id changes for debugging purposes

TODO:

  • Skip $snapshot events if $session_id is missing from event properties

💚 How did you test it?

  • Unit tests
  • Added a section in PostHogExample for manual testing
  • Objc project interface

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

//

// swiftlint:disable:next type_name
enum DI {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this purely for testing purposes, so that we can easily swap out singletons in the codebase for testing. We can slowly start porting dependencies here

@@ -1052,6 +1098,9 @@ let maxRetryDelay = 30.0
}

private func registerNotifications() {
guard !didRegisterNotifications else { return }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a simple test to not register for the same notification twice. When talking to a customer they mentioned calling .setup() multiple times

@@ -1013,25 +998,86 @@ let maxRetryDelay = 30.0
flagCallReported.removeAll()
}
context = nil
PostHogSessionManager.shared.endSession {
self.resetViews()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replay integration will handle this directly by using the new PostHogSessionManager.shared.onSessionIDChanged callback

@@ -1205,10 +1249,6 @@ let maxRetryDelay = 30.0

@objc func handleAppDidEnterBackground() {
captureAppBackgrounded()

PostHogSessionManager.shared.updateSessionLastTime()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We instead keep a sessionActivityTimestamp when the user interacts with the app. This is set when PostHogSessionManager.shared.touchSession() is called

@@ -8,115 +8,209 @@
import Foundation

// only for internal use
// Do we need to expose this as public API? Could be internal static instead?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried not to change the public API on this, but is this used somewhere? RN or Flutter maybe?

Comment on lines 53 to 58
/// Whether to start session replay automatically or manually during the SDK setup
/// Options are:
/// - .automatic: Start session replay automatically during the SDK setup
/// - .manual: Start/Stop session replay manually by calling `startSessionReplay()` and `stopSessionReplay()`
/// Default is .automatic
@objc public var startMode: PostHogSessionReplayStartMode = .automatic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JS SDK does not have this since it is automatic by default, but if sessionReplay is disabled, they can just call startSessionRecording
Any reason to be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah that's another an. Could be a bit awkward though switching the config.sessionReplay behind the scenes for the user? In my mind, that config is for the integration itself? But having less configuration options is for sure more desirable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a bit awkward though switching the config.sessionReplay behind the scenes for the user?

we won't do this, the user chooses to sessionReplay: true for everything automatically, or keep it false and start/stop manually, this is how the Web works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use config.sessionReplay throughout though to add some checks to replay integration. This means, config.sessionReplay will be used just in setup() code to start the integration or not, and we rework those checks. Okay i'll have a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed startMode from config. Calling just startSessionRecording() is enough now

@@ -15,7 +15,12 @@ import Foundation
*/
func applicationSupportDirectoryURL() -> URL {
let url = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first!
return url.appendingPathComponent(Bundle.main.bundleIdentifier!)
#if canImport(XCTest) // only visible to test targets
return url.appendingPathComponent(Bundle.main.bundleIdentifier ?? "com.posthog.test")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test was silently failing on me when force unwrapping nil value here. Possibly because of new swift-testing framework but should be safe to keep with a compiler directive

@@ -28,9 +28,10 @@
return
}

// swizzling twice will exchange implementations back to original
swizzle(forClass: UIApplication.self,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we were unswizzling wrong here. Swizzling the same methods again should restore original

sendEventOverride(event)
// update "last active" session
// we want to keep track of the idle time, so we need to maintain a timestamp on the last interactions of the user with the app. UIEvents are a good place to do so since it means that the user is actively interacting with the app (e.g not just noise background activity)
PostHogSessionManager.shared.touchSession()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main point of keeping taps on whether the user is interacting with the app (thus session is active) or not

Comment on lines +32 to +33
original: #selector(UIView.layoutSubviews),
new: #selector(UIView.layoutSubviewsOverride))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feel wrong? because the selectors are inverted after swizzle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, so we need to swap back
(custom) UIView.layoutSubviews -> (UIView.layoutSubviewsOverride under the hood)
(original) UIView.layoutSubviewsOverride -> (UIView.layoutSubviews under the hood)

I tested this locally and breakpoints in layoutSubviewsOverride don't fire when session replay is stopped which is what we need

@@ -92,6 +93,32 @@ struct ContentView: View {
var body: some View {
NavigationStack {
List {
Section("Manual Session Recording Control") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can run PostHogExample and play around with manual session start/stop

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.

Manually start and stop session recordings
2 participants