-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Issue] Excessive API calls during reload/restore captures #3137
Comments
➤ Sam commented: Excessive API calls are caused by initial implementation of ✓ feat: add icon to indicate capture with caption ( https://app.asana.com/0/1201016280880500/1202071964488862/f ). I think it's time to re-implement this feature. Original implementation is calling backend API every time we check for caption as mentioned here ( #1506 (comment) ). |
➤ Sam commented: James Chien, I have a question regarding adding new field to Proof object. Before we added a feature to show caption icon if asset has a caption as explained in previous comment ( https://app.asana.com/0/0/1206137436792270/1206168447347703/f ). Basically previous implementation issue is that there is 1 API call ( https://github.com/numbersprotocol/capture-lite/blob/master/src/app/features/home/capture-tab/capture-item/capture-item.component.ts#L74 ) is done to check wether it has a caption or not. We do API call because Poof object does not have "caption" field. Proposal #1 is to add caption field to Proof similar to diaBackendAssetId (
capture-lite/src/app/shared/dia-backend/asset/dia-backend-asset-repository.service.ts Line 357 in 6fb9378
Also want to clarify what was the intention of ProofMetadata.caption ( https://github.com/numbersprotocol/capture-lite/blob/master/src/app/shared/repositories/proof/proof.ts#L445 )? Because it is used to generate integrityHash ( https://github.com/numbersprotocol/capture-lite/blob/master/src/app/utils/nit/nit.ts#L7 ). Caption is changeable and why we use it in generating integrity hash? |
➤ James Chien commented: Sam Adding the caption to Proof sounds good to me. Proof is a legacy stuff and anything that makes it more synced with diaBackendAsset is good. For the caption in ProofMetadata, I checked the discussion before #779 ( #779 ) but didn’t see anything mentioning the caption and I can’t remember why we decide to have caption field in it. |
➤ Sam commented: Here is the working demo with miro (in claap comment) https://app.claap.io/numbers-protocol/fix-issue-excessive-api-calls-during-reload-c-O35CsUM4Uy-seXkIeUL9blw ( https://app.claap.io/numbers-protocol/fix-issue-excessive-api-calls-during-reload-c-O35CsUM4Uy-seXkIeUL9blw ) |
➤ Kenny Hung commented: Sam How does QA test this task? |
➤ Sam commented: Kenny Hung, there is no way easy way QA can test it without inspecting the network request (I use network tools as shown in claap ( https://app.asana.com/0/0/1206137436792270/1206201706613742/f )). Maybe you can do pull to refresh and ask backend how many request did the user at the time of pull to refresh. Previous version does lot's of request when doing pull to refresh. So I recommend
If step 9 has less (lot less) request then step 4 it can be considered as passed. |
➤ Sam commented: Kenny Hung if you need any assistance or have any other question please let me know. |
➤ Kenny Hung commented: Sam But the behavior is different with before version, in 0.88.x, when user pull to refresh it just show the pop-up like restore all asset, not refresh all asset. |
➤ Sam commented: Kenny Hung, correct. However under the hood I still use restore method. As mentioned before ( https://app.asana.com/0/0/1205627319931691/1206156807473475/f ) no mater is it restore or pull to refresh it will use restore method to make sure everything is synced locally. ✓ [Issue] Excessive API calls during reload/restore captures ( https://app.asana.com/0/0/1206137436792270 ) is more about using local copy of caption (proof.caption instead of fetching backend asset caption every time it need) I think it will be hard to test it without inspecting network request. Let me check if there are any UI friendly tools to check network request of APK. |
➤ Sam commented: Kenny Hung, I will try Charles Web Debugging Proxy • HTTP Monitor / HTTP Proxy / HTTPS & SSL Proxy / Reverse Proxy (charlesproxy.com) ( https://www.charlesproxy.com/ ) myself first if this can be used to test this issue I will share instruction on how to install/setup/test using this tool. Let me try first. |
➤ Sam commented: Kenny Hung, I think it take too much configuration. I think it's easier to test during code review if acceptable. During code review James Chien can pull changes locally and run web version of capture cam and verify in network inspector that it does less network calls. Kenny Hung, James Chien let me know if this option works? Otherwise these are instruction. Steps 3, 4, 5 are not user friendly. However if needed we can do that. Charles is a web debugging proxy tool that allows you to monitor and inspect network traffic between your computer and the internet. To use Charles to inspect network requests from a connected Android device, follow these steps:
Keep in mind that using Charles to inspect network traffic may involve handling sensitive information, and it's important to use this tool responsibly and ethically. Additionally, some apps may have security measures in place to prevent their traffic from being intercepted and inspected. I really think testing in code review is much better then doing it by QA. Also Charles ( https://www.charlesproxy.com/ ) networking tool is not as clear/friendly as Chrome networking tool. |
➤ James Chien commented: Kenny HungSam Here's another way to inspect API requests.
Note: please be careful not to edit existing dashboards in dashboard tab |
➤ Sam commented: James Chien, we had huddle with Kenny Hung, and I livedemo of the fix using chrome web inspector I belive Kenny Hung later will upload screenrecording to claap |
➤ James Chien commented: Great, anyway if there's any future testing needs you can use the log explorer. |
➤ Kenny Hung commented: live demo [Issue] Excessive API calls during reload/restore captures in Sprint 23.12.18 | Claap ( https://app.claap.io/numbers-protocol/issue-excessive-api-calls-during-reload-restore-captures-c-O35CsUM4Uy-ABZSjaXq23J3 ) (cc SamJames Chien) |
➤ Kenny Hung commented: James ChienSam (cc Scott Yan) QA check from backend dashboard, and the log of amounts has indeed decreased. QA pass. |
➤ Sherry Chung commented: Kenny HungScott Yan This is a great method to let QA know how many API calls per user action. Please keep a mindset that for any new feature we want to still keep the performance or make it even quicker. Please also include the method into the testing step if needed. |
Description
A clear and concise description of what the bug is.
Steps to Reproduce
Expected Behavior
N - means number of total assets that user has registered.
100 - means limit of total asset fetched per request
If user has 207 assets it should do 3 API calls in total.
Actual Behavior
Logs
For screen recording you can check this claap.
┆Issue is synchronized with this Asana task by Unito
┆Created By: Sam
The text was updated successfully, but these errors were encountered: