RUMM-1097 Fix DefaultUIKitRUMViewsPredicate for Objc view controllers

[ncreated]

What and why?

🐛 When RUM Views auto instrumentation was enabled, no UIViewController defined in Objc was tracked. No matter if the SDK was configured in Objc:

[configuration trackUIKitRUMViews];

or swift:

.trackUIKitRUMViews()

How?

The problem was caused by the filtering logic done in DefaultUIKitRUMViewsPredicate:

public func rumView(for viewController: UIViewController) -> RUMView? {
   let canonicalClassName = NSStringFromClass(type(of: viewController))
   let isCustomClass = canonicalClassName.contains(".") // custom class contains module prefix

   if isCustomClass {
      // ... capture the view
   } else {
      return nil // ignore the view
   }
}

isCustomClass was meant to indicate if the viewController is a custom VC provided by the user app (true) or is a standard VC coming from UIKit (false). This was to ignore all the "system" view controllers like UINavigationViewController, UITabBarController etc.

🐛 The implementation was based on the observation that custom Swift VCs are prefixed with module name when inspecting their name through NSStringFromClass() (e.g. MyApp.LoginViewController). This turned out to be not true for custom Objc VCs, which have no module prefix.

🌟 The fix is to change the way we recognise custom VC. Now we lookup the Bundle that the class is loaded from and we simply filter out all VCs coming from UIKitCore.framework bundle.

Why it was not catched before?

This bug was not visible, because we didn't have any tests covering Obj-C view controllers and RUM Views predicate.

How to prevent it in the future?

I added one unit test for covering this exact scenario and configured one of the integration tests to also work with Objective-C view controllers (to cover other potential issues of mixed Swift <> Objc codebase instrumentation).

The integration test was done by splitting the existing RUMResourcesScenario into two separate scenarios: RUMURLSessionResourcesScenario and RUMNSURLSessionResourcesScenario. They both have identical setup, storyboards and view controller implementations. The prior defines its VCs in Swift, the latter in Objective-C. It didn't require adding anything new in the UI, as those tests are based on the existing URLSession + NSURLSession scenarios.

As a bonus 🎁 we gain integration coverage for RUM Resources tracked from Obj-c code.

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

[acgh]

This is great 💪

[acgh]

We could also add test cases to check that some known UIKit classes are filtered out, to catch any regressions of the internal isUIKit with future releases or iOS.

[buranmert]

+1 to @acgh , we rely on private information (UIKitCore is private) and that may break at any time

[ncreated]

It's already there - just not visible in this diff. We had this test before:

    func testGivenDefaultPredicate_whenAskingUIKitViewController_itReturnsNoView() {
        // Given
        let predicate = DefaultUIKitRUMViewsPredicate()
        // When
        let uiKitViewController = UIViewController()
        let rumView = predicate.rumView(for: uiKitViewController)
        // Then
        XCTAssertNil(rumView)
    }

[acgh]

Nitpick: Put this as an internal extension on Bundle in the UIKitExtensions.swift file

[ncreated]

Do you mean sth like Bundle.isUIKit(class:) -> Bool? This would read weird, no? This seems to be rather a characteristic of the AnyClass than a Bundle - but as AnyClass is a metatype it cannot be extended.

[buranmert]

it's not important for me but i was thinking of smth like this:

// extension
extension AnyObject {
  var isUIKit: Bool { ... }
}
// calling site
if viewController.isUIKit { return }
else { ... }

[ncreated]

Done 👌 , I added:

internal extension Bundle {
    var isUIKit: Bool
}

extension and bunch of fixtures in tests.

[buranmert]

did we try it in a real device? IMO it's unlikely but bundleURL may be different there

[buranmert]

the real goal is excluding container view controllers, exclusion of UIKit types is just the workaround that we could come up with.
this comment here may be misleading if we come back to this issue later on, can we correct it?

here is the same issue in PR's description as well:

...This was to ignore all the "system" view controllers...

[ncreated]

It's about plain (not subclassed / customized) VCs from UIKit.

Before this PR, this implementation was considering MyCustomNavigationViewController as the RUM View. I think we shouldn't change that default if no one has reported it being wrong. We're doing a bug fix here, I wouldn't change the previous behaviour.

[buranmert]

no i'm not talking about the behavior, my concern is that the comments don't tell the whole story.
we first wanted to exclude containers and then noticed invisible view controllers in the hiearchy. if one asks "why do we exclude UIKit types?", the answer is not obvious without this story.

IMO this comment do not track UIKit controllers brings questions (why?) rather than answering

[ncreated]

I see 👍. Improved it with discussing the reasoning and optimisation.

[buranmert]

+1 to @acgh , we rely on private information (UIKitCore is private) and that may break at any time