-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[NoQA] Add memory usage breadcrumbs and context to Sentry spans #81344
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
[NoQA] Add memory usage breadcrumbs and context to Sentry spans #81344
Conversation
|
@mjasikowski 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f52a56657f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
PR doesn’t need product input as a tooling PR. Unassigning and unsubscribing myself. |
…ck-memory-usage-in-sentry-attributes-fix
|
@truph01 can you review this one today? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-06.at.23.18.18.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-02-06.at.23.19.28.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-05.at.15.44.01.mov |
|
@szymonzalarski98 In Android simulator, I have this log result: The |
Thank you, I've pushed fixes for this one |
|
@szymonzalarski98 Please comment on the AI comments and resolve them if there is nothing you want to discuss with the C+ or internal engineer |
Done, resolved all conversations with AI, left some comments, don't need to discuss it with other engineer. |
|
@truph01 can you retest please? |
|
Testing again |
| * - Thresholds: >600MB error, >300MB warning (supports iPhone 8+) | ||
| * - Note: iPhone 8/X jetsam ~300-350MB, iPhone 11+ ~400-600MB | ||
| */ | ||
| if (memoryInfo.platform === 'web') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to introduce a utility function like getLogLevel(memoryInfo: MemoryInfo), with platform-specific implementations (.web.ts, .ios.ts, .android.ts), instead of branching on memoryInfo.platform here?
| // Build breadcrumb message based on platform | ||
| let breadcrumbMessage: string; | ||
| if (memoryInfo.platform === 'web') { | ||
| const maxMB = memoryInfo.maxMemoryBytes ? Math.round(memoryInfo.maxMemoryBytes / (1024 * 1024)) : '?'; | ||
| breadcrumbMessage = `RAM Check: ${usedMemoryMB ?? '?'}MB / ${maxMB}MB limit`; | ||
| } else if (memoryInfo.platform === 'android') { | ||
| const usagePercent = memoryInfo.usagePercentage?.toFixed(0) ?? '?'; | ||
| breadcrumbMessage = `RAM Check: ${usedMemoryMB ?? '?'}MB used (${usagePercent}% device RAM)`; | ||
| } else { | ||
| breadcrumbMessage = `RAM Check: ${usedMemoryMB ?? '?'}MB used (iOS - no limit API)`; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as https://github.com/Expensify/App/pull/81344/changes#r2774970987
|
@szymonzalarski98 I just left two comments |
|
@truph01 On it, thanks |
|
@truph01 PR updated |
|
Thanks @szymonzalarski98 Does this PR need QA test steps so that they can test it on Sentry? If so, could you update the PR's author checklist for it? |
I think QA tests are not needed, I've updated the PR description |
|
🚧 @mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@szymonzalarski98 @truph01 This was reverted due to the fire it caused last week, did we make sure to test that the same issues are not present? |
I haven't noticed any problems with being logged out automatically or with the theme flickering. Recently, the problem was most likely caused by the use of the Log method by onyx initialization, but now we don't use logging. |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.3.16-0 🚀
|
Explanation of Change
Fixed Issues
$ #79660
PROPOSAL:
Tests
Offline tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari