Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tracing): Add automatic tracing of time to initial display for react-navigation #3588

Merged
merged 51 commits into from
Mar 18, 2024

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Feb 12, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This PR adds automatic TTID spans and measurements for React Navigation version 5 and newer. When TTID enables new navigation.processing span is added. This represents the duration from navigation dispatch to the new screen resolved in JS. This was the original minimum transaction duration.

Technical details

On iOS React Navigation uses ViewControllers to create RN Screens. We swizzle the RNSScreen view controller viewDidAppear to start listening for the next new frame. We can't start listening to the new frame when the JS layer is done with the navigation, because the next frame after that will be from the old screen/animation to the new screen.

A current limitation is that this works only for native view controller animations. Animations initiated from JS happen after the view didAppear.

On Android React Navigation creates Fragments to create RN Screens. We hook into onFragmentViewCreated where we check if the fragment is RNSScreenFragment. If that is we hook onto the event dispatcher attached to that fragment and wait for appeareEvent and then for the next rendered frame.

We can't use the JS viewApear hook as this is processed by the JS Engine asynchronously from the main application thread and might happen after (depending on the JS loop queue) the first frame of the screen is rendered.

Known limitations

  • Needs native code so it doesn't work in Expo Go
  • On iOS JS initiated animations, happen after the view controller first frame
  • There doesn't seem to be a way to hook into the View event dispatcher used by the RNScreens on iOS, so we had to use swizzling.

How to enable TTID

import * as Sentry from "@sentry/react-native";

const routingInstrumentation = new Sentry.ReactNavigationInstrumentation({
  enableTimeToInitialDisplay: true,
});

Sentry.init({
  integrations: [
    new Sentry.ReactNativeTracing({routingInstrumentation}),
  ],
});

Examples in Sentry

Example 1: https://sentry-sdks.sentry.io/discover/sentry-react-native:7432443eb31a4166be70d4b8d63b040b
Screenshot 2024-02-16 at 10 40 30

Example 2: https://sentry-sdks.sentry.io/discover/sentry-react-native:4429cff3025b4ec8aa2b128f1d564459
Screenshot 2024-02-16 at 10 43 10

💡 Motivation and Context

💚 How did you test it?

sample app, unit and integration tests

📝 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

Copy link
Contributor

github-actions bot commented Feb 12, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 9b833b2

Copy link
Contributor

github-actions bot commented Feb 19, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 422.54 ms 449.60 ms 27.06 ms
Size 17.73 MiB 19.92 MiB 2.19 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms
acadc0f+dirty 373.24 ms 381.51 ms 8.27 ms
9a3ca65+dirty 326.93 ms 330.14 ms 3.21 ms
ad6c299 375.94 ms 382.02 ms 6.08 ms
9433f35 347.64 ms 356.22 ms 8.58 ms
d7401ac+dirty 375.20 ms 383.51 ms 8.31 ms
15c80ab+dirty 336.27 ms 350.58 ms 14.31 ms
86d6d2c+dirty 332.90 ms 352.45 ms 19.55 ms
3853f43 329.68 ms 346.32 ms 16.64 ms
22e31b6 396.48 ms 419.64 ms 23.16 ms

App size

Revision Plain With Sentry Diff
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
acadc0f+dirty 17.73 MiB 19.75 MiB 2.01 MiB
9a3ca65+dirty 17.73 MiB 20.04 MiB 2.31 MiB
ad6c299 17.73 MiB 19.75 MiB 2.02 MiB
9433f35 17.73 MiB 19.81 MiB 2.08 MiB
d7401ac+dirty 17.73 MiB 19.75 MiB 2.02 MiB
15c80ab+dirty 17.73 MiB 20.04 MiB 2.31 MiB
86d6d2c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
3853f43 17.73 MiB 19.81 MiB 2.08 MiB
22e31b6 17.73 MiB 19.84 MiB 2.10 MiB

@krystofwoldrich krystofwoldrich marked this pull request as ready for review February 28, 2024 12:41
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I mainly focused on the Android parts, left one comment up for discussion

return () -> {
final SentryDate endDate = dateProvider.now();
WritableMap event = Arguments.createMap();
event.putDouble("newFrameTimestampInSeconds", endDate.nanoTimestamp() / 1e9);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's fine, but nanoTimestamp() will only give you a millisecond precision, see the issue linked here.
I see the duration is calculated via this._latestTransaction?.startTimestamp, but couldn't find the setter for .startTimestamp.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the note, the RN measurements have the nanoseconds precision (RN impl of performance now), this is used for the startTimestamp.

But it doesn't seem to be an issue, looking at the test transactions.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good from the cocoa point of view! 😉

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good from the JavaScript point of view!

@krystofwoldrich krystofwoldrich merged commit 9496de1 into main Mar 18, 2024
55 of 57 checks passed
@krystofwoldrich krystofwoldrich deleted the @krystofwoldrich/add-ttid branch March 18, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants