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

Use Dynamic Measurement ID in Analytics #2800

Merged
merged 27 commits into from
Sep 8, 2020
Merged

Use Dynamic Measurement ID in Analytics #2800

merged 27 commits into from
Sep 8, 2020

Conversation

hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Mar 25, 2020

Switch Analytics to using dynamic measurement ID fetched from REST endpoint. Previously it was using the ID provided in the user's config object. Measurement ID may change so we are switching to fetching it dynamically.

@hsubox76 hsubox76 requested a review from Feiyang1 as a code owner March 25, 2020 20:56
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 25, 2020

Binary Size Report

Affected SDKs

  • @firebase/analytics

    Type Base (d347c6c) Head (47deaa9) Diff
    esm2017 9.89 kB 18.4 kB +8.47 kB (+85.7%)
    main 11.2 kB 23.3 kB +12.1 kB (+108.8%)
    module 10.8 kB 22.8 kB +12.0 kB (+111.1%)
  • @firebase/remote-config

    Type Base (d347c6c) Head (47deaa9) Diff
    browser 23.2 kB 22.8 kB -347 B (-1.5%)
    esm2017 17.7 kB 17.4 kB -341 B (-1.9%)
    main 23.2 kB 22.8 kB -347 B (-1.5%)
    module 22.7 kB 22.4 kB -329 B (-1.4%)
  • @firebase/util

    Type Base (d347c6c) Head (47deaa9) Diff
    browser 20.5 kB 21.1 kB +657 B (+3.2%)
    esm2017 18.3 kB 18.8 kB +507 B (+2.8%)
    main 20.5 kB 21.2 kB +657 B (+3.2%)
    module 19.5 kB 20.1 kB +579 B (+3.0%)
  • firebase

    Type Base (d347c6c) Head (47deaa9) Diff
    firebase-analytics.js 28.3 kB 35.9 kB +7.52 kB (+26.5%)
    firebase-performance-standalone.es2017.js 71.3 kB 71.3 kB -1 B (-0.0%)
    firebase-performance-standalone.js 48.0 kB 48.0 kB -1 B (-0.0%)
    firebase-performance.js 38.4 kB 38.4 kB -1 B (-0.0%)
    firebase-remote-config.js 37.1 kB 37.1 kB +59 B (+0.2%)
    firebase.js 822 kB 829 kB +7.45 kB (+0.9%)

Test Logs

packages/analytics/src/get-config.ts Outdated Show resolved Hide resolved
packages/analytics/src/helpers.ts Outdated Show resolved Hide resolved
@hsubox76 hsubox76 requested a review from egilmorez as a code owner April 7, 2020 00:33
@hsubox76 hsubox76 changed the title WIP: Use Dynamic Measurement ID in Analytics Use Dynamic Measurement ID in Analytics Apr 8, 2020
@hsubox76 hsubox76 removed the request for review from egilmorez April 9, 2020 20:33
@hsubox76 hsubox76 changed the title Use Dynamic Measurement ID in Analytics DO NOT MERGE: Use Dynamic Measurement ID in Analytics Apr 13, 2020
packages/analytics/index.test.ts Outdated Show resolved Hide resolved
packages/analytics/src/errors.ts Outdated Show resolved Hide resolved
packages/analytics/src/exponential_backoff.ts Outdated Show resolved Hide resolved
packages/analytics/src/exponential_backoff.ts Outdated Show resolved Hide resolved
packages/analytics/src/functions.ts Outdated Show resolved Hide resolved
packages/analytics/src/factory.ts Outdated Show resolved Hide resolved
packages/analytics/src/get-config.test.ts Outdated Show resolved Hide resolved
packages/analytics/src/get-config.test.ts Outdated Show resolved Hide resolved
packages/analytics/src/helpers.ts Outdated Show resolved Hide resolved
@changeset-bot
Copy link

changeset-bot bot commented Jul 20, 2020

🦋 Changeset is good to go

Latest commit: 1933243

We got this.

This PR includes changesets to release 25 packages
Name Type
@firebase/remote-config Patch
@firebase/util Patch
@firebase/analytics Minor
@firebase/analytics-types Minor
firebase Patch
@firebase/app Patch
@firebase/component Patch
@firebase/database Patch
@firebase/firestore Patch
@firebase/installations Patch
@firebase/messaging Patch
@firebase/performance Patch
@firebase/rules-unit-testing Patch
@firebase/storage Patch
@firebase/testing Patch
@firebase/app-compat Patch
@firebase/app-exp Patch
@firebase/functions-exp Patch
@firebase/installations-exp Patch
firebase-size-analysis Patch
firebase-namespace-integration-test Patch
firebase-messaging-integration-test Patch
firebase-firestore-integration-test Patch
@firebase/functions Patch
firebase-exp Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/analytics/src/get-config.ts Show resolved Hide resolved
packages/analytics/index.ts Outdated Show resolved Hide resolved
packages/analytics/src/functions.ts Outdated Show resolved Hide resolved
@@ -30,23 +30,27 @@ import { GtagCommand } from './constants';
* @param eventName Google Analytics event name, choose from standard list or use a custom string.
* @param eventParams Analytics event parameters.
*/
export function logEvent(
export async function logEvent(
Copy link
Member

Choose a reason for hiding this comment

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

The other functions are not async. Should we make all of them the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make a difference as they all have to return a promise anyway? Changed them all for consistency in any case.

Copy link
Member

Choose a reason for hiding this comment

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

No, it is only about consistency. You changed the signature of the functions to async but still use then instead of await. We should change them to use await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

packages/analytics/src/get-config.ts Outdated Show resolved Hide resolved
packages/analytics/src/get-config.ts Show resolved Hide resolved
packages/analytics/src/helpers.ts Show resolved Hide resolved
packages/analytics/src/helpers.ts Outdated Show resolved Hide resolved
packages/analytics/src/factory.ts Outdated Show resolved Hide resolved
@@ -37,7 +39,7 @@
"rollup-plugin-commonjs": "10.1.0",
"rollup-plugin-json": "4.0.0",
"rollup-plugin-node-resolve": "5.2.0",
"rollup-plugin-typescript2": "0.27.1",
"rollup-plugin-typescript2": "0.27.0",
Copy link
Member

Choose a reason for hiding this comment

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

We should upgrade to 0.27.1 which is the current version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something always seems to go wrong with the rebasing. Fixed.

Comment on lines 161 to 163
`"apiKey" field is empty in Firebase config. This is needed to fetch the latest` +
` measurement id for this Firebase app. Falling back to measurement id ${app.options.measurementId}` +
` provided in "measurementId" field.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`"apiKey" field is empty in Firebase config. This is needed to fetch the latest` +
` measurement id for this Firebase app. Falling back to measurement id ${app.options.measurementId}` +
` provided in "measurementId" field.`
`The "apiKey" field is empty in the local Firebase config. This is needed to fetch the latest` +
` measurement ID for this Firebase app. Falling back to the measurement ID ${app.options.measurementId}` +
` provided in the "measurementId" field in the local Firebase config.`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 193 to 195
`Timed out fetching this Firebase app's measurement id from the server.` +
` Falling back to measurement id ${measurementId}` +
` provided in "measurementId" field. [${e.message}]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Timed out fetching this Firebase app's measurement id from the server.` +
` Falling back to measurement id ${measurementId}` +
` provided in "measurementId" field. [${e.message}]`
`Timed out fetching this Firebase app's measurement ID from the server.` +
` Falling back to the measurement ID ${measurementId}` +
` provided in the "measurementId" field in the local Firebase config. [${e.message}]`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 214 to 216
`Failed to fetch this Firebase app's measurement id from the server.` +
` Falling back to measurement id ${measurementId}` +
` provided in "measurementId" field. [${e.message}]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Failed to fetch this Firebase app's measurement id from the server.` +
` Falling back to measurement id ${measurementId}` +
` provided in "measurementId" field. [${e.message}]`
`Failed to fetch this Firebase app's measurement ID from the server.` +
` Falling back to the measurement ID ${measurementId}` +
` provided in the "measurementId" field in the local Firebase config. [${e.message}]`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 61 to 64
`Measurement ID in local firebase config (${app.options.measurementId})` +
` does not match measurement ID fetched from server (${config.measurementId}).` +
` To avoid analytics events being sent to the wrong measurement ID, update the` +
` measurement ID field in the local config or remove it.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Measurement ID in local firebase config (${app.options.measurementId})` +
` does not match measurement ID fetched from server (${config.measurementId}).` +
` To avoid analytics events being sent to the wrong measurement ID, update the` +
` measurement ID field in the local config or remove it.`
`The measurement ID in the local Firebase config (${app.options.measurementId})` +
` does not match the measurement ID fetched from the server (${config.measurementId}).` +
` To avoid analytics events being sent to the wrong measurement ID, update the` +
` measurement ID field in the local config or remove it from the local config.`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 50 to 62
'"apiKey" field is empty in Firebase config. Firebase Analytics requires this field to' +
'contain a valid API key.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'"apiKey" field is empty in Firebase config. Firebase Analytics requires this field to' +
'contain a valid API key.',
'The "apiKey" field is empty in the local Firebase config. Firebase Analytics requires this field to' +
'contain a valid API key.',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 53 to 65
'"appId" field is empty in Firebase config. Firebase Analytics requires this field to' +
'contain a valid app ID.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'"appId" field is empty in Firebase config. Firebase Analytics requires this field to' +
'contain a valid app ID.',
'The "appId" field is empty in the local Firebase config. Firebase Analytics requires this field to' +
'contain a valid app ID.',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

logger.warn(
`The measurement ID in the local Firebase config (${app.options.measurementId})` +
` does not match the measurement ID fetched from the server (${config.measurementId}).` +
` To avoid analytics events being sent to the wrong measurement ID, update the` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - is it that the events would be sent to the "wrong Analytics property", rather than the "wrong measurement ID". IIUC, the measurement ID is how the Web App is associated with its Analytics property, i.e., the actual entity that logs the events is the "Analytics property". Can anybody confirm this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be the wrong property. I suppose it could also be no property if it's an invalid measurement ID so I guess I was just trying to be very literal and not go any farther than saying it would make the gtag call using the wrong measurement ID as a parameter, which could cause it to either be sent to the wrong property or no property at all if it's not a valid measurement ID (the previous expired one?).

I'm not sure if that distinction is important but I guess I imagined people not finding it in any of their properties and then saying they were misled or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can flip this statement and say:

To ensure analytics events are always sent to the correct Analytics property, update....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Changed.

Copy link
Contributor

@rachelsaunders rachelsaunders left a comment

Choose a reason for hiding this comment

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

Approving error message copy only.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 24, 2020

Size Analysis Report

Affected Products

No changes between base commit (d347c6c) and head commit (47deaa9).

Test Logs

@hsubox76 hsubox76 force-pushed the ch-measurementid branch 2 times, most recently from 0ccfaba to ad2d2ed Compare August 31, 2020 20:00
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@hsubox76 hsubox76 merged commit fb3b095 into master Sep 8, 2020
@google-oss-bot google-oss-bot mentioned this pull request Sep 8, 2020
@hsubox76 hsubox76 deleted the ch-measurementid branch September 15, 2020 16:44
@firebase firebase locked and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants