getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
817 stars 330 forks source link

reportFullDisplay finishing the wrong span #4533

Open brustolin opened 2 weeks ago

brustolin commented 2 weeks ago

Description

There is a problem with the reportFullDisplay API:

This will finish TTFD for ViewControllerB, but it is still waiting for its request to respond.

Test inside SentryUIViewControllerPerformanceTrackerTests.swift from https://github.com/getsentry/sentry-cocoa/pull/4531 to reproduce the problem:

func test_waitForFullDisplay_NewViewControllerLoaded_BeforeReportTTFD_withBackgroundWork() throws {
    class CustomVC: UIViewController {
        var workSpan: Span?

        override func viewDidAppear(_ animated: Bool) {
            super.viewDidAppear(animated)
            //Start some work, it could be a http request
            workSpan = SentrySDK.span?.startChild(operation: "Work")
        }

        // We will use a function to finish the work
        // but it could be the request response
        func finishWork() {
            workSpan?.finish()
            SentrySDK.reportFullyDisplayed()
        }
    }

    let dateProvider = fixture.dateProvider
    let sut = fixture.getSut()
    let tracker = fixture.tracker
    let firstController = CustomVC()
    let secondController = CustomVC()

    var firstTracer: SentryTracer?
    var secondTracer: SentryTracer?

    sut.enableWaitForFullDisplay = true

    dateProvider.advance(by: 1)
    sut.viewControllerLoadView(firstController) {
        firstController.loadView()
        firstTracer = self.getStack(tracker).first as? SentryTracer
    }

    sut.viewControllerViewDidLoad(firstController) {
        firstController.viewDidLoad()
    }

    dateProvider.advance(by: 1)
    sut.viewControllerViewWillAppear(firstController) {
        firstController.viewWillAppear(false)
    }

    sut.viewControllerViewDidAppear(firstController) {
        firstController.viewDidAppear(false)
    }

    fixture.displayLinkWrapper.normalFrame()

    let firstFullDisplaySpan = try XCTUnwrap(firstTracer?.children.first { $0.operation == "ui.load.full_display" })

    XCTAssertFalse(firstFullDisplaySpan.isFinished)

    XCTAssertEqual(firstTracer?.traceId, SentrySDK.span?.traceId)

    dateProvider.advance(by: 1)
    sut.viewControllerLoadView(secondController) {
        secondController.loadView()
        secondTracer = self.getStack(tracker).first as? SentryTracer
    }

    // The work of the first controller asynchronously ended here, and a new frame was rendered
    // while the second controller hasn’t appeared yet.
    dateProvider.advance(by: 1)
    firstController.finishWork()
    fixture.displayLinkWrapper.normalFrame()

    dateProvider.advance(by: 1)
    sut.viewControllerViewWillAppear(secondController) {
        secondController.viewWillAppear(false)
    }
    sut.viewControllerViewDidAppear(secondController) {
        secondController.viewDidAppear(false)
    }
    fixture.displayLinkWrapper.normalFrame()

    dateProvider.advance(by: 1)
    let timeOfSecondControllerFinishWork = dateProvider.date()
    secondController.finishWork()

    fixture.displayLinkWrapper.normalFrame()

    XCTAssertTrue(firstFullDisplaySpan.isFinished)
    XCTAssertEqual(.deadlineExceeded, firstFullDisplaySpan.status)

    let secondFullDisplaySpan = try XCTUnwrap(secondTracer?.children.first { $0.operation == "ui.load.full_display" })

    XCTAssertEqual(secondFullDisplaySpan.timestamp, timeOfSecondControllerFinishWork)
}
philipphofmann commented 2 days ago

The proper solution for this is to extend the reportFullyDisplay method to allow passing in an argument to tell the SDK for which UIViewController fullyDisplay is reported. We still need to figure out what this API looks like, and we should align across all mobile SDKs.