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

Enhance ads measurement status connection checks #10190

Open
aaemnnosttv opened this issue Feb 6, 2025 · 3 comments
Open

Enhance ads measurement status connection checks #10190

aaemnnosttv opened this issue Feb 6, 2025 · 3 comments
Labels
Module: Ads Google Ads module related issues Module: Analytics Google Analytics module related issues Module: Tag Manager Google Tag Manager module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Feb 6, 2025

Feature Description

The core/site endpoint ads-measurement-status is a cross-module endpoint for the detection of Google Ads measurement which can be implemented a number of ways. Its response is surfaced via the isAdsConnected selector.

This endpoint supports Consent Mode – a non-module/service specific feature of the core tagging platform.


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

Acceptance criteria

  • The core handler for the endpoint should not rely on specific modules, but instead offer a hook that active modules can integrate with to inform the response
  • The response of the endpoint should be cached when requested on the SK dashboard but bypassed in a settings context

Implementation Brief

Updates to core/site/data/ads-measurement-status

  • Replace the body of the handler with a new filter googlesitekit_ads_measurement_connection_checks with a default value of an empty array
    • Iterate over the resulting array, which should be callable[]
    • Call each function in order and stop as soon as ads connection is detected (could be boolean or perhaps AW- tag string) – it should only be possible to flag ads connection via a callback, not negate which is why the filter expects callables rather than filtering a boolean as this would require each integration to implement the same condition to only run its logic if the given value is false
    • Return the response { connected: Boolean } as today using the value from the filter
  • Move module-specific logic into respective modules (Ads, GA4, GTM)
    • Maintain the same order of checks as today using filter callback priorities to add a callback earlier or later in the list
  • Refactor requests to use Module->get_data() instead of API client services directly (e.g. use GET:live-container-version instead of get_tagmanager_service()->accounts_containers_versions->live

Updates to isAdsConnected on client

  • Update ads-measurement-status request in its fetch store to not disable cache unconditionally, but via a "params" argument { useCache } that can be passed to the underlying api.get call
  • Create a new selector for isAdsConnectedUncached which does the same as isAdsConnected but calls ads-measurement-status fetch store with useCache: false from its resolver
  • Update components used outside of the dashboard to use the new isAdsConnectedUncached selector

Test Coverage

  • Add tests for the new selector
  • Update PHP tests for the changes to the REST controller + add tests to respective module test cases to cover the module-specific behavior

QA Brief

Changelog entry

@aaemnnosttv aaemnnosttv added Module: Ads Google Ads module related issues Module: Analytics Google Analytics module related issues Module: Tag Manager Google Tag Manager module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Feb 6, 2025
@tofumatt tofumatt self-assigned this Feb 13, 2025
@tofumatt
Copy link
Collaborator

Create a new selector for isAdsConnectedUncached which does the same as isAdsConnected but calls ads-measurement-status fetch store with useCache: true from its resolver

Shouldn't this be useCache: false, based on the name? 🤔

Why do we need a cached version of this selector? The API endpoint isn't especially slow/expensive, right? And we probably want the most up-to-date response when making the query, right?

@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Feb 13, 2025
@aaemnnosttv
Copy link
Collaborator Author

Shouldn't this be useCache: false, based on the name? 🤔

Oops! Yes, of course, I've updated that now, thanks!

Why do we need a cached version of this selector? The API endpoint isn't especially slow/expensive, right? And we probably want the most up-to-date response when making the query, right?

You're correct that we haven't needed to do this kind of thing before. That's mostly because endpoints are usually only needed for setup/settings or reporting. During setup/configuration, we always need the latest data, and since this endpoint was originally implemented for that purpose, that was correct. However, it is now being used within the requirements checks for one or more notifications, so it is being called on every view of the dashboard. Anything that's called from the dashboard should be cached, and shouldn't need the most up to date data. It's not about the speed but how many times we're calling it.

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Feb 13, 2025
@tofumatt
Copy link
Collaborator

Cool, makes sense, thanks!

IB ✅

@tofumatt tofumatt removed their assignment Feb 13, 2025
@ivonac4 ivonac4 added the Team M Issues for Squad 2 label Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Ads Google Ads module related issues Module: Analytics Google Analytics module related issues Module: Tag Manager Google Tag Manager module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants