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

Investigate slow requests to ads/accounts #2674

Open
2 tasks
Tracked by #2509
joemcgill opened this issue Nov 14, 2024 · 2 comments
Open
2 tasks
Tracked by #2509

Investigate slow requests to ads/accounts #2674

joemcgill opened this issue Nov 14, 2024 · 2 comments
Assignees

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Nov 14, 2024

During a review of #2653, @mikkamp made the following observation about slow requests to ads/accounts:

I noticed that loading the account information step is rather slow (in particular for me because I have a large amount of linked test accounts). On one of the requests to ads/accounts the request timed out. This timed out request is not visible but it leaves the user in a state where they can not reconnect to a new ads account.

It might not be vital to address since in this scenario I did already have an ads account connected so I could still proceed with the connected account, also a refresh cleared it up and the call didn't timeout the second time. However I just wanted to mention the behaviour.

Acceptance Criteria

  • These requests are not causing the UI to load slowly when there are many linked accounts (if possible).
  • Add error handling for when the request times out.
@joemcgill
Copy link
Collaborator Author

After a bit of investigation, I'm not convinced that the current implementation is considerably worse than the previous in terms of handling slow responses to get existing accounts. Currently, the GoogleAdsAccountCard will show a spinner until the API response resolves. If the response errors, the spinner persists:

Image

Now a spinner is shown on the combo card until the existing accounts can be resolved. If the ads accounts error during resolution then only the MC connection card is displayed since the existing Ads accounts cannot be displayed:

Image

In both cases, if there is an error, a toast error message is shown that describes the problem that has occurred.

Just to be sure, I've investigated how the API requests are triggered to make sure we're not delaying the requests in some way (e.g., requesting existing MC accounts and Ads accounts in parallel, etc.) and couldn't find any clear opportunities for improvement.

@mikkamp
Copy link
Contributor

mikkamp commented Nov 18, 2024

That's a great point, that there were circumstances where a failing call would continue to show a spinner.

One of the differences I was also trying to highlight was that if all the accounts are connected, then we used to just show everything as connected, without fetching all account details:

Image

Only if we decided to connect another account would it fetch all accounts so a different one could be connected.

But now even if we are connected it still makes the request to fetch all the accounts, both from MC and Ads accounts. So the connected screen is slower to load and I have to wait for it to complete:
Image

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

2 participants