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

Implement selector to look up GA4 account, property, and web data stream based on list of measurement IDs #6372

Closed
felixarntz opened this issue Jan 4, 2023 · 7 comments
Labels
Exp: SP Module: Analytics Google Analytics module related issues P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Jan 4, 2023

This selector is needed to identify the data to recommend reconfiguring the GA4 module with following an external change to the Google tag mapping (see #6083).


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

Acceptance criteria

  • A new selector getAnalyticsConfigByMeasurementIDs( measurementIDs ) should be implemented.
    • The measurement IDs can be expected to be valid GA4 measurement IDs ("G-...").
    • There is unfortunately no convenient API endpoint to do so. See GA4 setup flow, we probably need to rely on the getWebDataStreamsBatch selector passing all the GA4 property IDs that the current user has access to (via getAccountSummaries selector).
    • Maybe for efficiency it could be useful to only check property IDs for the currently set GA4 account first, as it's arguably more likely the property is from the same account.
    • The selector should return an object with the relevant values for the following GA4 settings (or null if no results were found, e.g. when the user does not have access to any of the web data streams referenced by the measurement IDs):
      • accountID
      • propertyID
      • webDataStreamID
      • measurementID
    • The result should be determined as follows:
      • If only a single measurement ID was passed to the selector:
        • As soon as that web data stream (and thus also its parent property and parent account) has been found, we should return it right away. This is the only option in this scenario.
      • If multiple measurement IDs were passed to the selector:
        • As soon as any web data stream (and thus also its parent property and parent account) has been found that matches one of the passed measurement IDs, check the web data stream's defaultUri.
        • If it refers to the current site, this is an eligible web data stream and we should return it right away (no need to look for alternative options).
        • If it refers to another site, we should keep looking for another one that does refer to the current site.
        • If at the end we have found a web data stream but none that refers to the current site, we should simply return the first one of those.
      • For either scenario, if after going through all web data streams none was found that matched one of the passed measurement IDs, null should be returned.

Note: This selector will be expensive to call due to several resulting API calls, so it should probably receive some doc comments highlighting that.

Implementation Brief

  • Inside assets/js/modules/analytics-4/datastore/webdatastreams.js
    • Create a new getAnalyticsConfigByMeasurementIDs( measurementIDs ) selector by using createRegistrySelector
    • Use getAccountID to get current accountID in use
    • UsegetAccountSummaries result to calculate and store the current account and other propertyIDs in eg. currentAccountPropertiesIDs and otherAccountPropertiesIDs respectively
      • Use getWebDataStreamsBatch to extract data streams for currentAccountPropertiesIDs and look for a match:
        • If only a single measurementID was passed to the selector: as soon as any web data stream has been found that matches passed measurementID return {accountID, propertyID, webDataStreamID, measurementID} data object
        • If multiple measurementIDs was passed to the selector: as soon as any web data stream has been found that matches one of the passed measurementIDs, check the web data stream's defaultUri by using isSiteURLMatch and in case 'true' (it matches) then return {accountID, propertyID, webDataStreamID, measurementID} data object
      • Do the same for otherAccountPropertiesIDs
    • If at the end we have found a web data stream but none that refers to the current site, we should simply return the first one of those that match passed measurementID
    • If after going through all web data streams none was found that matched one of the passed measurementIDs, return null

!NOTE: Since it is a heavy load, splitting between currentAccountPropertiesIDs and otherAccountPropertiesIDs is an optimization. Any other optimizations are welcome. Make sure the code is well documented :)

Test Coverage

Cover getAnalyticsConfigByMeasurementIDs with unit tests

QA Brief

  • Not much to QA here. Quick smoke test can be done to ensure that everything seem to work as intended and nothing is broken.
  • In the browser console ensure that you see the getAnalyticsConfigByMeasurementIDs when you type googlesitekit.data.select( 'modules/analytics-4' ).
    • Ensure that the selector works consistently and returns the correct result for your real account. By consistency, we need to verify that the selector returns undefined when data is being resolved and does not return null for the case when we expect the correct result and it should return null if there is no valid results for provided measurement ID.
    • Ensure that the various permutations described in the AC are covered, e.g. test for a single property, multiple properties, properties with/without a matching web data stream, etc.

Changelog entry

  • Implement getAnalyticsConfigByMeasurementIDs selector to look up GA4 account, property, and web data stream based on a set of measurement IDs.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Jan 4, 2023
@felixarntz felixarntz self-assigned this Jan 4, 2023
@felixarntz felixarntz removed their assignment Jan 5, 2023
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jan 8, 2023
@sashadoes sashadoes assigned sashadoes and unassigned sashadoes Jan 10, 2023
@tofumatt tofumatt self-assigned this Jan 16, 2023
@tofumatt
Copy link
Collaborator

IB ✅

@tofumatt tofumatt removed their assignment Jan 17, 2023
@eugene-manuilov eugene-manuilov self-assigned this Jan 25, 2023
@eugene-manuilov eugene-manuilov removed their assignment Feb 2, 2023
@techanvil techanvil removed their assignment Feb 7, 2023
@wpdarren wpdarren self-assigned this Feb 7, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 7, 2023

QA Update: ⚠️

@eugene-manuilov a quick check. Should getAnalyticsConfigByMeasurementIDs appear when the gteSupport feature flag is not enabled? It does at the moment.

image

@eugene-manuilov
Copy link
Collaborator

@wpdarren, it is not expected to be behind the feature flag, so that is okay that you see it.

@eugene-manuilov eugene-manuilov removed their assignment Feb 7, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 7, 2023

QA Update: ✅

Verified:

  • I've been through setting up Analytics with UA and GA4 properties. No issues.
  • I've been through changing the settings for Analytics. No issues.
  • I've looked through the main and entity dashboard and found no issues.
  • I ran the code in the QAB and I could find getAnalyticsConfigByMeasurementIDs
  • I ran through my checks with the gteSupport feature flag enabled and disabled.
  • No new warnings or errors introduced in the console.

image

@wpdarren wpdarren removed their assignment Feb 7, 2023
@felixarntz
Copy link
Member Author

Approval ❌

@techanvil @eugene-manuilov @sashadoes The selector appears to not work as expected and more importantly is causing a JS error. Below is a screenshot where I call it with a measurement ID of one of my web data streams, however not one with the current site URL set.
Screen Shot 2023-02-09 at 3 39 08 PM

That doesn't seem to be the only problem though, the same thing happens even when I call it with a measurement ID where it has the current site URL set. It first returns null (which it shouldn't since that is a valid measurement ID even for the current site), and then when calling it again it throws an error.

Noting here that the QA Brief is insufficient, as otherwise QA would have already been able to find that bug. We shouldn't just look whether a selector exists, we should also test that it works.

@techanvil
Copy link
Collaborator

Whoops, this one clearly slipped through the net. Thanks for spotting this. @felixarntz, and good point about the QAB.

@eugene-manuilov having taken a cursory look it appears that webDataStream is incorrect here, and it should in fact be webStreamData:

const { _id: webDataStreamID, webDataStream } = datastream;
const {
defaultUri: defaultURI,
measurementId: measurementID, // eslint-disable-line sitekit/acronym-case
} = webDataStream;

@felixarntz
Copy link
Member Author

Approval ✅

Thanks for the quick fix @eugene-manuilov @techanvil!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Module: Analytics Google Analytics module related issues P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants