Fix: Implement fallback system to screens that aren't reporting on the native layer the time to display.

[lucas-zimerman]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

Context: the current native implementation doesn't always emit the required event in order to track the required time to display to be set on the time to display span.

To fix that, I introduced an additional fallback emitter, in short, it reuses the same flow by emitting the original event when the fallback emitter received the notification that the page was rendered and the original emitter didn't emit any event on the following 3 seconds.

With those changes, we have the following benefits:

• Support for React Native web by also having the JavaScript fallback of requestAnimationFrame.
• Greatly reduces the wrong time to display end timestamp by having a fallback when the original implementation doesn't return any event.
• Fallback to the JavaScript implementation in case both Native emitters fail.

💡 Motivation and Context

Close #3934, #3809

💚 How did you test it?

The playground tab on our sample is a good case for testing, since it doesn't use navigation stack on it, the events on this condition are limited, not generating and required event by us in order to track the time to display.

I used it and other tabs to compare the difference between the original implementation and also requestAnimationFrame, and the time difference between each other was quite low:

1St Tab
JS: requestAnimationFrame, "newFrameTimestampInSeconds" is 1724293664.8300002
Original: InitAsync Event received {"newFrameTimestampInSeconds":1724293664.8179998}

2Nd Tab
JS: requestAnimationFrame, "newFrameTimestampInSeconds" is 1724293761.688
Original: InitAsync Event received {"newFrameTimestampInSeconds":1724293761.689}

3Rd Tab
JS: requestAnimationFrame, "newFrameTimestampInSeconds" is 1724293824.887
Original: Android didn't emit an event for this page so it wasn't measured

Before this change on the playground screen:
https://sentry-sdks.sentry.io/performance/trace/10bd4bf28052404592f408f3cf58175a/?dataset=transactions&field=title&field=event.type&field=project&field=user.display&field=timestamp&field=replayId&fov=0%2C15046.88916015625&id=21705&name=&node=trace-root&project=5428561&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h&timestamp=1724358725&topEvents=5&yAxis=count%28%29

After this change on the playground screen:
https://sentry-sdks.sentry.io/performance/trace/8cb478b4370e444fa4a7b9be778d51ab/?dataset=transactions&field=title&field=event.type&field=project&field=user.display&field=timestamp&field=replayId&fov=0%2C235&id=21705&name=&node=txn-b5a8ba18aa254fe79743133baf9071b2&project=5428561&query=&queryDataset=transaction-like&sort=-timestamp&source=discover&statsPeriod=1h&timestamp=1724359145&topEvents=5&yAxis=count%28%29

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
• All tests passing
• No breaking changes

🔮 Next steps

[github-actions]

Android (legacy) Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	494.60 ms	475.12 ms	-19.48 ms
Size	17.73 MiB	20.11 MiB	2.38 MiB

Previous results on branch: test/deadline

Startup times

Revision	Plain	With Sentry	Diff
5ba6409	516.43 ms	521.07 ms	4.63 ms
ca043af	422.28 ms	408.35 ms	-13.93 ms
993f0a4	486.06 ms	505.04 ms	18.98 ms
5285e19	467.43 ms	471.20 ms	3.77 ms
3503ebe	484.89 ms	465.06 ms	-19.83 ms
33f0cfb	366.32 ms	409.22 ms	42.90 ms
6e1c646	457.75 ms	439.92 ms	-17.83 ms

App size

Revision	Plain	With Sentry	Diff
5ba6409	17.73 MiB	20.07 MiB	2.33 MiB
ca043af	17.73 MiB	20.11 MiB	2.38 MiB
993f0a4	17.73 MiB	20.06 MiB	2.33 MiB
5285e19	17.73 MiB	20.07 MiB	2.33 MiB
3503ebe	17.73 MiB	20.11 MiB	2.38 MiB
33f0cfb	17.73 MiB	20.07 MiB	2.33 MiB
6e1c646	17.73 MiB	20.11 MiB	2.38 MiB

[github-actions]

Android (new) Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	399.02 ms	459.98 ms	60.96 ms
Size	7.15 MiB	8.39 MiB	1.24 MiB

Previous results on branch: test/deadline

Startup times

Revision	Plain	With Sentry	Diff
993f0a4+dirty	432.25 ms	491.56 ms	59.31 ms
5285e19+dirty	418.18 ms	460.47 ms	42.29 ms
ca043af+dirty	411.53 ms	441.56 ms	30.03 ms
33f0cfb+dirty	431.30 ms	492.47 ms	61.17 ms
6e1c646+dirty	417.20 ms	482.69 ms	65.50 ms
5ba6409+dirty	436.37 ms	445.90 ms	9.53 ms
3503ebe+dirty	384.49 ms	433.86 ms	49.37 ms

App size

Revision	Plain	With Sentry	Diff
993f0a4+dirty	7.15 MiB	8.34 MiB	1.19 MiB
5285e19+dirty	7.15 MiB	8.34 MiB	1.19 MiB
ca043af+dirty	7.15 MiB	8.39 MiB	1.24 MiB
33f0cfb+dirty	7.15 MiB	8.34 MiB	1.19 MiB
6e1c646+dirty	7.15 MiB	8.39 MiB	1.24 MiB
5ba6409+dirty	7.15 MiB	8.34 MiB	1.19 MiB
3503ebe+dirty	7.15 MiB	8.39 MiB	1.24 MiB

[github-actions]

iOS (legacy) Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1232.56 ms	1232.66 ms	0.10 ms
Size	2.36 MiB	3.14 MiB	794.48 KiB

Previous results on branch: test/deadline

Startup times

Revision	Plain	With Sentry	Diff
33f0cfb+dirty	1220.29 ms	1214.19 ms	-6.10 ms
993f0a4+dirty	1238.52 ms	1241.51 ms	2.99 ms
5ba6409+dirty	1209.31 ms	1207.46 ms	-1.85 ms
ca043af+dirty	1229.50 ms	1231.31 ms	1.81 ms
6e1c646+dirty	1221.02 ms	1234.47 ms	13.45 ms
5285e19+dirty	1234.54 ms	1235.31 ms	0.76 ms
3503ebe+dirty	1222.44 ms	1228.13 ms	5.69 ms

App size

Revision	Plain	With Sentry	Diff
33f0cfb+dirty	2.36 MiB	3.08 MiB	735.59 KiB
993f0a4+dirty	2.36 MiB	3.08 MiB	732.19 KiB
5ba6409+dirty	2.36 MiB	3.08 MiB	735.56 KiB
ca043af+dirty	2.36 MiB	3.14 MiB	794.23 KiB
6e1c646+dirty	2.36 MiB	3.14 MiB	794.20 KiB
5285e19+dirty	2.36 MiB	3.08 MiB	735.56 KiB
3503ebe+dirty	2.36 MiB	3.14 MiB	794.22 KiB

[github-actions]

iOS (new) Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1239.39 ms	1243.34 ms	3.95 ms
Size	2.92 MiB	3.69 MiB	795.14 KiB

Previous results on branch: test/deadline

Startup times

Revision	Plain	With Sentry	Diff
33f0cfb+dirty	1247.61 ms	1241.02 ms	-6.59 ms
993f0a4+dirty	1241.51 ms	1235.85 ms	-5.66 ms
5ba6409+dirty	1231.40 ms	1236.81 ms	5.42 ms
ca043af+dirty	1230.57 ms	1226.55 ms	-4.02 ms
6e1c646+dirty	1232.62 ms	1239.96 ms	7.34 ms
5285e19+dirty	1228.69 ms	1218.22 ms	-10.47 ms
3503ebe+dirty	1219.50 ms	1232.69 ms	13.19 ms

App size

Revision	Plain	With Sentry	Diff
33f0cfb+dirty	2.92 MiB	3.64 MiB	741.43 KiB
993f0a4+dirty	2.92 MiB	3.64 MiB	738.79 KiB
5ba6409+dirty	2.92 MiB	3.64 MiB	741.50 KiB
ca043af+dirty	2.92 MiB	3.69 MiB	794.99 KiB
6e1c646+dirty	2.92 MiB	3.69 MiB	795.00 KiB
5285e19+dirty	2.92 MiB	3.64 MiB	741.52 KiB
3503ebe+dirty	2.92 MiB	3.69 MiB	795.00 KiB

[krystofwoldrich]

Hi @lucas-zimerman,
thank you for the detailed description, and opening the draft PR, this makes validation and discussing the ideas much easier.

I think waiting for the next drawn frame is a good fallback. But there is one party breaker hidden in the RN implementation. Compared to browsers implementation, in RN requestAnimationFrame is the same as setTimeout(timeout, 0).

requestAnimationFrame -> CreateTimer -> Timer is hook to DisplayLink of JS Thread
https://github.com/facebook/react-native/blob/c30e35fb44affb179c9a208cf0a3e4575347e76f/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp#L443

DisplayLink uses current thread -> JS Thread
https://github.com/facebook/react-native/blob/4e12c2e37cb168f09f8768941cb915e4024a1c34/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm#L354

This means the callback runs after the React Native Node Tree is assembled and passed to native. So unless the JS loop is doing some heavy task, it will generally be executed before the render.

We can fix that by implementing our own requestAnimationFrame which will emit an event from the native layer when the next frame is rendered.

onNavigationStateChanged() {
  // executed in useEffect of the root navigator 
  SENTRY_NATIVE.requestAnimationFrame((frameTimestamp) {
  	// this will actually be called on the next native frame (linked to main app thread)
  })
}

// NATIVE IMPL
SENTRY_NATIVE.requestAnimationFrame(callback) {
	// Similar to RNSentryOnDrawReporterView implementation
	// https://github.com/getsentry/sentry-react-native/blob/cc9666e872837955125bd0a80492e57625fc16a0/android/src/main/java/io/sentry/react/RNSentryOnDrawReporterManager.java#L121
	// https://github.com/getsentry/sentry-react-native/blob/2b83fc5d3de33c5eab1c8a08d762ac813e3a7c9b/ios/RNSentryOnDrawReporter.m#L44
	hookToNextFrame(() {
		callback(Date.now())
	})
}

The native implementation would the same method to hook into the render loop as RNSentryOnDrawReporterView but instead of component it would be native method of RNSentry.

I think we can still use the JS requestAnimationFrame to add support to react-native-web and maybe good fall back for Expo Go.

It should work well with the current draft, replacing requestAnimationFrame with custom method.

Let me know what you think, if my though process make sense, or I've missed something.

[lucas-zimerman]

The Android integration is done, we will now finish the iOS integration

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[krystofwoldrich]

Thank you @lucas-zimerman, it's looking good.

I've noticed one last thing with the choreographer. See #4042 (comment)

[krystofwoldrich]

I've changed the target to v5 to avoid conflicts.

But we should include this also in version 6.

First we release it as beta on v5 and then also as a hotfix on 6.0.1

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

• src/js/NativeRNSentry.ts

[krystofwoldrich]

Thank you! @lucas-zimerman

Large improvement for TTD.

[krystofwoldrich]

@lucas-zimerman

Can you port this also to v6 (main)?

[lucas-zimerman]

Sure thing, I will port it to V6