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

Performance issue: Move GoogleAdsFailures::init to error_handler middleware #2099

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

jorgemd24
Copy link
Contributor

@jorgemd24 jorgemd24 commented Sep 18, 2023

Changes proposed in this Pull Request:

Closes part of #1250

The GoogleAdsClient:init is an expensive operation that is currently loaded whenever the Google Ads client is created. It gets triggered by the GoogleServiceProvider and is executed each time there's an admin page load or a request to wp-json.

See the full details of this issue: #1250 (comment)

In this PR, we relocate GoogleAdsFailures::init from the GoogleAdsClient constructor to the error_handler middleware. This change means that GoogleAdsFailure will only be loaded when there is a request made to the google-ads API.

Before this PR:

image

After this PR:

image

Shifting GoogleAdsFailure:init to the error_handler will help in cutting down the time allocated for the Google protobufs functions. However, the GL&A container is still consuming roughly 4.88% of the total loading process, therefore still some room for improvement.

Screenshots:

Detailed test instructions:

General Testing

  1. Connect your Ads account and test that you can create, edit, and delete ads campaigns.

Simulate an Ads API error

  1. Change this line:
    $campaign_fields['status'] = CampaignStatus::number( $params['status'] );

To: $campaign_fields['status'] = 999999

  1. Go to the dashboard page, and change the status of one of your campaigns.
  2. You should see the following error:

image

Simulate a partial failure error.

  1. Keep the change that you made in the previous testing (campaign_status = 99999).
  2. Change this:

$responses = $this->client->getGoogleAdsServiceClient()->mutate(
$this->options->get_ads_id(),
$operations
);

With:

		$responses = $this->client->getGoogleAdsServiceClient()->mutate(
			$this->options->get_ads_id(),
			$operations,
			[
				'partialFailure' => true,
			]
		);
  1. You should see the following error:

image

It shows an invalid campaign ID, because the current code is not expecting partial failures.

  1. If you comment out GoogleAdsFailures::init(); and try to change the campaign status, you will get the following error:

image

Additional details:

Changelog entry

Fix - Performance issue with GoogleAdsFailures::init

@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Sep 18, 2023
@jorgemd24 jorgemd24 marked this pull request as ready for review September 18, 2023 15:06
@jorgemd24 jorgemd24 self-assigned this Sep 18, 2023
@jorgemd24 jorgemd24 requested a review from a team September 18, 2023 15:06
Copy link
Contributor

@mikkamp mikkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. I've tested failures and partial failures and I get the expected response regardless of when it happens.

Just wanted to note that the before and after screenshots in the PR are identical, but I've seen the ones in #1250 (comment).

It's a shame that we can't initialize only on an error response, but it's still a great improvement to only initialize when there has actually been a request to the Google Ads API.


// Partial Failures come back with a status code of 200, so it's necessary to call GoogleAdsFailures:init every time.
if ( strpos( $path, 'google-ads' ) !== false ) {
GoogleAdsFailures::init();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering what happens if we ever send two requests (one after the other). However it seems that the init function only initializes a new instance if it can't find a previously initialized one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's correct.

@jorgemd24
Copy link
Contributor Author

Thanks @mikkamp for reviewing this PR.

Just wanted to note that the before and after screenshots in the PR are identical, but I've seen the ones in #1250 (comment).

I updated the description with the right images.

Thanks again!

@jorgemd24 jorgemd24 merged commit 31f33cb into develop Sep 20, 2023
9 checks passed
@jorgemd24 jorgemd24 deleted the fix/1250-google-ads-failure-init branch September 20, 2023 09:04
@jorgemd24 jorgemd24 mentioned this pull request Sep 20, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants