feat: UIKit instrumentation now sets the current view

[MustafaHaddara]

Which problem is this PR solving?

UIKit Instrumentation was emitting viewDidAppear and viewDidDisappear events, but wasn't persisting the "current screen" value as the SwiftUI Navigation instrumentation did in #23

Short description of the changes

UIKit Instrumentation now tracks the "current screen" value

Computing "current screen" is a little complex but tldr we use the following (in order of precedence):

• accessiblityIdentifier
• Storybook Identifier
• view.title
• current class name of the view controller

honeycombIdentifier is provided as an escape hatch for clients to opt out of this precedence chain and set some explicit value

This PR also updates our smoke test app to use nested UIKit navigations as a way to exercise the "current screen" computations.

Note that we don't do anything in the viewDidDisappear event: I considered setting the current screen to nil but opted against that because

1. it seemed like there were cases where, View B is replacing View A, View A's viewDidDisappear would run after View B's viewDidAppear. Thus setting current screen to nil would actually clobber the (now correct) value corresponding to whatever we get from View B
2. I reasoned that if one view is disappearing, then it must be replaced by some other view, and that other view is responsible to setting current screen (either automatically, if the new view is a UIKit view, or explicitly, if it's a SwiftUI view)

How to verify that this has the expected result

• run the smoke test app and manually inspect collector output
• updated smoke tests

[MustafaHaddara]

AFAICT, Apple doesn't actually document this, but StackOverflow to the rescue again

[beekhc]

In my experience with fields like this, it falls into a gray area. You're not really supposed to do it, but you're not really supposed to swizzle either, and it may break at any time. I don't really have a problem with us doing it, as one of several possible names, but I wouldn't want to rely on it too highly either... I'll reference this in the other comments.

[beekhc]

I think this should be:

return self.view.accessibilityIdentifier
    ?? self.title
    ?? self.storyboardId
    ?? NSStringFromClass(type(of: self))

From the debugger:

(lldb) po self.storyboardId
▿ Optional<String>
  - some : "UIViewController-Y6W-OH-hqX"
(lldb) po self.title
▿ Optional<String>
  - some : "UIKit Screen"

So I do think the title is better than the questionable storyboardId.

[MustafaHaddara]

The problem with this is that UINavigationController acquires the title of its root stack, so you end up with this fun duplication of names:

(the first UIKit Menu refers to the root UINavigationController controlling the whole thing, the second refers to the view it's displaying)

whereas doing storyboardId first gives us

[MustafaHaddara]

I don't know how confusing this would be for our users, or maybe I configured the storyboard incorrectly...

[beekhc]

I spent some time thinking about this, but didn't reach any conclusion. It seems that there are several conflicting constraints:

1. Listing the UINavigationController in the path with the same name as the child view kinda sucks.
2. Listing "UIViewController-Y6W-OH-hqX" in the path instead of the actual title kinda sucks.
3. Relying too much on the undocumented, unsupported storyboardId field is relatively risky.

So then I think we have these options:

1. Prefer storyboardId to title, and encourage use of the accessibilityIdentifier override.
2. Prefer title to storyboardId, and accept that part of the path is not useful. If we switch to using just the last thing in the path as the screen.name, maybe this is less of an issue?
3. Special-case our logic so that for UINavigationController, we prefer storyboardId, while for other controllers, we prefer title. We'd have to investigate other common controllers to see how they behave, although we should probably do that anyway.

I think I prefer Option 3 > 2 > 1, but I'm open to discussion.

[beekhc]

I think it would be better to remove the accessibilityIdentifier from this particular view, and have a second view at the same place in the view hierarchy that does have an accessibilityidentifier, so that we can test both flows for deriving the path. But I'm open to alternatives, if there's an easier way to test it.

[MustafaHaddara]

Well the view before this one doesn't have an accessiblityIdentifier...ie. we're already doing this. I can add a test for the touch began for the "go to ui kit app" button on that screen.

[beekhc]

I think that's fine, it just seems like we'd want to catch something being named like "UIViewController-Y6W-OH-hqX" in a test. The fact that it's an unpredictable name seems like a problem to me. I guess it depends on the resolution of the precedence discussion.

[MustafaHaddara]

The fact that it's an unpredictable name seems like a problem to me.

It's unpredictable but it will be consistent; the id is (randomly?) generated once when we add the view controller to the storyboard but it is encoded in the storyboard file.

https://github.com/honeycombio/honeycomb-opentelemetry-swift/pull/26/files#diff-3a792e4bb5fbf9f72a2eac01afcf8b8ffde6872be0cb547a88b661ca6478b02eR14

[beekhc]

Just a thought, and please feel free to push back, but do you think we should have screen.name as a whole path like this, or do you think it would be better to have screen.path and screen.name as separate fields where the name is the last segment of the path? I dunno, I don't want to complicate things, so we could just do this and revisit it later. I think the main question is how the name looks in the timeline analysis UI.

[MustafaHaddara]

That sounds reasonable, would match what we get from the manual instrumentation on the SwiftUI side.

[MustafaHaddara]

done in 6dd5460

[beekhc]

Looks good to me!