fix: Inaccurate number of frames for transactions (#3439)

The total frames were too high for transactions because the SDK subscribed twice to the CADisplayLink. This is fixed now by only subscribing once. Furthermore, this PR adds a UI test to validate that the number of total frames is not entirely off.
getsentry · Nov 22, 2023 · b064665 · b064665
1 parent 59ea421
commit b064665
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Fix inaccurate number of frames for transactions (#3439)
+
 ## 8.16.0
 
 ### Features

diff --git a/Samples/iOS-Swift/iOS-Swift.xcodeproj/project.pbxproj b/Samples/iOS-Swift/iOS-Swift.xcodeproj/project.pbxproj
@@ -662,6 +662,8 @@
 				7B64386E26A6C544000D0F65 /* PBXTargetDependency */,
 			);
 			name = "iOS-SwiftUITests";
+			packageProductDependencies = (
+			);
 			productName = "iOS-SwiftUITests";
 			productReference = 7B64386826A6C544000D0F65 /* iOS-SwiftUITests.xctest */;
 			productType = "com.apple.product-type.bundle.ui-testing";
@@ -782,6 +784,9 @@
 				Base,
 			);
 			mainGroup = 637AFD9D243B02760034958B;
+			packageReferences = (
+				6268F2C02B0DF9920019DA38 /* XCRemoteSwiftPackageReference "Nimble" */,
+			);
 			productRefGroup = 637AFDA7243B02760034958B /* Products */;
 			projectDirPath = "";
 			projectReferences = (
@@ -1291,7 +1296,7 @@
 				CODE_SIGN_STYLE = Manual;
 				DEVELOPMENT_TEAM = 97JCY7859U;
 				INFOPLIST_FILE = "iOS-SwiftUITests/Info.plist";
-				IPHONEOS_DEPLOYMENT_TARGET = 11.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 12.0;
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
 					"@executable_path/Frameworks",
@@ -1315,7 +1320,7 @@
 				CODE_SIGN_STYLE = Manual;
 				DEVELOPMENT_TEAM = 97JCY7859U;
 				INFOPLIST_FILE = "iOS-SwiftUITests/Info.plist";
-				IPHONEOS_DEPLOYMENT_TARGET = 11.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 12.0;
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
 					"@executable_path/Frameworks",
@@ -1476,7 +1481,7 @@
 				CODE_SIGN_STYLE = Manual;
 				DEVELOPMENT_TEAM = 97JCY7859U;
 				INFOPLIST_FILE = "iOS-SwiftUITests/Info.plist";
-				IPHONEOS_DEPLOYMENT_TARGET = 11.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 12.0;
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
 					"@executable_path/Frameworks",
@@ -1700,7 +1705,7 @@
 				CODE_SIGN_STYLE = Manual;
 				DEVELOPMENT_TEAM = 97JCY7859U;
 				INFOPLIST_FILE = "iOS-SwiftUITests/Info.plist";
-				IPHONEOS_DEPLOYMENT_TARGET = 11.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 12.0;
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
 					"@executable_path/Frameworks",
@@ -2089,6 +2094,17 @@
 		};
 /* End XCConfigurationList section */
 
+/* Begin XCRemoteSwiftPackageReference section */
+		6268F2C02B0DF9920019DA38 /* XCRemoteSwiftPackageReference "Nimble" */ = {
+			isa = XCRemoteSwiftPackageReference;
+			repositoryURL = "https://github.com/Quick/Nimble";
+			requirement = {
+				kind = exactVersion;
+				version = 10.0.0;
+			};
+		};
+/* End XCRemoteSwiftPackageReference section */
+
 /* Begin XCVersionGroup section */
 		D845F35927BAD4CC00A4D7A2 /* SentryData.xcdatamodeld */ = {
 			isa = XCVersionGroup;

diff --git a/Samples/iOS-Swift/iOS-SwiftUITests/LaunchUITests.swift b/Samples/iOS-Swift/iOS-SwiftUITests/LaunchUITests.swift
@@ -80,7 +80,48 @@ class LaunchUITests: BaseUITest {
 
     func testCheckSlowAndFrozenFrames() {
         app.buttons["Extra"].tap()
-        checkSlowAndFrozenFrames()
+
+        let (totalFrames, slowFrames, frozenFrames) = getFramesStats()
+
+        let slowFramesPercentage = Double(slowFrames) / Double(totalFrames)
+        XCTAssertTrue(0.5 > slowFramesPercentage, "Too many slow frames.")
+
+        let frozenFramesPercentage = Double(frozenFrames) / Double(totalFrames)
+        XCTAssertTrue(0.5 > frozenFramesPercentage, "Too many frozen frames.")
+    }
+
+    func testCheckTotalFrames() {
+        app.buttons["Extra"].tap()
+
+        let (startTotalFrames, _, _) = getFramesStats()
+        let startDate = Date()
+
+        let dispatchQueue = DispatchQueue(label: "CheckSlowAndFrozenFrames")
+        let dispatchGroup = DispatchGroup()
+        dispatchGroup.enter()
+
+        dispatchQueue.asyncAfter(deadline: .now() + 3.0) {
+            dispatchGroup.leave()
+        }
+        dispatchGroup.wait()
+
+        let (endTotalFrames, _, _) = getFramesStats()
+        let endDate = Date()
+
+        let secondsBetween = endDate.timeIntervalSince(startDate)
+
+        // Retrieving the number of frames from the text label is inaccurate.
+        // Therefore, we add a one-second tolerance for the expected amount of frames.
+        // We only want to validate if the number of total frames is entirely off.
+        let maximumFramesPerSecond = Double(UIScreen.main.maximumFramesPerSecond)
+
+        let expectedMinimumTotalFrames = (secondsBetween - 1.0) * maximumFramesPerSecond
+        let expectedMaximumTotalFrames = (secondsBetween + 1.0) * maximumFramesPerSecond
+
+        let actualTotalFrames = Double(endTotalFrames - startTotalFrames)
+
+        XCTAssertGreaterThanOrEqual(actualTotalFrames, expectedMinimumTotalFrames, "Actual frames:\(actualTotalFrames) should be greater than \(expectedMinimumTotalFrames) with frame rate of \(maximumFramesPerSecond)")
+        XCTAssertLessThanOrEqual(actualTotalFrames, expectedMaximumTotalFrames, "Actual frames:\(actualTotalFrames) should be less than \(expectedMaximumTotalFrames) with frame rate of \(maximumFramesPerSecond)")
     }
 
     /**
@@ -101,28 +142,24 @@ class LaunchUITests: BaseUITest {
         // this causes another transaction to be recorded which hits the codepath necessary for the ASAN to trip
         app.tabBars["Tab Bar"].buttons["Extra"].tap()
     }
-}
-
-private extension LaunchUITests {
 
-    func checkSlowAndFrozenFrames() {
+    private func getFramesStats() -> (Int, Int, Int) {
         let frameStatsLabel = app.staticTexts["framesStatsLabel"]
         frameStatsLabel.waitForExistence("Frame statistics message not found.")
 
         let frameStatsAsStringArray = frameStatsLabel.label.components(separatedBy: CharacterSet.decimalDigits.inverted)
         let frameStats = frameStatsAsStringArray.filter { $0 != "" }.map { Int($0) }
 
         XCTAssertEqual(3, frameStats.count)
-        guard let totalFrames = frameStats[0] else { XCTFail("No total frames found."); return }
-        guard let slowFrames = frameStats[1] else { XCTFail("No slow frames found."); return }
-        guard let frozenFrames = frameStats[1] else { XCTFail("No frozen frames found."); return }
-
-        let slowFramesPercentage = Double(slowFrames) / Double(totalFrames)
-        XCTAssertTrue(0.5 > slowFramesPercentage, "Too many slow frames.")
+        guard let totalFrames = frameStats[0] else { XCTFail("No total frames found."); return (0, 0, 0) }
+        guard let slowFrames = frameStats[1] else { XCTFail("No slow frames found."); return (0, 0, 0) }
+        guard let frozenFrames = frameStats[1] else { XCTFail("No frozen frames found."); return (0, 0, 0) }
 
-        let frozenFramesPercentage = Double(frozenFrames) / Double(totalFrames)
-        XCTAssertTrue(0.5 > frozenFramesPercentage, "Too many frozen frames.")
+        return (totalFrames, slowFrames, frozenFrames)
     }
+}
+
+private extension LaunchUITests {
 
     func assertApp() {
         let confirmation = app.staticTexts["ASSERT_MESSAGE"]

diff --git a/SentryTestUtils/TestDisplayLinkWrapper.swift b/SentryTestUtils/TestDisplayLinkWrapper.swift
@@ -30,7 +30,9 @@ public class TestDisplayLinkWrapper: SentryDisplayLinkWrapper {
         self.dateProvider = dateProvider
     }
 
+    public var linkInvocations = Invocations<Void>()
     public override func link(withTarget target: Any, selector sel: Selector) {
+        linkInvocations.record(Void())
         self.target = target as AnyObject
         self.selector = sel
     }

diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m
@@ -93,7 +93,12 @@ - (void)resetProfilingTimestamps
 
 - (void)start
 {
+    if (_isRunning) {
+        return;
+    }
+
     _isRunning = YES;
+
     [_displayLinkWrapper linkWithTarget:self selector:@selector(displayLinkCallback)];
 }
 

diff --git a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
@@ -1,3 +1,4 @@
+import Nimble
 import SentryTestUtils
 import XCTest
 
@@ -37,20 +38,39 @@ class SentryFramesTrackerTests: XCTestCase {
     }
 
     func testIsNotRunning_WhenNotStarted() {
-        XCTAssertFalse(fixture.sut.isRunning)
+        expect(self.fixture.sut.isRunning) == false
     }
 
     func testIsRunning_WhenStarted() {
         let sut = fixture.sut
         sut.start()
-        XCTAssertTrue(sut.isRunning)
+        expect(self.fixture.sut.isRunning) == true
+    }
+
+    func testStartTwice_SubscribesOnceToDisplayLink() {
+        let sut = fixture.sut
+        sut.start()
+        sut.start()
+
+        expect(self.fixture.displayLinkWrapper.linkInvocations.count) == 1
     }
 
     func testIsNotRunning_WhenStopped() {
         let sut = fixture.sut
         sut.start()
         sut.stop()
-        XCTAssertFalse(sut.isRunning)
+
+        expect(self.fixture.sut.isRunning) == false
+    }
+
+    func testStartAfterStopped_SubscribesTwiceToDisplayLink() {
+        let sut = fixture.sut
+        sut.start()
+        sut.stop()
+        sut.start()
+
+        expect(sut.isRunning) == true
+        expect(self.fixture.displayLinkWrapper.linkInvocations.count) == 2
     }
 
     func testSlowFrame() throws {