From fea9ec5ea498e25b24d65ef9e619790102337a0d Mon Sep 17 00:00:00 2001 From: Levi Bostian Date: Thu, 22 Feb 2024 14:46:43 -0600 Subject: [PATCH] fix: prevent duplicate automatic screenview events from being tracked Closes: https://linear.app/customerio/issue/MBL-130/automatic-screen-view-feature-causing-multiple-events-tracked The SDK keeps track of the last screen automatically tracked. If the new screenview event is equal to this last screen tracked, ignore the request. The linear ticket gives context on why this solution was chosen over alternative approaches. commit-id:336b0aff --- Sources/Common/Util/LockManager.swift | 1 + .../Plugins/AutoTrackingScreenViews.swift | 57 +++++++++++++++++-- .../AutoDependencyInjection.generated.swift | 27 +++++++++ ...pelineImplementation+ScreenViewsTest.swift | 53 ++++++++++++++++- 4 files changed, 133 insertions(+), 5 deletions(-) diff --git a/Sources/Common/Util/LockManager.swift b/Sources/Common/Util/LockManager.swift index 85fc2614b..9a29ac660 100644 --- a/Sources/Common/Util/LockManager.swift +++ b/Sources/Common/Util/LockManager.swift @@ -32,6 +32,7 @@ public class LockManager { public enum LockReference: String { case queueStorage case pushHistory + case autoTrackScreenViewStore } // Dependency needs to be accessed by multiple graphs. diff --git a/Sources/DataPipeline/Plugins/AutoTrackingScreenViews.swift b/Sources/DataPipeline/Plugins/AutoTrackingScreenViews.swift index 31c6696ea..9804d2e13 100644 --- a/Sources/DataPipeline/Plugins/AutoTrackingScreenViews.swift +++ b/Sources/DataPipeline/Plugins/AutoTrackingScreenViews.swift @@ -12,7 +12,13 @@ public class AutoTrackingScreenViews: UtilityPlugin { public let type = PluginType.utility public var analytics: Segment.Analytics? - public var diGraph = DIGraphShared.shared + public var diGraph: DIGraphShared { + DIGraphShared.shared + } + + private var store: AutoTrackingScreenViewStore { + diGraph.autoTrackingScreenViewStore + } static let notificationName = Notification.Name(rawValue: "AutoTrackingScreenViewsNotification") static let screenNameKey = "name" @@ -36,7 +42,6 @@ public class AutoTrackingScreenViews: UtilityPlugin { public init( filterAutoScreenViewEvents: ((UIViewController) -> Bool)? = nil, - autoScreenViewBody: (() -> [String: Any])? = nil ) { self.filterAutoScreenViewEvents = filterAutoScreenViewEvents @@ -92,9 +97,9 @@ extension AutoTrackingScreenViews { } let filter = customerOverridenFilter ?? defaultSdkFilter - let shouldTrackEvent = filter(viewController) + let screenPassesFilter = filter(viewController) - guard shouldTrackEvent else { + guard screenPassesFilter else { let isUsingSdkDefaultFilter = customerOverridenFilter == nil diGraph.logger.debug( "automatic screenview ignored for, \(name):\(viewController.bundleIdOfView ?? ""). It was filtered out. Is using sdk default filter: \(isUsingSdkDefaultFilter)" @@ -102,6 +107,13 @@ extension AutoTrackingScreenViews { return // event has been filtered out. Ignore it. } + // Because of the automatic screenview tracking implementation, it's possible to see 2+ screenview events get tracked for 1 screen when the screen is viewed. To prevent this duplication, we only track the event if the screen the user is looking at has changed. + let isLastScreenTracked = store.isScreenLastScreenTracked(screenName: name) + guard !isLastScreenTracked else { + diGraph.logger.debug("automatic screenview ignored for, \(name):\(viewController.bundleIdOfView ?? ""). It was already tracked.") + return + } + let addionalScreenViewData = autoScreenViewBody?() ?? [:] analytics?.screen(title: name, properties: addionalScreenViewData) } @@ -211,3 +223,40 @@ extension UIViewController { return nil } } + +// Store for the automatic screenview feature. +protocol AutoTrackingScreenViewStore { + /* + Returns true if `screenName` is equal to the `screenName` the last time this function was called. + Use this return value to determine if the screenview event has already been tracked or not. + + This function will save `screenName` as the last screen tracked to be used the next time this function is called. + To make the store's thread-safety easier, all getting/setting in the store is encapsulated in 1 function call. + */ + func isScreenLastScreenTracked(screenName: String) -> Bool +} + +// in-memory store because the data stored is not relevant when the app is started. We purposely are not persisting the store's data. +// +// sourcery: InjectRegisterShared = "AutoTrackingScreenViewStore" +// sourcery: InjectSingleton +class InMemoryAutoTrackingScreenViewStore: AutoTrackingScreenViewStore { + let lock: Lock + + private var lastScreenTracked: String? + + init(lockManager: LockManager) { + self.lock = lockManager.getLock(id: .autoTrackScreenViewStore) + } + + func isScreenLastScreenTracked(screenName: String) -> Bool { + lock.lock() + defer { self.lock.unlock() } + + let isLastScreenTracked = lastScreenTracked == screenName + + lastScreenTracked = screenName + + return isLastScreenTracked + } +} diff --git a/Sources/DataPipeline/autogenerated/AutoDependencyInjection.generated.swift b/Sources/DataPipeline/autogenerated/AutoDependencyInjection.generated.swift index 42ea7cdde..d3bb4a9c6 100644 --- a/Sources/DataPipeline/autogenerated/AutoDependencyInjection.generated.swift +++ b/Sources/DataPipeline/autogenerated/AutoDependencyInjection.generated.swift @@ -78,6 +78,9 @@ extension DIGraphShared { func testDependenciesAbleToResolve() -> Int { var countDependenciesResolved = 0 + _ = autoTrackingScreenViewStore + countDependenciesResolved += 1 + _ = deviceAttributesProvider countDependenciesResolved += 1 @@ -85,6 +88,30 @@ extension DIGraphShared { } // Handle classes annotated with InjectRegisterShared + // AutoTrackingScreenViewStore (singleton) + var autoTrackingScreenViewStore: AutoTrackingScreenViewStore { + getOverriddenInstance() ?? + sharedAutoTrackingScreenViewStore + } + + var sharedAutoTrackingScreenViewStore: AutoTrackingScreenViewStore { + // Use a DispatchQueue to make singleton thread safe. You must create unique dispatchqueues instead of using 1 shared one or you will get a crash when trying + // to call DispatchQueue.sync{} while already inside another DispatchQueue.sync{} call. + DispatchQueue(label: "DIGraphShared_AutoTrackingScreenViewStore_singleton_access").sync { + if let overridenDep: AutoTrackingScreenViewStore = getOverriddenInstance() { + return overridenDep + } + let existingSingletonInstance = self.singletons[String(describing: AutoTrackingScreenViewStore.self)] as? AutoTrackingScreenViewStore + let instance = existingSingletonInstance ?? _get_autoTrackingScreenViewStore() + self.singletons[String(describing: AutoTrackingScreenViewStore.self)] = instance + return instance + } + } + + private func _get_autoTrackingScreenViewStore() -> AutoTrackingScreenViewStore { + InMemoryAutoTrackingScreenViewStore(lockManager: lockManager) + } + // DeviceAttributesProvider var deviceAttributesProvider: DeviceAttributesProvider { getOverriddenInstance() ?? diff --git a/Tests/DataPipeline/DataPipelineImplementation+ScreenViewsTest.swift b/Tests/DataPipeline/DataPipelineImplementation+ScreenViewsTest.swift index 55dd7a3ab..ed3a4c557 100644 --- a/Tests/DataPipeline/DataPipelineImplementation+ScreenViewsTest.swift +++ b/Tests/DataPipeline/DataPipelineImplementation+ScreenViewsTest.swift @@ -15,7 +15,11 @@ class DataPipelineImplementationScreenViewsTest: IntegrationTest { // setting up required plugins outputReader = (CustomerIO.shared.add(plugin: OutputReaderPlugin()) as! OutputReaderPlugin) - autoTrackingScreenViews = (CustomerIO.shared.add(plugin: AutoTrackingScreenViews()) as! AutoTrackingScreenViews) + autoTrackingScreenViews = getTrackingScreenViewsPlugin() + } + + private func getTrackingScreenViewsPlugin() -> AutoTrackingScreenViews { + (CustomerIO.shared.add(plugin: AutoTrackingScreenViews()) as! AutoTrackingScreenViews) } // MARK: performScreenTracking @@ -56,6 +60,53 @@ class DataPipelineImplementationScreenViewsTest: IntegrationTest { assertEventTracked() } + func test_performScreenTracking_givenViewSameScreenMultipleTimes_expectNoTrackingDuplicateEvents() { + class ViewInsideOfHostApp: UIViewController {} + class AnotherViewInsideOfHostApp: UIViewController {} + + // The first time that the screen is tracked, an event should be added + autoTrackingScreenViews.performScreenTracking(onViewController: ViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 1) + + // If the screen is tracked again, ignore the event. + autoTrackingScreenViews.performScreenTracking(onViewController: ViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 1) + + // Check that an event is added, if the next screen is not equal to the last screen tracked. + autoTrackingScreenViews.performScreenTracking(onViewController: AnotherViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 2) + } + + func test_performScreenTracking_givenChangeScreen_expectTrackNonDuplicateScreens() { + class ViewInsideOfHostApp: UIViewController {} + class AnotherViewInsideOfHostApp: UIViewController {} + + // The first time that the screen is tracked, an event should be added + autoTrackingScreenViews.performScreenTracking(onViewController: ViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 1) + + // Change to a different screen, expect to track it. + autoTrackingScreenViews.performScreenTracking(onViewController: AnotherViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 2) + + // Re-visit the first screen again, expect to track it. + autoTrackingScreenViews.performScreenTracking(onViewController: ViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 3) + } + + func test_performScreenTracking_givenMultiplePluginInstances_expectNoTrackingDuplicateEvents() { + class ViewInsideOfHostApp: UIViewController {} + + let plugin1 = getTrackingScreenViewsPlugin() + let plugin2 = getTrackingScreenViewsPlugin() + + plugin1.performScreenTracking(onViewController: ViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 1) + + plugin2.performScreenTracking(onViewController: ViewInsideOfHostApp()) + assertEventTracked(numberOfEventsAdded: 1) + } + // MARK: getNameForAutomaticScreenViewTracking func test_getNameForAutomaticScreenViewTracking_givenViewWithNoTitle_expectNil() {