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

Setting a lot of data in Onyx is slow #21766

Closed
iwiznia opened this issue Jun 27, 2023 · 29 comments
Closed

Setting a lot of data in Onyx is slow #21766

iwiznia opened this issue Jun 27, 2023 · 29 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@iwiznia
Copy link
Contributor

iwiznia commented Jun 27, 2023

Coming from #21444

Threads with context:
https://expensify.slack.com/archives/C03TQ48KC/p1687893191020179
https://expensify.slack.com/archives/C036S0BVAHH/p1687812903398059

Problem

Setting a lot of data in Onyx is super slow.

This started/finished is calls to onyx. You can see it takes 20 seconds to set the key 19 and 1 minute 8 seconds the second one.

19:58:45.761 web.development.js:1304 started 19 () => mergeCollection(key, value)
19:58:45.762 web.development.js:1304 started 20 () => mergeCollection(key, value)
19:59:06.247 web.development.js:1305 finished 19 () => mergeCollection(key, value)
19:59:53.190 web.development.js:1305 finished 20 () => mergeCollection(key, value)

I have a file with the data that's failing, but I think it might contain sensitive information but I can say it's 30MB uncompressed. The 20th call above is the biggest as one might expect.

In order to test this in dev (for internal engineers only) is to change the OpenApp code to (data.json is the file with all the data):

    } elseif ($command === 'OpenApp') {
        $response = json_decode(file_get_contents('data.json'), true);
//        $response['onyxData'] = AppInit::openApp($authToken, $_REQUEST['policyIDList'] ?? '');

I assume it can also be reproduced by joining 1000s of policies. This user's data:

  • 3216 policies
  • 7k personal details (x 2)
  • 9k reports

Solution

Either make onyx be faster or recognize we need to do pagination of policies and reports

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a864f2f0848526ae
  • Upwork Job ID: 1676300792032952320
  • Last Price Increase: 2023-07-04
@iwiznia iwiznia added the Bug Something is broken. Auto assigns a BugZero manager. label Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@iwiznia
Copy link
Contributor Author

iwiznia commented Jun 28, 2023

Updated the data above, just realized that we have 2x the number of personal details that we need to due to the migration to accountIDs, so if we remove that, this will get a bit better, but not nearly enough since most of the time is on the reports.

@melvin-bot melvin-bot bot added the Overdue label Jun 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

@michaelhaxhiu Eep! 4 days overdue now. Issues have feelings too...

@michaelhaxhiu
Copy link
Contributor

@iwiznia what should i do with this one? Does it go to Engineering?

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2023
@iwiznia
Copy link
Contributor Author

iwiznia commented Jul 4, 2023

For the pagination stuff, yes.
For the making onyx faster it can go to contributors, in fact I thought there were some initiatives that could help with it (like sqlite backend for onyx), but not sure what state they are in.

@michaelhaxhiu
Copy link
Contributor

Sorry I just mean what did you hope to see for this particular GH that you made? Do you want it to go to external or internal for now?

@iwiznia iwiznia added Improvement Item broken or needs improvement. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 4, 2023
@iwiznia
Copy link
Contributor Author

iwiznia commented Jul 4, 2023

Let's use this issue for ht pagination

@iwiznia iwiznia added the Internal Requires API changes or must be handled by Expensify staff label Jul 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01a864f2f0848526ae

@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @eVoloshchak (Internal)

@melvin-bot melvin-bot bot added the Overdue label Jul 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 7, 2023

@eVoloshchak, @michaelhaxhiu Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 2023

@eVoloshchak, @michaelhaxhiu 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@michaelhaxhiu
Copy link
Contributor

Slack convo going on here https://expensify.slack.com/archives/C01GTK53T8Q/p1689036681307159 that was started yesterday

@marcaaron
Copy link
Contributor

@AndrewGable Need to move my focus elsewhere and the Guides seem unblocked for now, but here's a summary of my attempts today...

  • This PR introduced something called removeItems() which replaced the call to Storage.clear()
  • The problem with removeItems() is that it just doesn't work for large accounts when they are signing out. Tons of data is left behind.
  • I tried to swap out the method, but it was not as straightforward as I thought and a bunch of tests broke.
  • I am moving on to other things for now.

Ideas for what to do next...

  • Pass this to a contributor to fix
  • Get the contributor who introduced the change to roll it back
  • Get an "expert" contributor to figure out why removeItems is not working

@melvin-bot
Copy link

melvin-bot bot commented Jul 21, 2023

@AndrewGable, @michaelhaxhiu Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2023

@AndrewGable, @michaelhaxhiu 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@melvin-bot
Copy link

melvin-bot bot commented Jul 27, 2023

@AndrewGable, @michaelhaxhiu 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

@AndrewGable, @michaelhaxhiu 12 days overdue. Walking. Toward. The. Light...

@melvin-bot melvin-bot bot removed the Daily KSv2 label Aug 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2023

This issue has not been updated in over 14 days. @AndrewGable, @michaelhaxhiu eroding to Weekly issue.

@melvin-bot melvin-bot bot added the Weekly KSv2 label Aug 3, 2023
@michaelhaxhiu michaelhaxhiu removed their assignment Aug 4, 2023
@michaelhaxhiu
Copy link
Contributor

Note: I'm preparing to go OOO for 2 weeks and removing my assignment. This is an Improvement and seems to be moving forward (e.g. I notice @kidroca making moves in Expensify/react-native-onyx#281), but if it needs a BZ assigned can you please apply the Bug or New Feature label to ensure one is assigned?

cc @AndrewGable

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Aug 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

This issue has not been updated in over 15 days. @AndrewGable 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 Aug 28, 2023
@maddylewis maddylewis added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Oct 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot removed the Monthly KSv2 label Oct 7, 2023
@maddylewis
Copy link
Contributor

added Bug label based on this comment / it wasn't ever added - #21766 (comment)

i don't totally understand what Onyx is or how it's affecting our users but I have a couple of SR escalated issues having to do with admins not being able to use newdot/chat at all. so wanted to get some action on this issue again just in case that's related.

@marcaaron
Copy link
Contributor

i don't totally understand what Onyx is or how it's affecting our users

attempted to answer this here

@marcaaron
Copy link
Contributor

I have a couple of SR escalated issues having to do with admins not being able to use newdot/chat at all

Whatever these issues are we should escalate and get more 👀 on them. My guess would be that it's not related to this issue.

@maddylewis
Copy link
Contributor

maddylewis commented Oct 16, 2023

@marcaaron - here are the two issues I was referencing:

admittedly, im not sure of the status of the second issue. but, it looks like the first one i linked is still outstanding.

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

@AndrewGable, @kevinksullivan Huh... This is 4 days overdue. Who can take care of this?

@AndrewGable
Copy link
Contributor

@muttmuure and I discussed this issue in Slack, since it's somewhat a catchall issue with no clear solution, we think we should tackle this issue with our new format of finding specific issues via audits with our external contributors. Please reopen if you disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests

7 participants