RUMM-1745 Use single handler for UIKit and SwiftUI views instrumentation #657

maxep · 2021-11-05T13:59:16Z

What and why?

UIHostingController and UIViewControllerRepresentable provide bridges between SwiftUI and UIKit allowing navigation between the two UI frameworks.

Therefore, we need to perform a consistent view tracking when instrumenting both SwiftUI.View and UIKit.UIViewController.

How?

Single View Handler

#645 has introduced a new handler for instrumenting SwiftUI.View only: SwiftUIRUMViewsHandler.
Both UIKitRUMViewsHandler and SwiftUIRUMViewsHandler work similarly for forward navigation but adopt different solutions for tracking backward navigation.

Backward navigation tracking of UIKit.UIViewController uses a view-controller hierarchy inspector to retrieve the currently visible view, this approach is incompatible with SwiftUI since we cannot retrieve the declared SwiftUI.View from the view-controllers hierarchy.

The SwiftUIRUMViewsHandler, on the other hand, uses a stack of views to keep track of the appeared views and re-start them when needed during backward navigation. This approach is ui-framework agnostic and can easily be applied to UIKit.UIViewController.

The goal of the PR is to showcase the usage of a stack to track both SwiftUI.View and UIKit.UIViewController in a single handler: RUMViewsHandler

Tests

The same unit tests applied to UIKitRUMViewsHandler now run on RUMViewsHandler.
The integration tests RUMSwiftUIScenarioTests now perform hybrid navigation between SwiftUI and UIKit. Any other integration test stays untouched.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference

Sources/Datadog/RUM/Instrumentation/Views/RUMViewsHandler.swift

maxep · 2021-11-05T14:40:44Z

Tests/DatadogTests/Datadog/RUM/Instrumentation/Views/RUMViewsHandlerTests.swift

+
+            func rumView(for viewController: UIViewController) -> RUMView? {
+                numberOfCalls += 1
+                return .init(name: .mockRandom())


It differs from the original test implementation: If the predicate rejects the vc, it is not added to the stack.

Nice 👌, this clearly shows the improvement over previous approach, where UIKit handler would be invoked 5 times for this test 👍.

Sources/Datadog/RUM/Instrumentation/RUMInstrumentation.swift

maxep · 2021-11-05T14:56:47Z

Tests/DatadogIntegrationTests/Scenarios/RUM/RUMModalViewsScenarioTests.swift

@@ -9,7 +9,7 @@ import XCTest

 private extension ExampleApplication {
    func tapButton(titled buttonTitle: String) {
-        buttons[buttonTitle].tap()
+        buttons[buttonTitle].safeTap(within: 5)


Mitigating flakiness

ncreated

Looks like a very good approach 👌. I left few notes, but to me this is a good direction 👍.

Sources/Datadog/RUM/Instrumentation/RUMInstrumentation.swift

Sources/Datadog/RUM/Instrumentation/Views/SwiftUI/SwiftUIViewHandler.swift

ncreated · 2021-11-08T11:16:44Z

Sources/Datadog/RUM/Instrumentation/Views/UIKit/UIKitHierarchyInspector.swift

-    private func findHighestPresentedViewControllerInHierarchy(of viewController: UIViewController) -> UIViewController? {
-        if let presentedVC = viewController.presentedViewController {
-            return findHighestPresentedViewControllerInHierarchy(of: presentedVC)
-        }
-        return viewController
-    }
-
-    private func findTopViewControllerInHierarchy(of viewController: UIViewController) -> UIViewController? {
-        if let tabBarVC = viewController as? UITabBarController {
-            guard let selectedVC = tabBarVC.selectedViewController else {
-                return tabBarVC // when `UITabBarController` displays no view controllers
-            }
-            // When `UITabBarController` has selected VC, continue searching, as the selected VC
-            // may be containing another hierarchy (e.g. `UINavigationController`).
-            return findTopViewControllerInHierarchy(of: selectedVC)
-        } else if let navigationVC = viewController as? UINavigationController {
-            guard let topVC = navigationVC.topViewController else {
-                return navigationVC // when `UINavigationController` displays no view controllers
-            }
-            // When `UINavigationController` has top VC, continue searching, as the top VC
-            // may be containing another hierarchy (e.g. `UITabBarController`).
-            return findTopViewControllerInHierarchy(of: topVC)
-        } else {
-            return viewController
-        }
-    }


All these are smoothly supported with the new approach of tracking the stack? Or do you think there are still some edge cases we need to look closer at?

UITabBarController and UINavigationController are both supported by the stack, it send events in the same manner. I did some manual monkey testing to validate it, and integration tests are both green. @ncreated do you see any other checks I should run? (don't know if e2e tests applies here).

I can think of one edge case which I need to verify: When presenting multiple modal page sheet and one of them is rejected by the predicate. Then the back navigation can be inconsistent:

start view 1

reject view 2

start view 3

Dismiss view 3 -> will restart view 1 ❌

This is due to the fact that the vc below a page sheet does not disappear, in this case view 1 will still be in the stack

I see 👍. Yes, this new approach will require quite manual testing (not only passing integration tests), as we're entirely changing the model. The initial reason for introducing UIKitHierarchyInspector was to deal with the complexity of transitions between UIViewControllers, notably modal view controllers. Test fixtures were added to Example in #271 and the UIKitHierarchyInspector was done in #278.

Our integration tests do not cover all complexity (that would be tedious), and there might be still scenarios worth manual testing, e.g.:

changing the entire navigation tree, by window.rootViewController = ...;

dismissing first VC in the modal hierarchy of N VCs (it should dismiss the entire hierarchy);

changing navigationController.viewControllers stack programmatically;

jumping by few steps at once in the VCs hierarchy (e.g. imagine app receiving deeplink and handling it by pushing one VC to the stack + opening modal with another navigation VC).

I also remember that our UIKitHierarchyInspector was not well compatible with UISearchController and handling it required additional logic in the "views predicate". This compatibility could be best tested when dogfooding a branch in our Datadog iOS App, as it uses UISearchController in many places. We should have a look into this at the end, before final merge (I'll be happy to help with this and manual testing as well 🙌).

I have performed and validate the following manual testing by modifying the Example scenarios:

changing the entire navigation tree, by window.rootViewController = ... ✅

dismissing first VC in the modal hierarchy of N VCs, it should dismiss the entire hierarchy ✅

changing navigationController.viewControllers stack programmatically ✅

jumping by few steps at once in the VCs hierarchy (e.g. imagine app receiving deeplink and handling it by pushing one VC to the stack + opening modal with another navigation VC). ✅

I still need to test it with a UISearchController!

I still need to test it with a UISearchController!

Keep in mind we don't yet support UISearchController very well - it still requires manual tweaking in the implementation of predicate to support search bar transitions to embedded VCs. I think it will be enough to just do regression test on UISearchController and check if it gets recognised by UIKit instrumentation as a whole. Solving the problem with search bar transitions to embedded VCs is beyond the scope of this PR.

Sources/Datadog/RUM/Instrumentation/RUMInstrumentation.swift

Sources/Datadog/RUM/Instrumentation/Views/RUMViewsHandler.swift

Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewIdentityTests.swift

ncreated · 2021-11-16T10:31:20Z

Sources/Datadog/RUM/Instrumentation/Views/UIKit/UIKitHierarchyInspector.swift

-    private func findHighestPresentedViewControllerInHierarchy(of viewController: UIViewController) -> UIViewController? {
-        if let presentedVC = viewController.presentedViewController {
-            return findHighestPresentedViewControllerInHierarchy(of: presentedVC)
-        }
-        return viewController
-    }
-
-    private func findTopViewControllerInHierarchy(of viewController: UIViewController) -> UIViewController? {
-        if let tabBarVC = viewController as? UITabBarController {
-            guard let selectedVC = tabBarVC.selectedViewController else {
-                return tabBarVC // when `UITabBarController` displays no view controllers
-            }
-            // When `UITabBarController` has selected VC, continue searching, as the selected VC
-            // may be containing another hierarchy (e.g. `UINavigationController`).
-            return findTopViewControllerInHierarchy(of: selectedVC)
-        } else if let navigationVC = viewController as? UINavigationController {
-            guard let topVC = navigationVC.topViewController else {
-                return navigationVC // when `UINavigationController` displays no view controllers
-            }
-            // When `UINavigationController` has top VC, continue searching, as the top VC
-            // may be containing another hierarchy (e.g. `UITabBarController`).
-            return findTopViewControllerInHierarchy(of: topVC)
-        } else {
-            return viewController
-        }
-    }


I still need to test it with a UISearchController!

Keep in mind we don't yet support UISearchController very well - it still requires manual tweaking in the implementation of predicate to support search bar transitions to embedded VCs. I think it will be enough to just do regression test on UISearchController and check if it gets recognised by UIKit instrumentation as a whole. Solving the problem with search bar transitions to embedded VCs is beyond the scope of this PR.

buranmert

pr lgtm but i was wondering if we could steer our users away from using RUMViewModifier with small views?
for example there can be many fragments in the screen at once in Android, i guess now it's potentially the same in iOS with SwiftUI. is it somehow possible to disallow using RUMViewModifier with list cells or small views like that?

Sources/Datadog/RUM/Instrumentation/Views/RUMViewsHandler.swift

maxep · 2021-11-16T16:20:07Z

@buranmert That is true, but we have no way of knowing the rendered view ratio or presentation context sadly. We could use something like GeometryReader, but this would add much more complexity in terms of integration for our users.

ncreated

Great work 🏅, it looks good to me 👍.

ncreated · 2021-11-18T10:47:27Z

Tests/DatadogTests/Datadog/RUM/Instrumentation/Views/RUMViewsHandlerTests.swift

+
+            func rumView(for viewController: UIViewController) -> RUMView? {
+                numberOfCalls += 1
+                return .init(name: .mockRandom())


Nice 👌, this clearly shows the improvement over previous approach, where UIKit handler would be invoked 5 times for this test 👍.

maxep commented Nov 5, 2021

View reviewed changes

Sources/Datadog/RUM/Instrumentation/Views/RUMViewsHandler.swift Outdated Show resolved Hide resolved

maxep commented Nov 5, 2021

View reviewed changes

Sources/Datadog/RUM/Instrumentation/RUMInstrumentation.swift Outdated Show resolved Hide resolved

maxep commented Nov 5, 2021

View reviewed changes

maxep force-pushed the maxep/RUMM-1745/swiftui-uikit-navigation branch from cf6cc10 to fa81f82 Compare November 5, 2021 15:04

maxep requested review from ncreated and buranmert November 5, 2021 16:11

ncreated reviewed Nov 8, 2021

View reviewed changes

maxep self-assigned this Nov 10, 2021

maxep marked this pull request as ready for review November 15, 2021 10:05

maxep requested a review from a team as a code owner November 15, 2021 10:05

maxep requested a review from ncreated November 15, 2021 10:05

ncreated reviewed Nov 16, 2021

View reviewed changes

buranmert reviewed Nov 16, 2021

View reviewed changes

Sources/Datadog/RUM/Instrumentation/Views/RUMViewsHandler.swift Outdated Show resolved Hide resolved

maxep force-pushed the feature/swiftui branch from db8df52 to c807806 Compare November 17, 2021 10:42

maxep force-pushed the maxep/RUMM-1745/swiftui-uikit-navigation branch 2 times, most recently from 2d3e62d to a1d113d Compare November 17, 2021 11:46

ncreated approved these changes Nov 18, 2021

View reviewed changes

maxep mentioned this pull request Nov 18, 2021

RUMM-1658 Manual instrumentation of SwiftUI actions #661

Merged

2 tasks

maxep requested a review from buranmert November 22, 2021 09:43

maxep and others added 9 commits November 22, 2021 13:30

RUMM-1745 Use single handler for UIKit and SwiftUI views instrumentation

1c75e6b

RUMM-1745 prefix publisher methods with notify_

0f83834

RUMM-1745 Rename RUM instrumentations properties

6ebc2a5

RUMM-1745 Keep weak ref to view controller

724ff1e

RUMM-1745 Fix typo

a0c6699

RUMM-1745 Remove unnecessary stack check

e149db5

RUMM-1745 Test nil identity

d127697

RUMM-1745 Keep predicate private to view handler

a86632a

RUMM-1745 Fix SwiftUI scenario navigation bar title

3dd94c2

maxep force-pushed the maxep/RUMM-1745/swiftui-uikit-navigation branch from a1d113d to 3dd94c2 Compare November 22, 2021 12:31

buranmert approved these changes Nov 24, 2021

View reviewed changes

maxep merged commit b01284d into feature/swiftui Nov 24, 2021

maxep deleted the maxep/RUMM-1745/swiftui-uikit-navigation branch November 24, 2021 12:35

maxep mentioned this pull request Dec 1, 2021

SwiftUI Instrumentation #676

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-1745 Use single handler for UIKit and SwiftUI views instrumentation #657

RUMM-1745 Use single handler for UIKit and SwiftUI views instrumentation #657

maxep commented Nov 5, 2021 •

edited

Loading

maxep Nov 5, 2021

ncreated Nov 18, 2021

maxep Nov 5, 2021

ncreated left a comment •

edited

Loading

ncreated Nov 8, 2021

maxep Nov 8, 2021

ncreated Nov 9, 2021 •

edited

Loading

maxep Nov 15, 2021

ncreated Nov 16, 2021

ncreated Nov 16, 2021

buranmert left a comment

maxep commented Nov 16, 2021

ncreated left a comment

ncreated Nov 18, 2021

RUMM-1745 Use single handler for UIKit and SwiftUI views instrumentation #657

RUMM-1745 Use single handler for UIKit and SwiftUI views instrumentation #657

Conversation

maxep commented Nov 5, 2021 • edited Loading

What and why?

How?

Single View Handler

Tests

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buranmert left a comment

Choose a reason for hiding this comment

maxep commented Nov 16, 2021

ncreated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxep commented Nov 5, 2021 •

edited

Loading

ncreated left a comment •

edited

Loading

ncreated Nov 9, 2021 •

edited

Loading