Syncing audiences and custom dimensions in parallel can result in the cached audiences not being persisted

[techanvil]

Bug Description

When making parallel requests to sync both audiences and custom dimensions (POST:sync-audiences and POST:sync-custom-dimensions), it can result in the cached audiences not being persisted. It seems the call to sync-audiences saves the audiences, but the parallel call to sync-custom-dimensions then deletes them.

Steps to reproduce

Here is a straightforward way to exhibit the bug. Please note this was discovered during development of #8160 and so these instructions will only work once that issue's PR is merged (or by checking out the branch for the PR, see https://github.com/google/site-kit-wp/pull/8885).

1. Set up Site Kit with the audienceSegmentation feature flag enabled, and connect an Analytics property which has some data.
2. Set up Key Metrics, selecting a metric which depends on a custom dimension.
3. Click Enable groups on the Audience Segmentation Setup CTA Banner to set up the feature.
4. Once the dashboard has loaded, use the WP CLI to delete the availableAudiences and availableCustomDimensions settings.

wp option patch delete googlesitekit_analytics-4_settings availableAudiences
wp option patch delete googlesitekit_analytics-4_settings availableCustomDimensions

5. Reload the dashboard. You should see requests in the network tab being made to the POST:sync-audiences and POST:sync-custom-dimensions endpoints. If the property has got the new/returning visitor audiences set up with data, you may also see a couple of failed requests to the save-resource-data-availability-date endpoint (see screenshot below). Once this bug is fixed these requests should not fail. Note that the bug doesn't occur every time - if you see these calls to save-resource-data-availability-date succeed, try again from point 3.
6. Query the Analytics settings via the WP CLI. You should see availableAudiences is NULL or empty, rather than being an array of audiences as expected. Again, the bug doesn't occur every time so if you do see an array of audiences, try again from point 3.

# Retrieve all settings.
wp option get googlesitekit_analytics-4_settings

# Retrieve the individual `availableAudiences` setting.
wp option pluck googlesitekit_analytics-4_settings availableAudiences

Screenshots

Actual:

Expected:

Additional Context

• PHP Version: any
• OS: any
• Browser: any
• Plugin Version: pre-release 1.130.0
• Device: any

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Parallel requests to POST:sync-audiences and POST:sync-custom-dimensions should not interfere with each other.
• Specifically, cached audiences should not be deleted by a parallel call to sync-custom-dimensions.
• It should be ensured the reverse can't happen too, i.e. the available custom dimensions can't be deleted by a parallel call to sync-audiences with different timing.

Implementation Brief

• [ ]

Test Coverage

QA Brief

Changelog entry

[techanvil]

Moved to Backlog until we get all the P1 issues done and/or the initial release out.

[benbowler]

@techanvil I've spent some time looking into this this week. As suspected in the issue description, the issue comes down to the REST endpoints sync-audiences and sync-custom-dimensions overlapping. Because of the way the Options/Settings classes are defined, the whole wp_options object is loaded before new settings are merged into the existing object and the updated object is re-serialized and saved.

I have experimented in detail on the PHP side to see if there is a way to refetch or flush cache for options as late as possible before merging in the new changes, regardless of these changes it's always possible to get into the situation where new settings are saved then overwritten by a later call to save settings from an outdated base options object.

I can see two routes forward here and would love your perspective on the direction to take:

1. From the PHP side; split the audience settings from the main analytics-4 settings - Creating a separate Settings class with a key such as googlesitekit_analytics-4_settings and googlesitekit_analytics-4_audiences_settings would work because these endpoints don't need to change the same keys, however the structure of settings means all settings are updated each time any setting is changed.

2. A JS approach; add infrastructure around either or both of syncAvailableCustomDimensions and syncAvailableAudiences actions to prevent calls to one control callback, while the other is fetching.

[techanvil]

Hi @benbowler, thanks for looking into this. It is a fairly fundamental issue, and it's one we've certainly run into before with other endpoints, e.g. where multiple requests to update the settings for a module can conflict with each other in a similar way.

It would be great to simply say let's put a lock on the table or a mutex around option updates, but this wouldn't realistically be practical for compatibility and/or performance reasons.

To the case in hand, I think your suggestion of the PHP settings split is the best way forward here. I think it would be simpler to implement and manage, with it still allowing the sync-audiences and sync-custom-dimensions requests to be made in parallel.

I can envisage some complexity were we to take the JS route, as we'd likely still be wanting to make the requests around the same time so we'd probably need to implement a wait/retry mechanism, which as well as adding complexity could impact the UX.

So, I'd go for the server side route, unless you discover a reason that makes the JS approach more attractive.

[benbowler]

This work will require care and attention as the Audiences work is tightly coupled with the Analytics module. In the IB I refactored the minimum logic to split the settings keys to fix the issue as described.

We could go further an create an "Audiences" class that extracts and handles more of the logic, event inheriting Module traits to handle settings etc. However, this is already a fairly large ticket which will require detailed review and testing to ensure it doesn't impact all the features of Audience Segmentation. So we should considered further refactoring in stages if we consider it better for code clarity.

[techanvil]

Hi @benbowler, thanks for drafting this IB. It's off to a good start, but it's missing how to use the refactored settings on the JS side. It looks like we'll need to add a couple of REST endpoints to get/set the settings, and update the client-side code to use them. Note this client-side update to the audienceSegmentationSetupCompletedBy setting:

https://github.com/google/site-kit-wp/blob/390cbd5f9b6242272ef6f1ce792f8f3d64869d6c/assets/js/modules/analytics-4/datastore/audiences.js#L584-L590

Here are a few points on the server-side changes you have outlined:

• Reviewing the current list of settings classes, I think that at this stage, rather than creating a new Analytics_4/Audiences/ folder which only contains Settings.php, we should simply create a new Analytics_4/Audience_Settings.php file. We can consider how to refactor this further if/when we do a more substantial refactoring of the audience code.
  
  Furthermore, although it's preferable to keep additions to the Analytics_4 module class to a minimum, I think we should at this stage simply add a couple more datapoints to the module to get/set the settings as per my initial point. They will be simple endpoints and AFAIAA we don't have an established pattern for spreading module datapoints over multiple classes (note that we don't currently use the REST_.*_Controller pattern in the Modules namespace). Refactoring module datapoints seems like something we should tackle more broadly in a separate issue.
  
  One detail to bear in mind is that we should preload the new audience settings datapoint to ensure we retain the current behaviour (the module settings endpoints are all preloaded so at present the audience settings are available on page load).

# List of settings classes.

$ find includes -name "Settings.php"
includes/Modules/Sign_In_With_Google/Settings.php
includes/Modules/Reader_Revenue_Manager/Settings.php
includes/Modules/AdSense/Settings.php
includes/Modules/Analytics_4/Settings.php
includes/Modules/Search_Console/Settings.php
includes/Modules/PageSpeed_Insights/Settings.php
includes/Modules/Ads/Settings.php
includes/Modules/Tag_Manager/Settings.php

$ find includes -name "*_Settings.php"
includes/Core/Key_Metrics/Key_Metrics_Settings.php
includes/Core/Modules/Module_Sharing_Settings.php
includes/Core/Modules/Module_With_Settings.php
includes/Core/Modules/Module_Settings.php
includes/Core/User/Conversion_Reporting_Settings.php
includes/Core/User/Audience_Settings.php
includes/Core/Tags/First_Party_Mode/First_Party_Mode_Settings.php
includes/Core/Conversion_Tracking/Conversion_Tracking_Settings.php
includes/Core/Consent_Mode/Consent_Mode_Settings.php

• Within the current $this->get_settings()->on_change( callback, refactor checks against the three audience settings keys to an on_change event on the new audience settings store.

• Regarding the above point, it looks like the only relevant checks are those for availableAudiences in this block of code, so we could be a bit more explicit here.

• Update the filter on pre_update_option_googlesitekit_analytics-4_settings to filter pre_update_option_googlesitekit_analytics-4_audiences_settings

• Re. the above point, this won't preserve the logic as expected, as the condition for nulling the audience settings is the property/measurement ID changing, which means we should keep the current hook name but set the audience settings to null within it via the new Audience_Settings class.
• A minor naming/plurality point, let's change googlesitekit_analytics-4_audiences_settings to googlesitekit_analytics-4_audience_settings.

So, there a few additional aspects to consider here. Interested to hear your thoughts.

[benbowler]

Thanks for the notes @techanvil. I've added considerable additional detail above. This is definitely going to be a large ticket that will touch many areas of the module and feature so it will require additional time for test and QA. I can't see a clear way to divide this ticket into smaller issues so I've increased the estimate to account for this.

---

In my update I suggest using new unique named selectors/action for these settings so as to distinguish that these aren't the auto generated selectors/action from a settings store but we could keep the same name if preferred:

• getAudienceSettingsAvailableAudiences
• getAudienceSettingsAudienceSegmentationSetupCompletedBy
• setAudienceSettingsAudienceSegmentationSetupCompletedBy

[techanvil]

Hi @benbowler, thanks for the update. A few points on the revised IB:

• [ ] Update includes/Modules/Analytics_4.php:

It's worth a note in the above section that we should also update this line to retrieve the audience settings from the new class instance:

https://github.com/google/site-kit-wp/blob/c2e25335021b5c4482aa3ee97eb6d5ac3ad417a6/includes/Modules/Analytics_4.php#L259

We also need to update usage of audience settings in get_debug_fields() and set_available_audiences():

https://github.com/google/site-kit-wp/blob/c33c9ba507df8493a789f6086a69589dbf29ebb3/includes/Modules/Analytics_4.php#L582

https://github.com/google/site-kit-wp/blob/c33c9ba507df8493a789f6086a69589dbf29ebb3/includes/Modules/Analytics_4.php#L2467-L2472

• Preload the GET:get-audience-settings path like so.

Interesting, it looks like the above line is a stale remnant from when we had the user-scoped audience settings located in the Analytics 4 module. This was removed in the following PR, but the preload for the endpoint was missed. We should simply be able to keep the same line as it will apply to the new datapoint.

https://github.com/google/site-kit-wp/pull/9143/files#diff-090952b401bbcfab89340ec0e493b5ea942c199f0c73168668f230f1a50a2b82

• GET:get-audience-settings
• POST:set-audience-settings-audience-segmentation-setup-completed-by

We don't usually prefix a GET request with get- in the datapoint name, and we don't tend to use set- in the POST datapoint name. I'd also suggest we provide a general POST audience settings datapoint that would be more future proof than the specific POST:set-audience-settings-audience-segmentation-setup-completed-by datapoint.

Naming wise, I'd suggest the following for consistency:

• GET:audience-settings
• POST:save-audience-settings

We could restrict the POST datapoint to only allow updating the audienceSegmentationSetupCompletedBy setting for now, but it would leave it open for future settings.

• Update references to getAudienceSettings to getUserAudienceSettings and update all current uses of this selector within the app.

We should also update the saveAudienceSettings(), resetAudienceSettings() and isSavingAudienceSettings() actions and selector that operate on the user audience settings to include User in their names.

In my update I suggest using new unique named selectors/action for these settings so as to distinguish that these aren't the auto generated selectors/action from a settings store but we could keep the same name if preferred:

• getAudienceSettingsAvailableAudiences
• getAudienceSettingsAudienceSegmentationSetupCompletedBy
• setAudienceSettingsAudienceSegmentationSetupCompletedBy

I don't think this is necessary, really. We already use hand crafted selectors/actions that are indistinguishable from the auto generated ones in a lot of cases. Not that "we already do that" is necessarily a good reason to do something, but I don't think keeping the same names would be a problem. Also, we do already have the user-scoped audience settings, and the suggested names could be a bit confusing what with there being two sets of audience settings that they could belong to at first glance.

[techanvil]

This is definitely going to be a large ticket that will touch many areas of the module and feature so it will require additional time for test and QA. I can't see a clear way to divide this ticket into smaller issues so I've increased the estimate to account for this.

Hmm, it is a large issue for sure. It would be great to break it down a bit though - I think we could manage it with the following split (including relevant tests in each issue of course).

1. Add the new Audience_Settings class and datapoints.
2. Add the new audience settings store, but don't integrate it into the Analytics store. Write tests just for the store slice.
3. Integrate the new settings, both on the backend and frontend. Update the audience settings store tests to follow the usual integrated pattern.

That would help break it down a bit, the third issue would still be pretty large but would be easier to manage than a single 30 pointer.

WDYT?

[benbowler]

Hey @techanvil, I've gone ahead and created points 1. and 2. as sub-issues ( #10153 and #10154 ) for this ticket and marked them as blockers. As far as I can see, they work as standard issues in GH and ZH with the addition of being tracked here. I expect that those can be worked on then this ticket can remain as the ticket that handles the completion of the work.

I've split and updated the IBs as I believe they should be split, and have assigned them to you in Triage to update the ACs. Feel free to send them back to me for missing elements based on your ACs.

---

I've updated the IBs based on your comments here across this and the new tickets.

[benbowler]

Updated estimates so this is a 15 with two additional 7's.

[techanvil]

Thanks @benbowler! I've reviewed both of those new issues and sent them back your way with a bit of feedback.

As to this issue's IB, it's looking almost ready to roll, with just a couple of points to consider/address:

• The GET / POST datapoints will be defined in #10153, so their definition can be removed from this IB.
• Re. the rename of getAvailableAudiences -> getAudienceSettingsAvailableAudiences, getAudienceSegmentationSetupCompletedBy getAudienceSettingsAudienceSegmentationSetupCompletedBy and setAudienceSegmentationSetupCompletedBy setAudienceSettingsAudienceSegmentationSetupCompletedBy, taking a look at this again I do feel these renamed selectors are starting to get a bit unwieldy and so I'd reiterate on this point I made previously:

I don't think this is necessary, really. We already use hand crafted selectors/actions that are indistinguishable from the auto generated ones in a lot of cases. Not that "we already do that" is necessarily a good reason to do something, but I don't think keeping the same names would be a problem. Also, we do already have the user-scoped audience settings, and the suggested names could be a bit confusing what with there being two sets of audience settings that they could belong to at first glance.

[benbowler]

Thanks @techanvil, I've reviewed this ticket as well as #10153 and #10154 at the same time to make sure they all match.

• [ ] Update assets/js/modules/analytics-4/datastore/audiences.js:
  • Remove the existing definition of getAvailableAudiences as this will be replaced by the new definition of this selector in assets/js/modules/analytics-4/datastore/audience-settings.js created in https://github.com/google/site-kit-wp/issues/10154.

I added this detail to this ticket, this is to make sure that in #10154 the existing selectors are not touched so that existing functionality can be maintained until the selectors and actions are replaced with those from the new assets/js/modules/analytics-4/datastore/audience-settings.js file in this final ticket.

[techanvil]

Thanks @benbowler!

IB ✅

[ankitrox]

@hussain-t As discussed, I will assign this ticket to myself while you look into another one. This ticket was dependent on 2 sub-issue, of which one is approved and other is merged and in QA, so this one can be started.

CC: @ivonac4

[ankitrox]

Marked this as blocked by #10148 because this issue leads to lot of test failures because of feature flag persistence.

[hussain-t]

QA Verified ✅

• Verified that after setting up Key Metrics and enabling Audience Segmentation, the availableAudiences and availableCustomDimensions options were deleted using WP CLI prior to reloading the dashboard.
• Verified both POST:sync-audiences and POST:sync-custom-dimensions requests completed successfully.
• Verified there were no API request failures.
• Confirmed via WP CLI that availableAudiences was correctly repopulated after sync:

wp option pluck googlesitekit_analytics-4_audience_settings availableAudiences

• Performed a smoke test across Audience Segmentation and Key Metrics features—everything worked as expected with no regressions.

[hussain-t]

While working on #10089, we identified a few regressions introduced by changes made as part of this ticket. We’ve moved this ticket back to execution to allow for thorough testing and resolution of the issues. Please refer to this comment and the related Slack discussion for more context.

cc: @wpdarren @ankitrox

[ankitrox]

@hussain-t Thank you for looking into this and adding the comment describing the problem.

It seems that the problem is happening here because availableAudiences is undefined .

We already have the PR 10568 for the issue 10449 which fixes this issue. I've tested this with the current PR and it seems that things are working fine with this change as can be seen in this video:

https://www.loom.com/share/263a029e65264cc398c5159ee2aa180a?sid=b60ec832-6cf8-48a8-8baa-d2f608198472

Please let me know if we can review and merge 10449 to fix the problem instead of patching it in #8888 because 10449 also requires smoke testing around AS feature which is similar QA requirement as that of 8888.

CC: @nfmohit

[nfmohit]

@ankitrox has very kindly created a follow-up PR for the issue raised by @hussain-t here. The PR has now been merged.

I realise this issue does not have a general QA Brief (it only has QA:Eng), which IMO it deserves, as it is a significant change to how audience configuration is stored and fetched. I've added a short QA brief as a result.

Thank you!

[wpdarren]

QA Update: ✅

Verified:

• I have completed some smoke testing to ensure the issues previously reported are fixed. I also created new audiences, renamed audiences, and added descriptions; everything was synced. I will continue to do more thorough testing during release testing, but since we're already considerably delayed, I will move this to approval.