Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.11k stars 2.61k forks source link

[$250] MEDIUM: [Debugability] Add profile trace download option to Desktop and Web #43363

Open muttmuure opened 3 weeks ago

muttmuure commented 3 weeks ago

If you havenโ€™t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


What performance issue do we need to solve?

e.g. memory consumption, storage read/write times, React native bridge concerns, inefficient React component rendering, etc.

Desktop and web performance is currently difficult to evaluate compared with mobile because we don't support profile tracing

What is the impact of this on end-users?

List specific user experiences that will be improved by solving this problem e.g. app boot time, time to for some interaction to complete, etc.

Desktop and web users are unable to have their performance issues fully triaged.

List any benchmarks that show the severity of the issue

Please also provide exact steps taken to collect metrics above if any so we can independently verify the results.

Triaging this issue would benefit from having a profile trace

Proposed solution (if any)

Please list out the steps you think we should take to solve this issue.

Add web support to react-native-release-profiler Make the "share profile" flow work on web as well

Add profile trace functionality to the desktop and web apps, in the Troubleshoot menu and four-finger-tap menu

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

Note: These should be the same as the benchmarks collected before any changes.

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: Reproducible in staging?: Reproducible in production?: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: Issue reported by: @muttmuure Slack conversation:

View all open jobs on Upwork

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018b1cc2a053306cc3
  • Upwork Job ID: 1801178849066862909
  • Last Price Increase: 2024-06-20
Issue OwnerCurrent Issue Owner: @jayeshmangwani
melvin-bot[bot] commented 3 weeks ago

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

melvin-bot[bot] commented 3 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~018b1cc2a053306cc3

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani (External)

melvin-bot[bot] commented 2 weeks ago

@jayeshmangwani Huh... This is 4 days overdue. Who can take care of this?

jayeshmangwani commented 2 weeks ago

Waiting on proposal, bumped on #expensify-open-source

melvin-bot[bot] commented 2 weeks ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

hannojg commented 1 week ago

Hey, Hanno from Margelo here ๐Ÿ‘‹ we built react-native-release-profiler the library we use on mobile for recording performance profiles. We can enable recording profiles for desktop (and web), by adding web support to the library. Feel free to assign me!

kirillzyusko commented 1 week ago

Hey, Kiryl from Margelo here ๐Ÿ‘‹

@hannojg asked me to work on this task. Can you please assign me on this task? ๐Ÿ‘€

melvin-bot[bot] commented 1 week ago

@hannojg, @kirillzyusko, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick!

kirillzyusko commented 1 week ago

I'm working on this task. At the moment I can capture profile traces and I'm discovering the best option for representing this data (obviously it has to match Google Chrome parser standards). Right now I have two options:

I think tomorrow or the day after tomorrow the PR will be ready for review ๐Ÿ‘€

jayeshmangwani commented 1 week ago

Not Overdue! Kirill is working on this issue. The last update is here, and the PR will be ready for review in a day or two.

kirillzyusko commented 6 days ago

Still working on it! Basic functionality works - I just need to verify, that source map support works as well and it can correctly show function names for minified code ๐Ÿ‘€

melvin-bot[bot] commented 6 days ago

@hannojg, @kirillzyusko, @jayeshmangwani Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

jayeshmangwani commented 5 days ago

PR is in Draft

muttmuure commented 2 days ago

How is this coming along? Do you need an internal reviewer to help out too?

melvin-bot[bot] commented 2 days ago

@hannojg, @kirillzyusko, @jayeshmangwani Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

hannojg commented 1 day ago

Hey, we are done with:

The only thing we are currently working on is getting the source map symbolication to work correctly. On web/desktop, when we make a release build the js code gets obfuscated / the names get mangled. We are currently trying to see if we can create a source map with de-mangled names. Working on this today with Kiryl, should have a new update by EOD!

muttmuure commented 1 day ago

amazing

hannojg commented 1 day ago

Hey, sorry our availability was a bit limited today - no new updates yet. I'll look into this first thing tomorrow morning!

hannojg commented 14 hours ago

Alright, so here are our findings:

That being said, the function names might look weird, but we still get the file name + line, which is pretty usable:

image

Note: We can build a script that will use some open source solutions to bring back the function names, but that would probably be a endeavour of multiple days and we didn't want to block this issue by this (as there is no direct tool for this and we'd need to build one). We can maybe open a new issue for that (would take 1-2 days to get that working)?

Next steps on the PR are:

melvin-bot[bot] commented 5 hours ago

@hannojg, @kirillzyusko, @jayeshmangwani Eep! 4 days overdue now. Issues have feelings too...