feat: release-profiler on web by kirillzyusko · Pull Request #44432 · Expensify/App

kirillzyusko · 2024-06-26T09:14:18Z

Details

Added an ability to profile web release apps.

Warning

You'll need to add 'Document-Policy': 'js-profiling' to responses to make profiler working.

Fixed Issues

$ #43363
PROPOSAL: N/A

Tests

Offline tests

N/A

QA Steps

N/A

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2024-07-05T11:18:20Z

@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

jayeshmangwani · 2024-07-07T20:47:21Z

@kirillzyusko Unit tests are failing in the PR.

kirillzyusko · 2024-07-08T09:33:52Z

@jayeshmangwani thank you for pointing it out! I fixed that!

jayeshmangwani · 2024-07-08T14:16:06Z

@kirillzyusko IMO, the share option here should be for the native platforms (Android and iOS) only. We are doing the same with the Share logs option too.

kirillzyusko · 2024-07-08T14:52:03Z

@jayeshmangwani do you want to rename "Share" to "Download"? Or do you want to remove that button at all?

If we remove and will start automatic download, then I'm not sure such concurrent downloads will be handled correctly 🤔

jayeshmangwani · 2024-07-08T15:13:15Z

Or do you want to remove that button at all?

I was thinking of removing it, as the Use profiling toggle should work the same way as the Client side logging toggle. When disabled, we download the file on web/desktop and display the Share button on native devices. However, I'm not entirely sure about this. @muttmuure Can you please suggest what you think about showing the Share button?

jayeshmangwani · 2024-07-11T09:38:50Z

@kirillzyusko We will proceed with the existing functionality added in the PR. The Profiling Share button for the web/desktop will download the file. We also need the same functionality for client-side logging. If possible, can we please add a share button to client logs in this PR that downloads the file?

kirillzyusko · 2024-07-11T12:42:27Z

If possible, can we please add a share button to client logs in this PR that downloads the file?

@jayeshmangwani I added it here: c2d7c31

However I think it's slightly incorrect... This button on web:

will trigger a download of additional App_info file that has following content:

{
    "appVersion": "9.0.4-7",
    "environment": "development",
    "platform": "web",
    "totalMemory": "7.45 GiB",
    "usedMemory": "1.36 GiB"
}

This file is an additional file for profiler stack trace, but it's not mandatory - and thus we give user an ability to download it on demand. But the trace file will be downloaded automatically.

So... Should I revert changes that I made? 😅

jayeshmangwani · 2024-07-11T14:05:09Z

So... Should I revert changes that I made? 😅

😅 In my opinion, when we press the Profile Trace Share button, the Profile_trace_for_9.0.6-1.cpuprofile and _App_Info_9.0.6-1.json files should be downloaded (similar to iOS)

kirillzyusko · 2024-07-11T16:15:05Z

😅 In my opinion, when we press the Profile Trace Share button, the Profile_trace_for_9.0.6-1.cpuprofile and _App_Info_9.0.6-1.json files should be downloaded (similar to iOS)

@jayeshmangwani the small problem here is that react-native-profiler-release had following API stopProfiling(saveToDownloads) -> pathToDownloads. And such API allowed you to re-use existing file.

But the problem is that in browser we don't have a direct access to FS, so we have to download file (and we can do that only once - at least that' how we designed the API): https://github.com/margelo/react-native-release-profiler/blob/80ff24f4ad7354c0b04047ed59e915735b986ea1/src/index.web.tsx#L84-L90

I'll be glad to unify the approach across platforms, but we need to keep in mind, that browser doesn't have FS nad Share API, so we are polyfilling them here 🤷‍♂️

muttmuure · 2024-07-12T10:25:46Z

That makes sense, it would be nice to have consistency across platforms but I think we can live without it if it means getting this out

kirillzyusko · 2024-07-15T10:22:30Z

@muttmuure @jayeshmangwani so, should I revert changes that I made for "client-side logging" functionality? Or we can review the PR with these changes?

jayeshmangwani · 2024-07-15T10:57:34Z

@kirillzyusko For the web: I think we should hide the share option for Profile Trace, as the share button does not download the profiling file but instead downloads the App_info file.

For 'client-side logging,' we should keep the share button, as it downloads the file correctly.

kirillzyusko · 2024-07-15T15:56:08Z

@jayeshmangwani got you - re-worked the code so that it meets your expectations 😊

jayeshmangwani · 2024-07-15T19:40:44Z

@kirillzyusko Thanks for making the changes 🙏

Do we have to do any extra steps to load a file in speedscope.app? I captured a file on the web and tried to open it, but I'm getting a warning: Unrecognized format! See documentation about supported formats.

File:
test.cpuprofile

jayeshmangwani · 2024-07-15T20:30:29Z

src/components/ProfilingToolMenu/BaseProfilingToolMenu.tsx

    const stop = useCallback(async () => {
-        const path = await stopProfiling(getPlatform() === CONST.PLATFORM.IOS);
-        setPathIOS(path);
+        const path = await stopProfiling(getPlatform() === CONST.PLATFORM.IOS || getPlatform() === CONST.PLATFORM.WEB, newFileName);


Do we need the condition getPlatform() === CONST.PLATFORM.WEB here?

For stopProfiling true/false both values are downloading the file in the Downloads folder for me.

@jayeshmangwani without that condition it'll always throw a warning:

https://github.com/margelo/react-native-release-profiler/blob/582a72f627bb1d1c8f1ef94c0e7031a4568c2de3/src/index.web.tsx#L86-L90

Okay, gotcha. I was suggesting that to avoid the platform-specific code in the file, though we can pass the saveToDownloads value from the platform-specific files

though we can pass the saveToDownloads value from the platform-specific files

Should I re-work this part as you are suggesting? 👀

I think it's fine here. We were already using getPlatform() === CONST.PLATFORM.IOS. Sorry for the confusion. Please leave it as it is for now.

src/components/ProfilingToolMenu/BaseProfilingToolMenu.tsx

kirillzyusko · 2024-07-16T10:33:18Z

Do we have to do any extra steps to load a file in speedscope.app? I captured a file on the web and tried to open it, but I'm getting a warning: Unrecognized format! See documentation about supported formats.

@jayeshmangwani Yes, you need to run npm run symbolicate-release:web and it'll convert from Hermes stacktrace format to Google Chrome format 👀

stitesExpensify

lgtm

OSBotify · 2024-07-22T18:59:34Z

✋ 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 · 2024-07-23T08:35:32Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.0.11-0 🚀

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

…on-web" This reverts commit cec2514, reversing changes made to 485332d.

[CP Staging] Revert "Merge pull request #44432 from margelo/feat/release-profiler-on-web"

[CP Staging] Revert "Merge pull request #44432 from margelo/feat/release-profiler-on-web" (cherry picked from commit 29cc7c1)

OSBotify · 2024-07-24T00:15:50Z

🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.0.11-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

francoisl · 2024-07-24T00:51:11Z

desktop/electron-serve.ts

+        try {
+            const data = await fs.promises.readFile(resolvedPath);
+            callback({
+                mimeType: 'text/html',


Note: I believe this may have caused issue #46048 by overriding the Content-Type response header for non-html responses.

…release-profiler-on-web"" This reverts commit d50f007.

OSBotify · 2024-07-25T03:13:48Z

🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀

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

OSBotify · 2024-07-25T03:37:53Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.0.12-0 🚀

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

OSBotify · 2024-07-26T05:36:11Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.0.13-0 🚀

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

…release-profiler-on-web"" This reverts commit d50f007.

OSBotify · 2024-08-06T04:27:29Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.0.17-0 🚀

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

OSBotify · 2024-08-07T22:03:19Z

🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.17-2 🚀

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

OSBotify · 2024-08-08T04:52:56Z

🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.17-2 🚀

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

OSBotify · 2024-08-08T20:04:20Z

🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.17-2 🚀

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

kirillzyusko changed the title ~~feat: release-profiler on web~~ [WIP] feat: release-profiler on web Jun 26, 2024

kirillzyusko force-pushed the feat/release-profiler-on-web branch from a60b8f6 to 726184b Compare June 26, 2024 13:53

jayeshmangwani mentioned this pull request Jun 29, 2024

[HOLD for payment 2024-08-14] [$250] MEDIUM: [Debugability] Add profile trace download option to Desktop and Web #43363

Closed

6 tasks

kirillzyusko force-pushed the feat/release-profiler-on-web branch from 333d1bc to 742b9a1 Compare July 4, 2024 12:16

kirillzyusko changed the title ~~[WIP] feat: release-profiler on web~~ feat: release-profiler on web Jul 5, 2024

kirillzyusko marked this pull request as ready for review July 5, 2024 11:18

kirillzyusko requested a review from a team as a code owner July 5, 2024 11:18

melvin-bot bot requested review from jayeshmangwani and removed request for a team July 5, 2024 11:18

kirillzyusko force-pushed the feat/release-profiler-on-web branch from ec590a1 to c63498e Compare July 8, 2024 09:21

kirillzyusko force-pushed the feat/release-profiler-on-web branch from d8b3730 to c2d7c31 Compare July 11, 2024 12:36

kirillzyusko force-pushed the feat/release-profiler-on-web branch from c2d7c31 to 2430ed3 Compare July 15, 2024 15:08

jayeshmangwani reviewed Jul 15, 2024

View reviewed changes

src/components/ProfilingToolMenu/BaseProfilingToolMenu.tsx Outdated Show resolved Hide resolved

kirillzyusko added 4 commits July 22, 2024 18:28

fix: disable share button

1674da7

fix: prettier

c6765ba

fix: changes after code review

67a0ab5

fix: re-generate package-lock.json

6f7f259

kirillzyusko force-pushed the feat/release-profiler-on-web branch from b75c3e5 to 6f7f259 Compare July 22, 2024 16:29

stitesExpensify approved these changes Jul 22, 2024

View reviewed changes

stitesExpensify merged commit cec2514 into Expensify:main Jul 22, 2024

github-actions bot mentioned this pull request Jul 23, 2024

Deploy Checklist: New Expensify 2024-07-23 #45991

Closed

95 tasks

francoisl added a commit that referenced this pull request Jul 23, 2024

Revert "Merge pull request #44432 from margelo/feat/release-profiler-…

d50f007

…on-web" This reverts commit cec2514, reversing changes made to 485332d.

This was referenced Jul 23, 2024

[HOLD for payment 2024-08-02] [HOLD for payment 2024-08-01] Desktop - App - App font is different on staging #46048

Closed

[CP Staging] Revert "Merge pull request #44432 from margelo/feat/release-profiler-on-web" #46070

Merged

srikarparsi added a commit that referenced this pull request Jul 23, 2024

Merge pull request #46070 from Expensify/francois-revert-44432

29cc7c1

[CP Staging] Revert "Merge pull request #44432 from margelo/feat/release-profiler-on-web"

OSBotify pushed a commit that referenced this pull request Jul 23, 2024

Merge pull request #46070 from Expensify/francois-revert-44432

9d7af96

[CP Staging] Revert "Merge pull request #44432 from margelo/feat/release-profiler-on-web" (cherry picked from commit 29cc7c1)

francoisl reviewed Jul 24, 2024

View reviewed changes

kirillzyusko added a commit to margelo/Expensify that referenced this pull request Jul 24, 2024

Revert "Revert "Merge pull request Expensify#44432 from margelo/feat/…

dd51cb3

…release-profiler-on-web"" This reverts commit d50f007.

github-actions bot mentioned this pull request Jul 25, 2024

Deploy Checklist: New Expensify 2024-07-25 #46161

Closed

64 tasks

kirillzyusko mentioned this pull request Jul 25, 2024

feat: web profiler v2 #46133

Merged

48 tasks

kirillzyusko added a commit to margelo/Expensify that referenced this pull request Jul 31, 2024

Revert "Revert "Merge pull request Expensify#44432 from margelo/feat/…

5dd4330

…release-profiler-on-web"" This reverts commit d50f007.

kirillzyusko added a commit to margelo/Expensify that referenced this pull request Aug 2, 2024

Revert "Revert "Merge pull request Expensify#44432 from margelo/feat/…

8e8e963

…release-profiler-on-web"" This reverts commit d50f007.

github-actions bot mentioned this pull request Aug 6, 2024

Deploy Checklist: New Expensify 2024-08-06 #46849

Closed

83 tasks

Conversation

kirillzyusko commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jul 5, 2024

Uh oh!

jayeshmangwani commented Jul 7, 2024

Uh oh!

kirillzyusko commented Jul 8, 2024

Uh oh!

jayeshmangwani commented Jul 8, 2024

Uh oh!

kirillzyusko commented Jul 8, 2024

Uh oh!

jayeshmangwani commented Jul 8, 2024

Uh oh!

jayeshmangwani commented Jul 11, 2024

Uh oh!

kirillzyusko commented Jul 11, 2024

Uh oh!

jayeshmangwani commented Jul 11, 2024

Uh oh!

kirillzyusko commented Jul 11, 2024

Uh oh!

muttmuure commented Jul 12, 2024

Uh oh!

kirillzyusko commented Jul 15, 2024

Uh oh!

jayeshmangwani commented Jul 15, 2024

Uh oh!

kirillzyusko commented Jul 15, 2024

Uh oh!

jayeshmangwani commented Jul 15, 2024

Uh oh!

jayeshmangwani Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

kirillzyusko Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

jayeshmangwani Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

kirillzyusko Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

jayeshmangwani Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kirillzyusko commented Jul 16, 2024

Uh oh!

stitesExpensify left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Jul 22, 2024

Uh oh!

OSBotify commented Jul 23, 2024

Uh oh!

OSBotify commented Jul 24, 2024

Uh oh!

francoisl Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Jul 25, 2024

Uh oh!

OSBotify commented Jul 25, 2024

Uh oh!

OSBotify commented Jul 26, 2024

Uh oh!

OSBotify commented Aug 6, 2024

Uh oh!

OSBotify commented Aug 7, 2024

kirillzyusko commented Jun 26, 2024 •

edited

Loading