feat: manual start/stop session recording #276

ioannisj · 2024-12-19T09:03:26Z

💡 Motivation and Context

#skip-changelog

Closes #262 and #227

Docs PR: Pending

Added the following:

startMode in replay config - This controls if the SDK setup will start session replay if enabled or not
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.

ioannisj · 2024-12-19T09:17:17Z

PostHog/DI.swift

+//
+
+// swiftlint:disable:next type_name
+enum DI {


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

ioannisj · 2024-12-19T09:18:23Z

PostHog/PostHogSDK.swift

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

    private func registerNotifications() {
+        guard !didRegisterNotifications else { return }


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

ioannisj · 2024-12-19T09:18:59Z

PostHog/PostHogSDK.swift

+         - This will resume the current session or create a new one if it doesn't exist
+         */
+        @objc(startSessionRecording)
+        public func startSessionRecording() {


Had to opt out from default params for Objc interop, so two methods here

ioannisj · 2024-12-19T09:20:30Z

PostHog/PostHogSDK.swift

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


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

ioannisj · 2024-12-19T09:21:51Z

PostHog/PostHogSDK.swift

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

    @objc func handleAppDidEnterBackground() {
        captureAppBackgrounded()
-
-        PostHogSessionManager.shared.updateSessionLastTime()


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

ioannisj · 2024-12-19T09:22:30Z

PostHog/PostHogSessionManager.swift

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

 // only for internal use
+// Do we need to expose this as public API? Could be internal static instead?


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

marandaneto · 2024-12-19T09:24:41Z

PostHog/Replay/PostHogSessionReplayConfig.swift

+        /// 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


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?

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

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.

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

ioannisj · 2024-12-19T09:25:10Z

PostHog/PostHogStorage.swift

@@ -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")


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

ioannisj · 2024-12-19T09:26:06Z

PostHog/Replay/PostHogSessionReplayConfig.swift

+        /// 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()`


Suggested change

/// - .manual: Start/Stop session replay manually by calling `startSessionReplay()` and `stopSessionReplay()`

/// - .manual: Start/Stop session replay manually by calling `startSessionRecording()` and `stopSessionRecording()`

ioannisj · 2024-12-19T09:27:03Z

PostHog/Replay/UIApplicationTracker.swift

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

+            // swizzling twice will exchange implementations back to original
            swizzle(forClass: UIApplication.self,


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

ioannisj · 2024-12-19T09:27:37Z

PostHog/Replay/UIApplicationTracker.swift

            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()


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

marandaneto · 2024-12-19T09:27:45Z

PostHog/Replay/ViewLayoutTracker.swift

+                    original: #selector(UIView.layoutSubviews),
+                    new: #selector(UIView.layoutSubviewsOverride))


That feel wrong? because the selectors are inverted after swizzle

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

PostHog/Utils/ApplicationLifecyclePublisher.swift

ioannisj · 2024-12-19T09:29:47Z

PostHogExample/ContentView.swift

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


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

marandaneto · 2024-12-19T09:29:51Z

PostHogTests/PostHogContextTest.swift

-            #if os(iOS) || os(tvOS)
-                expect(context["$screen_width"] as? Float) != nil
-                expect(context["$screen_height"] as? Float) != nil
-            #endif


why did we remove this?

This is always nil because of a previous refactor of how we fetch screen_width and screen_height async - it requires a UIApplication instance which is not present in unit tests

if this is nil, the tests would always fail and CI is happy right now, so I don't follow?

CI doesn't run these tests actually cause of #if os(iOS).
make test (swift test) runs on current machine, so tests run on macOS by default

We could think of running make testOniOSSimulator instead though

True, I think I had issues with simulators back then and have gone with a purely swift test, but it seems like the time to do it if many tests depend on the simulator infrastructure, create an issue?

marandaneto · 2024-12-19T09:30:32Z

PostHogTests/PostHogAutocaptureEventTrackerSpec.swift

@@ -19,7 +19,6 @@
                    let view = UIView()
                    let eventData = view.eventData!

-                    expect(eventData.targetClass).to(equal("UIView"))


Any reason to touch autocapture files in this PR? feels unrelated

#276 (comment)

PostHogTests/PostHogAutocaptureIntegrationSpec.swift

ioannisj · 2024-12-19T09:31:14Z

PostHogTests/PostHogSessionManagerTest.swift

+//
+
+import Foundation
+import Testing


Failing in CI, will probably need Xcode 16 for this

any reason to switch from Nimble, and Quick that works across all tooling versions?

I just wanted to test out the new native framework, and to be honest I like it so far - it's expressive and works with existing tests. I thought we could slowly start porting (and revisiting) tests and eventually remove quick/nimble dependencies.

I don't have a strong preference here tbh, so I'll leave the call to you here

if this is failing CI and it's not an easy fix, I'd rather keep the test infrastructure that works across different tooling versions.
No preference either as long as everything works.

PostHog/PostHogSDK.swift

PostHog/PostHogSessionManager.swift

marandaneto · 2024-12-19T09:37:02Z

PostHog/PostHogStorage.swift

+    #if canImport(XCTest) // only visible to test targets
+        return url.appendingPathComponent(Bundle.main.bundleIdentifier ?? "com.posthog.test")
+    #else
+        // TODO: Should we be using a fallback temp directory instead of force unwrapping here?


If for some reason this can be nil, I'd fall back to in-memory storage but we don't have this yet.
We cannot cache everything in the applicationSupportDirectory that is visible for all the apps, this has to be sandboxed for only this app.

ioannisj added 5 commits December 13, 2024 10:24

chore: add swiftformat config

8a7a667

fix: avoid registering for notifications multiple times

581a08b

feat: add session replay automatic start config

5faf328

fix: capture timestamp along with snapshot events

9fc8a1e

feat: add start/stop session recording and refactor session manager

ac651e4

ioannisj commented Dec 19, 2024

View reviewed changes