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

[HOLD for payment 2024-10-11] Reduce the number of redundant ReconnectApp calls or clean up the logic that enqueues them #47742

Open
6 tasks
muttmuure opened this issue Aug 20, 2024 · 11 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Engineering Weekly KSv2

Comments

@muttmuure
Copy link
Contributor

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.

@gedu has noticed that app calls ReconnectApp a lot, often redundantly

Here: https://github.com/Expensify/App/blob/main/src/libs/PusherUtils.ts
Thread: https://expensify.slack.com/archives/C05LX9D6E07/p1724159545236079?thread_ts=1721152209.600379&cid=C05LX9D6E07

Here: https://github.com/Expensify/App/blob/main/src/libs/Middleware/RecheckConnection.ts
Thread: https://expensify.slack.com/archives/C05LX9D6E07/p1724076205528369?thread_ts=1721152209.600379&cid=C05LX9D6E07

Here: https://expensify.slack.com/archives/C05LX9D6E07/p1723806097226589?thread_ts=1721152209.600379&cid=C05LX9D6E07

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.

If a user's request queue gets stuck (like it does here) then ReconnectApp can cause the request queue to climb with unprocessed requests

It has been suggested as the root cause with issues with partially loaded LHNs, but also broken queues of requests

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.

https://expensify.slack.com/archives/C05LX9D6E07/p1723566890894989?thread_ts=1721152209.600379&cid=C05LX9D6E07

@danielrvidal's queue of requests climbed to 630 here

Proposed solution (if any)

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

Reduce the number of queued ReconnectApp calls or clean up the logic that queues them so they don't climb so high

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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

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: @danielrvidal
Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1723566890894989?thread_ts=1721152209.600379&cid=C05LX9D6E07

View all open jobs on Upwork

@muttmuure muttmuure added Engineering Daily KSv2 AutoAssignerNewDotQuality Used to assign quality issues to engineers labels Aug 20, 2024
Copy link

melvin-bot bot commented Aug 20, 2024

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

Copy link

melvin-bot bot commented Aug 20, 2024

Triggered auto assignment to @marcochavezf (AutoAssignerNewDotQuality)

@melvin-bot melvin-bot bot added the Weekly KSv2 label Aug 20, 2024
@muttmuure
Copy link
Contributor Author

@gedu I think you're working on this one?

@gedu
Copy link
Contributor

gedu commented Aug 21, 2024

Hey, yes, I'm working on this one

@gedu
Copy link
Contributor

gedu commented Aug 21, 2024

Sent a first "proposal" for discussion: on slack

Copy link

melvin-bot bot commented Sep 16, 2024

This issue has not been updated in over 15 days. @gedu, @marcochavezf eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Sep 16, 2024
@muttmuure
Copy link
Contributor Author

Bugs that have this root cause are being spread across different threads -

https://expensify.slack.com/archives/C05LX9D6E07/p1723132613137659?thread_ts=1722970682.900369&cid=C05LX9D6E07

https://expensify.slack.com/archives/C05LX9D6E07/p1721152209600379

So we're going to consolidate in this issue, because it describes the problem the best

@iwiznia
Copy link
Contributor

iwiznia commented Sep 26, 2024

BTW I see this also happening with OpenApp: https://github.com/Expensify/Expensify/issues/431330 maybe the issues are related

@marcochavezf marcochavezf added Weekly KSv2 and removed Monthly KSv2 labels Oct 1, 2024
Copy link

melvin-bot bot commented Oct 3, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 4, 2024
@melvin-bot melvin-bot bot changed the title Reduce the number of redundant ReconnectApp calls or clean up the logic that enqueues them [HOLD for payment 2024-10-11] Reduce the number of redundant ReconnectApp calls or clean up the logic that enqueues them Oct 4, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 4, 2024
Copy link

melvin-bot bot commented Oct 4, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Oct 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.44-12 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-11. 🎊

For reference, here are some details about the assignees on this issue:

  • @gedu does not require payment (Contractor)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Engineering Weekly KSv2
Projects
Development

No branches or pull requests

4 participants