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

[Issue] Excessive API calls during reload/restore captures #3137

Closed
sync-by-unito bot opened this issue Dec 11, 2023 · 17 comments
Closed

[Issue] Excessive API calls during reload/restore captures #3137

sync-by-unito bot opened this issue Dec 11, 2023 · 17 comments

Comments

@sync-by-unito
Copy link

sync-by-unito bot commented Dec 11, 2023

Description

A clear and concise description of what the bug is.

bugs found

Steps to Reproduce

  1. Go to verified captures
  2. Pull to refresh

Expected Behavior

  • Expected: To do (N / 100 + N % 100) API calls.

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

  • Actual: It's doing 10x more API calls then expected. For example when user with 16 assets pulls to refresh the assets it does 178 API calls.

Logs

For screen recording you can check this claap.

┆Issue is synchronized with this Asana task by Unito
┆Created By: Sam

Copy link
Author

sync-by-unito bot commented Dec 14, 2023

➤ 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) ).

Copy link
Author

sync-by-unito bot commented Dec 19, 2023

➤ 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 (

diaBackendAssetId?: string = undefined;
). And every time caption is updated copy DiaBackendAsset.caption ( ) to Proof.caption.

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?

Copy link
Author

sync-by-unito bot commented Dec 19, 2023

➤ 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.

@sync-by-unito sync-by-unito bot closed this as completed Dec 20, 2023
Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ Kenny Hung commented:

Sam How does QA test this task?

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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

  1. download public version from App/Play store
  2. log in to account that has fewer assets (10-15-30 assets) faster to test.
  3. after login do not restore
  4. pull to refresh
  5. ask backend how many request did that user at given time
  6. download v231207-capture-cam-ionic-(0.88.3)(firebase release)
  7. log in to account that has fewer assets (10-15-30 assets)
  8. after login do not restore assets
  9. pull to refresh
  10. ask backend how many request did that user at given time

If step 9 has less (lot less) request then step 4 it can be considered as passed.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ Sam commented:

Kenny Hung if you need any assistance or have any other question please let me know.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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:

  1. Download and Install Charles:
    * Make sure you have Charles installed on your computer. You can download it from the official website: Charles Proxy ( https://www.charlesproxy.com/ ).

  2. Configure Charles Proxy:
    * Open Charles on your computer.
    * In the menu, go to "Proxy" > "Proxy Settings" and note the IP address and port number listed under the "Proxy Information" section.

  3. Configure Android Device:
    * Connect your Android device to the same network as your computer.
    * On your Android device, go to "Settings" > "Wi-Fi," and long-press on the connected Wi-Fi network.
    * Select "Modify Network" or "Advanced" depending on your device.
    * Choose "Manual" for the Proxy settings and enter the IP address and port number from Charles.

  4. Install Charles SSL Certificate on Android:
    * In Charles, go to "Help" > "SSL Proxying" > "Install Charles Root Certificate" and follow the instructions for your operating system.
    * On your Android device, open a web browser and navigate to http://charlesproxy.com/getssl ( http://charlesproxy.com/getssl ) to download and install the Charles SSL certificate.

  5. Enable SSL Proxying:
    * In Charles, go to "Proxy" > "SSL Proxying Settings" and make sure the "Enable SSL Proxying" option is checked.

  6. Start Capturing Traffic:
    * In Charles, click the "Record" button to start capturing network traffic.
    * Perform the actions on your Android device that you want to inspect.

  7. Inspect Network Traffic:
    * In Charles, you should see the captured network traffic in the "Session" tab. You can expand entries to view details of each request and response.

  8. Decrypt SSL Traffic (Optional):
    * If you want to inspect encrypted HTTPS traffic, you may need to enable SSL proxying and install the SSL certificate on your Android device.

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.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ James Chien commented:

Kenny HungSam

Here's another way to inspect API requests.

  1. Go to https://log.dia.numbersprotocol.io/explore ( https://log.dia.numbersprotocol.io/explore )

  2. Login (I'll DM account/password via Signal) Kenny Hung please add my Signal +886931332443

  3. Go to "Explorer" tab

  4. Paste {job="api_prod"} | json | user_email = "userA@nbs.io", replace the user email with the test account's email.
    1. If on QA site, please change job="api_prod" to job="api_qa"

  5. On top panel options, choose the time period (default to last 1 hour)

  6. Click "Run query"

  7. It will show all API requests made by the account in the time period.

截圖 2023-12-22 下午6.15.42.png

Note: please be careful not to edit existing dashboards in dashboard tab

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ James Chien commented:

Great, anyway if there's any future testing needs you can use the log explorer.

Copy link
Author

sync-by-unito bot commented Dec 22, 2023

➤ 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)

Copy link
Author

sync-by-unito bot commented Dec 25, 2023

➤ Kenny Hung commented:

James ChienSam (cc Scott Yan)

QA check from backend dashboard, and the log of amounts has indeed decreased. QA pass.

Copy link
Author

sync-by-unito bot commented Dec 25, 2023

➤ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

0 participants