[No QA] Performance tracking with flipper-plugin-performance #4760

kidroca · 2021-08-19T22:19:38Z

Details

Adds the Flipper side of react-native-performance where we can see a timeline of our performance updates

In order to see some data on the timeline I've integrated the <Performance> component similar to how
it's used on the example project here: https://github.com/oblador/react-native-performance/blob/ccfe4557350a0ef206268e9a50317d9cc2480464/examples/vanilla/App.tsx#L28

Related Issues

$ #4656
$ #4549
$ Expensify/react-native-onyx#101 (package hash should be updated after the onyx PR is merged)

Tests

Update local .env configuration and set CAPTURE_METRICS to true
Reset the metro cache since there's a babel config change npm start -- --reset-cache
Build a debug build with bundled JS (optional) - see how here: [Performance] Improve Android Cold Boot TTI (Time To Interactive) #4656 (comment)
Launch Flipper (desktop app) and install the "Performance" plugin. Search for flipper-plugin-performance
Start the App (kill the metro server if you opted to use the bundled JS bytecode)
Observe data on the Performance timeline in Flipper

QA Steps

N/A

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Onyx captures a lot of measures. We can inspect those on the Flipper Timeline

Update the metro configuration so that when `CAPTURE_METRICS` is `true` we pass aliases to allow capturing data with the Profiler component Otherwise React would just skip capturing metrics in release

kidroca

Added some notes on the changes

babel.config.js

package.json

src/libs/Performance.js

src/libs/actions/App.js

src/pages/home/report/ReportActionsView.js

kidroca · 2021-08-19T22:52:56Z

Onyx metrics add a lot of bars to the timeline this might be distracting, we might update the CAPTURE_METRICS variable to be something like

all -> capture and show all metrics on the timeline
onyx -> show only Onyx metrics
app -> show only app metrics
app,onyx:get,onyx:set,onyx:mergeCollection -> app and selected Onyx methods
false -> no metrics capturing and reporting

Usages through `process.env` cover missing .env files

kidroca · 2021-08-20T08:15:44Z

Ready for review

Tested on all platforms

Perf metrics are only captured on iOS and Android
Verified the rest of the platform are unaffected by the changes

Note: we can either revert or update the hash to react-native-onyx depending on whether this PR gets merged first: Capture metrics using react-native-performance react-native-onyx#101

marcaaron

Great changes! PR is really awesome! Just had some comments/questions...

babel.config.js

src/components/OnyxProvider.js

src/libs/Performance.js

src/libs/actions/App.js

src/pages/home/report/ReportActionsView.js

src/pages/home/sidebar/SidebarLinks.js

kidroca

Replied to the questions
Would push updates on Monday

src/components/OnyxProvider.js

src/libs/Performance.js

src/libs/actions/App.js

src/pages/home/report/ReportActionsView.js

…art marks

…RenderTrace

kidroca · 2021-08-23T16:50:35Z

Addressed requested changes. Ready for review

marcaaron

LGTM, @Luke9389 do you want to take a look at this?

marcaaron · 2021-08-23T17:20:34Z

react-native-onyx PR merged - Expensify/react-native-onyx@6eadd7f

kidroca · 2021-08-23T21:14:17Z

Onyx hash updated.
Removed [Hold]
Should this be flagged as [No QA]?

Luke9389 · 2021-08-23T23:43:45Z

Hey, sorry. Getting caught up here.

Luke9389 · 2021-08-23T23:54:12Z

This is an awesome PR @kidroca

Yea, we can flag this as [No QA] I think.
I'm gonna run through the tests you gave and report back.

Luke9389 · 2021-08-24T00:37:23Z

@kidroca are you using flipper with the non-mobile platforms as well?

It worked for me on your branch with iPhone, but I didn't get any devices recognized on Flipper for web.

Here are my iPhone results

Luke9389

I read through the review iterations to get familiar with some of these changes. Tested on my end and looks good.

marcaaron · 2021-08-24T02:26:26Z

Thanks @Luke9389 Flipper is for native platforms only :) can use Chrome Dev Tools for testing the website - just without as much fanciness. Some things are easier - other things less so.

OSBotify · 2021-08-24T02:27:24Z

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

dklymenk · 2021-08-24T06:06:30Z

Hi, I'm sorry to intervene, but I have an issue with web version not building:

ERROR in ./node_modules/react-native-flipper/index.js 35:11
Module parse failed: Unexpected token (35:11)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file.
See https://webpack.js.org/concepts#loaders
|
| class Connection {
>   connected;
|   pluginId;
|
 @ ./node_modules/react-native-performance-flipper-reporter/src/index.js 1:0-49 90:2-11
 @ ./src/libs/Performance.js
 @ ./src/libs/actions/App.js
 @ ./src/libs/Navigation/NavigationRoot.js
 @ ./src/Expensify.js
 @ ./src/App.js
 @ ./index.js

I made sure my packages are up to date and I haven't done any changes to my .env or anything. Commenting out these 2 lines fixes the problem. I am not exactly sure if this is a missing package issue or that code is not meant to be run on web at all.

Anyway, I am up-to-date on main and last commit at the time of posting is 8a5cf2d.

Thanks.

kidroca · 2021-08-24T06:57:45Z

I made sure my packages are up to date

So you've installed the new packages with npm install when you pulled main ?
That should have been enough
You can also try running npm ci this would remove node_modules and install the exact dependencies from the -lock file

The code is not supposed to run on web, but the bundler analyzes all require calls as it can't tell if the code would not be used from conditions like if (canCapturePerformanceMetrics())
If you have all packages installed it shouldn't be a problem

Hi, I'm sorry to intervene, but I have an issue with web version not building:

ERROR in ./node_modules/react-native-flipper/index.js 35:11
Module parse failed: Unexpected token (35:11)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file.
See https://webpack.js.org/concepts#loaders
|
| class Connection {
>   connected;
|   pluginId;
|
 @ ./node_modules/react-native-performance-flipper-reporter/src/index.js 1:0-49 90:2-11
 @ ./src/libs/Performance.js
 @ ./src/libs/actions/App.js
 @ ./src/libs/Navigation/NavigationRoot.js
 @ ./src/Expensify.js
 @ ./src/App.js
 @ ./index.js
I made sure my packages are up to date and I haven't done any changes to my .env or anything. Commenting out these 2 lines fixes the problem. I am not exactly sure if this is a missing package issue or that code is not meant to be run on web at all.

Anyway, I am up-to-date on main and last commit at the time of posting is 8a5cf2d.

Thanks.

kidroca · 2021-08-24T12:49:11Z

I did another go on mac with npm ci and I get the error as well
Besides the error getting printed on the console everything seems to work while developing (npm run web, npm run desktop)
But then I tried to make a prod bundle with npm run build and the build failed with the same error

The issue is related to this

The code is not supposed to run on web, but the bundler analyzes all require calls as it can't tell if the code would not be used from conditions like if (canCapturePerformanceMetrics())

Because of the above, webpack would follow imports to node_modules/react-native-flipper and fail to parse the file as we exclude most files from node_modules

Adding react-native-flipper here fixes the issue:

App/config/webpack/webpack.common.js

Lines 14 to 23 in 8a5cf2d

    
           const includeModules = [ 
        
               'react-native-animatable', 
        
               'react-native-reanimated', 
        
               'react-native-picker-select', 
        
               'react-native-web', 
        
               '@react-native-picker', 
        
               'react-native-modal', 
        
               'react-native-onyx', 
        
               'react-native-gesture-handler', 
        
           ].join('|');

This would mean that some unused code gets bundled into the end bundle:

It doesn't add much, but still I think the right thing to do is not to include unused modules

This prevents an error related to bundling: Expensify#4760 (comment)

…per-plugin [No QA] Fix regression on web due to #4760

OSBotify · 2021-08-30T17:37:57Z

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

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

botify · 2021-09-01T01:26:13Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-08. 🎊

kidroca added 7 commits August 20, 2021 00:22

Enable Onyx.captureMetrics with canCapturePerformanceMetrics

942d418

add react-native-performance-flipper-reporter

b30fde0

Add performance capturing sugar

8d1524a

Capture performance marks for init and chat switches

23f0ad4

Update onyx hash to see more data on the perf timeline

9bceb08

Alert only key metrics

acc391f

Onyx captures a lot of measures. We can inspect those on the Flipper Timeline

Allow using the <Profiler> in release

9a9dc38

Update the metro configuration so that when `CAPTURE_METRICS` is `true` we pass aliases to allow capturing data with the Profiler component Otherwise React would just skip capturing metrics in release

kidroca commented Aug 19, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

kidroca changed the title ~~[HOLD] Kidroca/add rn performance flipper plugin~~ Performance tracking with flipper-plugin-performance Aug 20, 2021

kidroca changed the title ~~Performance tracking with flipper-plugin-performance~~ [HOLD] Performance tracking with flipper-plugin-performance Aug 20, 2021

kidroca added 3 commits August 20, 2021 10:05

Update docs in libs/Performance

ff179e4

Update Podfile.lock after flipper updates

6e20694

Update babel.config ENV handling

fdd1ee1

Usages through `process.env` cover missing .env files

kidroca marked this pull request as ready for review August 20, 2021 08:11

kidroca requested a review from a team as a code owner August 20, 2021 08:11

MelvinBot requested review from Luke9389 and removed request for a team August 20, 2021 08:11

marcaaron requested changes Aug 20, 2021

View reviewed changes

kidroca commented Aug 20, 2021

View reviewed changes

kidroca added 7 commits August 23, 2021 18:50

Performance: add exception handling to cover errors due to missing st…

78373e2

…art marks

Measure initial report render and report switches separately

87dc0fd

Add ONYX_METRICS to .env and expose checks

a5e623a

Better document babel.config aliasing change

f2b27a3

Remove mount time measures from ReportActionsView - covered by with…

36c62c2

…RenderTrace

Move mark capture to SidebarScreen.js

4427cd9

Convert Performance to single default export

9ea80ba

kidroca force-pushed the kidroca/add-rn-performance-flipper-plugin branch from 7c0282e to 9ea80ba Compare August 23, 2021 16:45

kidroca requested a review from marcaaron August 23, 2021 16:50

marcaaron previously approved these changes Aug 23, 2021

View reviewed changes

Update Onyx hash - perf metrics

5201038

kidroca dismissed marcaaron’s stale review via 5201038 August 23, 2021 21:10

kidroca changed the title ~~[HOLD] Performance tracking with flipper-plugin-performance~~ Performance tracking with flipper-plugin-performance Aug 23, 2021

Luke9389 approved these changes Aug 24, 2021

View reviewed changes

marcaaron merged commit 95bf36d into Expensify:main Aug 24, 2021

marcaaron changed the title ~~Performance tracking with flipper-plugin-performance~~ [No QA] Performance tracking with flipper-plugin-performance Aug 24, 2021

kidroca added a commit to kidroca/Expensify.cash that referenced this pull request Aug 24, 2021

Add react-native-flipper to includeModules

c2d1d8e

This prevents an error related to bundling: Expensify#4760 (comment)

kidroca mentioned this pull request Aug 24, 2021

[No QA] Fix regression on web due to #4760 #4800

Merged

5 tasks

marcaaron added a commit that referenced this pull request Aug 24, 2021

Merge pull request #4800 from kidroca/kidroca/add-rn-performance-flip…

5f6c147

…per-plugin [No QA] Fix regression on web due to #4760

This was referenced Aug 24, 2021

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

Deploy Checklist: New Expensify 2021-08-25 #4828

Closed

kidroca mentioned this pull request Aug 27, 2021

Cap Software - External Contributors August 2021 #4850

Closed

9 tasks

This was referenced Jan 10, 2022

Android build failing after RN upgrade #7111

Closed

[No QA] Fix Android deploy failures #7112

Merged

[No QA] Performance tracking with flipper-plugin-performance #4760

[No QA] Performance tracking with flipper-plugin-performance #4760

Uh oh!

Conversation

kidroca commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

kidroca commented Aug 19, 2021

Uh oh!

kidroca commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kidroca commented Aug 23, 2021

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Aug 23, 2021

Uh oh!

kidroca commented Aug 23, 2021

Uh oh!

Luke9389 commented Aug 23, 2021

Uh oh!

Luke9389 commented Aug 23, 2021

Uh oh!

Luke9389 commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luke9389 left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Aug 24, 2021

Uh oh!

OSBotify commented Aug 24, 2021

Uh oh!

dklymenk commented Aug 24, 2021

Uh oh!

kidroca commented Aug 24, 2021

kidroca commented Aug 19, 2021 •

edited

Loading

kidroca commented Aug 20, 2021 •

edited

Loading

Luke9389 commented Aug 24, 2021 •

edited

Loading