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

[No QA] Add React Native Firebase Performance monitoring #4173

Merged
merged 7 commits into from
Jul 22, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jul 22, 2021

Details

This PR adds some new events to the Firebase dashboard for Expensify/App:

  • App start time - how long the app takes to launch
  • js_loaded custom event - how long the app takes to launch + for the main app component to mount

This should give us some valuable metrics for detecting material improvements or regressions to app start time.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/171518

Tests

The metrics are disabled when in DEBUG to avoid logging bad values for now. Dev builds take considerably longer to start up because the JS is bundled in metro and the app is technically "launched" while that's happening.

To get around this we need to create release builds. Instructions for how to do that are here:

How do I run iOS Native Chat release builds on the iOS Simulator?
How do I run Android Expensify.cash release builds in Android Studio?

iOS / Android

  1. Log into firebase dashboard
  2. Build the app
  3. Wait til it loads and force quit
  4. Repeat a few times.
  5. Note that the Performance dashboard has updated with new times

Screen Shot 2021-07-21 at 4 08 44 PM

To verify we are only running this in debug we can build the apps normally (debug) and grep for this log:

[StartupTimer] Metric tracing disabled in DEBUG

QA Steps

No QA

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Jul 22, 2021
@marcaaron marcaaron requested review from Julesssss and a team July 22, 2021 02:27
@MelvinBot MelvinBot requested review from tylerkaraszewski and removed request for a team July 22, 2021 02:27
@marcaaron marcaaron changed the title [WIP] [No QA] Add React Native Firebase Performance monitoring [No QA] Add React Native Firebase Performance monitoring Jul 22, 2021
@marcaaron marcaaron marked this pull request as ready for review July 22, 2021 02:34
@marcaaron marcaaron requested a review from a team as a code owner July 22, 2021 02:34
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team July 22, 2021 02:34
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

I haven't ran the tests, just reviewed the code. But the implementation looks good to me, I have no changes to suggest.

I wonder if it might be worth migrating the Grafana timers to Firebase too later on -- as Firebase is much more closely tied to the app dashboards and perhaps more likely to be used.

import com.google.firebase.perf.FirebasePerformance;
import com.google.firebase.perf.metrics.Trace;

public class StartupTimer extends ReactContextBaseJavaModule {
Copy link
Contributor

@Julesssss Julesssss Jul 22, 2021

Choose a reason for hiding this comment

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

NAB: Actually, my only comment is that maybe this could be made generic, so that each timing trace doesn't require a new Java class. But I'm sure you're aware of this, and it's not necessary until we add the second trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I guess my thinking here was that we really only need a single timer in the native layer (when the app starts) for now. If we need to do performance traces with firebase we can do that via the JS later and this package

https://rnfirebase.io/perf/usage#custom-tracing

@marcaaron
Copy link
Contributor Author

I wonder if it might be worth migrating the Grafana timers to Firebase too later on -- as Firebase is much more closely tied to the app dashboards and perhaps more likely to be used.

Good thought @Julesssss! I think we can pretty easily move some of the Grafana stuff to Firebase. The only thing is Firebase will not tell us anything about web - so if we want to move some of the traces to Firebase then they will be split between the two platforms. Not necessarily a bad thing. I think it's more likely we'll be worried about native performance than web performance in the long run anyway.

@marcaaron marcaaron merged commit 1f12c30 into main Jul 22, 2021
@marcaaron marcaaron deleted the marcaaron-reactNativeFirebase branch July 22, 2021 16:48
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants