RUMM-1615 SwiftUI View Instrumentation #645

maxep · 2021-10-19T11:59:04Z

What and why?

Add SwiftUI.View instrumentation by providing a SwiftUI.View extension to monitor a view with RUM.

import SwiftUI
import Datadog

struct FooView: View {

    var body: some View {
        FooContent {
            ...
        }
        .trackRUMView(name: "Foo")
    }
}

How?

UIKit Default Predicate

DefaultUIKitRUMViewsPredicate now exclude any UIViewController defined in SwiftUI framework. It will exclude the public UIHostingController but also any private container that SwiftUI creates under the hood.

SwiftUI View Modifier

A SwiftUI.View extension provides a simple interface to start monitoring the view. The extension applies a ViewModifier that responds to the .onAppear and .onDisappear to respectively start and stop monitoring the view with the internal RUMInstrumentation instance.

A ViewModifier returns a different version of the view which allow applying other modifiers or other handlers for .onAppear or .onDisappear without interfering with the RUM instrumentation.

SwiftUI RUM View Handler

The introduced SwiftUIRUMViewsHandler will keep track of the appeared views published by the RUMViewModifier to send Start and Stop RUM view commands in a consistent way. This handler uses a stack of appeared views:

When a view appears, it stops the previous view and start the new one.
When a view disappears, it removes it from the stack. If the view is the last one added, it will send a Stop rum command for that view and look for any remaining view in the stack to re-start.

This behaviour allow to re-start views when cancelling the back interaction in a navigation stack, and when dismissing a page sheet in modal navigation.

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

maxep · 2021-10-19T14:33:38Z

Tests/DatadogIntegrationTests/Scenarios/RUM/RUMNavigationControllerScenarioTests.swift

+        buttons["Push Next Screen"].safeTap(within: 5)
    }

    func tapBackButton() {
-        navigationBars["Screen 4"].buttons["Screen 3"].tap()
+        navigationBars["Screen 4"].buttons["Screen 3"].safeTap()
    }

    func tapPopToTheFirstScreenButton() {
-        buttons["Pop To The First Screen"].tap()
+        buttons["Pop To The First Screen"].safeTap()
    }

    func swipeInteractiveBackGesture() {
        let coordinate1 = coordinate(withNormalizedOffset: .init(dx: 0, dy: 0.5))
-        let coordinate2 = coordinate(withNormalizedOffset: .init(dx: 0.75, dy: 0.5))
+        let coordinate2 = coordinate(withNormalizedOffset: .init(dx: 0.80, dy: 0.5))


This is not related to SwiftUI but I've noticed some flakiness in this test which I'm mitigating here.

ncreated

Looks very good 🚀. I left few comments and questions.

Sources/Datadog/RUM/AutoInstrumentation/Views/SwiftUIViewModifier.swift

Sources/Datadog/RUM/RUMMonitor/Scopes/RUMSessionScope.swift

Tests/DatadogIntegrationTests/UITestsHelpers.swift

Tests/DatadogTests/Datadog/Utils/SwiftUIExtensionsTests.swift

ncreated

Looks great in the code 👌. However, when I played with this scenario on Simulator, I found one bug 🐞 when canceling interactive back gesture from UIKit view controller. Check the GIF below and see how our RUM debug outline view (at the bottom) disappears - meaning that there is no active view at that moment.

Seems that SwiftUI starts the view, then cancels it, but then UIKit doesn't own the previous view. AFAIR, in UIKit this case is handled from UIKitRUMViewsHandler and its notify_viewDidDisappear(viewController:animated:) invoked by the cancelled VC (the "back" view in navigation sense). This might be a tricky case to solve 💡 (this sort of complexity was one reason to introduce UIKitRUMViewsHandler.swift) but it's definitely something we must handle for apps mixing UIKit + SwiftUI. You can debug this concept in UIKit by running this scenario:

Sources/Datadog/RUM/AutoInstrumentation/Views/SwiftUIViewModifier.swift

Datadog/Example/Scenarios/RUM/SwiftUIInstrumentation/SwiftUIRootViewController.swift

Datadog/Example/Scenarios/RUM/RUMScenarios.swift

Tests/DatadogIntegrationTests/Scenarios/RUM/RUMSwiftUIScenarioTests.swift

maxep · 2021-10-22T08:55:55Z

That's a tricky case indeed. The SwiftUI Hosting VCs are filtered out before reaching the UIKitRUMViewsHandler.startIfNotStarted. I will see where we can best cover this use case.

maxep · 2021-10-27T11:36:11Z

I've made significant changes in the way we track navigation in SwiftUI. It's involved creation of a specific RUM View handler capable of tracking the state of the appeared views. The goal is to cover the following use cases:

Dismiss Presentation Sheet (modal)
Cancel back interaction in navigation

ncreated

Really good and thoughtful work 👌. I left few open questions on some elements that puzzle me, wondering on your take on these.

I've also ran unit tests on iOS 11, 12 and 13 - seems good 👍.

Sources/Datadog/RUM/Instrumentation/RUMInstrumentation.swift

ncreated · 2021-11-03T09:38:19Z

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

+    /// This stack allows to track appearing and disappearing views to consistently
+    /// publish start and stop commands to the subscriber. The last item of the
+    /// stack is the visible one, any items below it have appeared before but not yet
+    /// disappeared. Therefore, they are considered not visible but can be revealed
+    /// if the last item disappears.
+    private var stack: [View] = []


Is this critical to deal with the complexity in SwiftUI's onAppear / onDisappear callbacks or is it more a helper we introduce for "start" / "stop" commands consistency?

I'm only concerned that this might introduce a second source of truth for views tracking. RUM scopes already solve some complexity, e.g. "if view B is started, but A was not stopped, A will be stopped automatically" or "if view X is started twice, the second command will be ignored".

With this complexity solved in RUM scopes, the views handler for UIKit (UIKitRUMViewsHandler) does not track VCs stack nor make any additional assumptions over historical state in its navigation. By tracking stack for SwiftUI, we'd need to ensure that both handlers are in sync (i.e. modify SwiftUI stack from UIKit handler for hybrid UIKit + Swift UI apps). Won't it get very complex?

On the other hand, handling all UIKit + SwiftUI navigation specifics in RUM scopes might get complex and tricky as well. RUM scopes aim at being UI-framework agnostic and that might be tough to have uniform behaviour for SwiftUI and UIKit. What's your take on this @maxep ?

This is critical to deal with the mentioned use cases:

Dismiss Presentation Sheet (modal)

Cancel back interaction in navigation

Without it, we don't restart the visible view when necessary. With UIKit auto-instrumentation, this is done with the help of UIKitHierarchyInspector and a simple state management. From my understanding, the RUM View scope only performs sanity checks while UIKitRUMViewsHandler and SwiftUIRUMViewsHandler track the views' states.

You are right about navigating between SwiftUI and UIKit, it will be very complex to keep them in sync, especially with the assumptions made by UIKitHierarchyInspector which does not work well with VC containers used in SwiftUI. I would be more keen on having a unified/ui-agnostic handler for both in RUMInstrumentation, I will be dealing with this issue in the next iteration (RUMM-1745) but I would be happy to chat more about possible solution!

With UIKit auto-instrumentation, this is done with the help of UIKitHierarchyInspector and a simple state management.

Indeed, it's all true and very good observation 👌. Thanks for more insights - clearly, we can't handle value-based SwiftUI.Views in the same way as we do with object-based UIKit.UIViewControllers. The stack seems like a good, minimal approach 👍.

Speaking of RUMM-1745, I don't have any clear idea ATM. Like you say, having a single handler that will receive both notify_viewDidAppear() and onAppear() callbacks could be a solution. This way we could be considering both UIKit and SwiftUI navigation in one place, possibly leading to more control. Another approach could be to introduce next handler that combines UIKit and SwiftUI handlers into single stream - in theory that would mean less refactoring, but in practise it can be not doable if the hybrid support is mainly about details.

ncreated · 2021-11-03T09:48:20Z

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

+    func trackRUMView(
+        name: String,
+        attributes: [AttributeKey: AttributeValue] = [:]
+    ) -> some View {
+        let path = "\(name)/\(typeDescription.hashValue)"
+        return modifier(RUMViewModifier(name: name, path: path, attributes: attributes))
+    }


We're missing coverage for this and RUMViewModifier in unit tests. Ideally, we may instantiate the view, force-call onAppear() / onDisappear() and record its interaction with stubbed SwiftUIViewHandler. It was easy in UIKit tests, but with SwiftUI this might get trickier. Any idea on this?

We cannot call .onAppear()/ .onDisappear() from a SwiftUI.View, a view only declare its body. That's why I left it out of unit tests and cover this in integration tests. I could try to workaround that by embedding the view in a UIHostController and see if I can trigger the events from it.

If it's not too complex, IMO we can give it a shot. Otherwise, integration tests coverage is fine 👌 and definitely better than tricky or flaky units.

I've added a unit test using UIHostingController. It also involve a mocked SwiftUIViewHandler and a new RUMInstrumentation.init.
onAppear and onDisappear have to be tested in a single unit because it has to wait for appearance before disappearing.

Sorry, I will revert the test.. It fails on iOS 14.4, I prefer reverting and sticking with the integration test to avoid introducing any flakiness.

Tests/DatadogTests/Datadog/Utils/SwiftUIExtensionsTests.swift

This reverts commit ea81936.

ncreated

🚀

maxep requested a review from a team as a code owner October 19, 2021 11:59

maxep added the needs-docs To mark PRs which need documentation update label Oct 19, 2021

maxep force-pushed the maxep/RUMM-1615/swiftui-view branch 3 times, most recently from 4a5066b to 73adca2 Compare October 19, 2021 14:25

maxep commented Oct 19, 2021

View reviewed changes

maxep force-pushed the maxep/RUMM-1615/swiftui-view branch from 73adca2 to 40a1150 Compare October 19, 2021 14:39

maxep self-assigned this Oct 19, 2021

maxep force-pushed the maxep/RUMM-1615/swiftui-view branch from 45441bd to dc87558 Compare October 20, 2021 08:09

ncreated reviewed Oct 20, 2021

View reviewed changes

maxep requested a review from ncreated October 21, 2021 08:19

ncreated requested changes Oct 22, 2021

View reviewed changes

maxep force-pushed the maxep/RUMM-1615/swiftui-view branch from a11dced to 8c28680 Compare October 27, 2021 10:44

maxep force-pushed the maxep/RUMM-1615/swiftui-view branch 2 times, most recently from 337e379 to 8f3d4d7 Compare October 28, 2021 11:40

maxep marked this pull request as draft November 2, 2021 07:42

maxep added 3 commits November 2, 2021 11:52

RUMM-1615 SwiftUI View Instrumentation

e13213d

RUMM-1615 Fix integration test flakiness

c4a7956

RUMM-1615 Workaround FB8907671 in 14.2 <= iOS < 14.5

566a95b

maxep force-pushed the maxep/RUMM-1615/swiftui-view branch from 8f3d4d7 to 566a95b Compare November 2, 2021 10:53

maxep marked this pull request as ready for review November 2, 2021 13:06

maxep requested review from ncreated and a team November 2, 2021 13:06

ncreated reviewed Nov 3, 2021

View reviewed changes

maxep added 4 commits November 3, 2021 17:21

RUMM-1615 Add RUMViewModifier unit tests

ea81936

RUMM-1615 Use native view modifier

dcec5b0

RUMM-1615 Remove RUMInstrumentation compliance to SwiftUIViewHandler

47b8d48

Revert "RUMM-1615 Add RUMViewModifier unit tests"

9fbfc16

This reverts commit ea81936.

RUMM-1615 Update identity property name and documentation

b59211e

maxep requested a review from ncreated November 4, 2021 10:08

ncreated approved these changes Nov 4, 2021

View reviewed changes

maxep merged commit db8df52 into feature/swiftui Nov 5, 2021

maxep deleted the maxep/RUMM-1615/swiftui-view branch November 5, 2021 10:40

maxep mentioned this pull request Nov 5, 2021

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

Merged

2 tasks

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-1615 SwiftUI View Instrumentation #645

RUMM-1615 SwiftUI View Instrumentation #645

maxep commented Oct 19, 2021 •

edited

Loading

maxep Oct 19, 2021

ncreated left a comment

ncreated left a comment

maxep commented Oct 22, 2021 •

edited

Loading

maxep commented Oct 27, 2021

ncreated left a comment

ncreated Nov 3, 2021

maxep Nov 3, 2021

ncreated Nov 3, 2021

ncreated Nov 3, 2021

maxep Nov 3, 2021 •

edited

Loading

ncreated Nov 3, 2021

maxep Nov 3, 2021

maxep Nov 3, 2021

ncreated left a comment

RUMM-1615 SwiftUI View Instrumentation #645

RUMM-1615 SwiftUI View Instrumentation #645

Conversation

maxep commented Oct 19, 2021 • edited Loading

What and why?

How?

UIKit Default Predicate

SwiftUI View Modifier

SwiftUI RUM View Handler

Review checklist

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

maxep commented Oct 22, 2021 • edited Loading

maxep commented Oct 27, 2021

ncreated left a comment

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

Choose a reason for hiding this comment

maxep Nov 3, 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

ncreated left a comment

Choose a reason for hiding this comment

maxep commented Oct 19, 2021 •

edited

Loading

maxep commented Oct 22, 2021 •

edited

Loading

maxep Nov 3, 2021 •

edited

Loading