Backup CID in Storage for non-proxy origin #30375

micajuine-ho · 2020-09-23T20:28:36Z

Change the cid-impl's GetCidDef to include the optOutBackup property.

 * @typedef {{
 *   scope: string,
 *   createCookieIfNotPresent: (boolean|undefined),
 *   cookieName: (string|undefined),
 *   optOutBackup: (boolean|undefined),
 * }}

Adds the opt_optOutBackup parameter to the CLIENT_ID macro in url-replacements, (defaulting to false)

'CLIENT_ID', (scope, opt_userNotificationId, opt_cookieName, opt_optOutBackup) => ...

When the document is served from a non-proxy origin, and optOutBackup is false, we will store any AMP generated CIDs in storage as a backup, as well as search for backup CID value in storage if the cookie does not exist.

It will be stored in local storage in the following manner:

Key: amp-store:${origin}
Value: btoa('{vv: {amp-cid-backup-cookiename: {v: cid, t: timestamp}}}')

The cid-impl is used in other parts of the code base (extensions and src/ad-cid.js), and we have elected to not opt-out to use this new feature in any of these cases.

No changes to inabox as CIDs are not allowed in that environment.

This feature is guarded by an experiment flag: amp-cid-backup. Publishers and vendors will be able to opt into this experiment to test out the feature. After this PR is merged we will turn the flags on for canary and prod, which will give pubs/vendors 2 weeks to change their configs to opt out of this feature.

src/service/cid-impl.js

zhouyx · 2020-09-24T23:15:41Z

src/service/cid-impl.js

@@ -51,6 +51,8 @@ const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout';

 const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptOut';

+const AMP_CID_BACKUP_PREFIX = 'amp-cid-backup-';


I realized that all AMP stored value in origin are stored under the same localstorage key already. amp-store:${origin}.
How important it is for vendors to access the storage backup value? Shall we reuse the namespace?

In the design doc, we stated that we wanted to give publishers the option to access (and potentially change) the backup value. I think reusing the name space is ok as long as it doesn't hinder this goal.

That was my concern: reusing the namespace might hinder the goal.
One need to access the data like this

ampStorage = JSON.parse(atob(localStorage.getItem('amp-store:myorigin'))); cidStorage = ampStorage['amp-cid-backup-scope']

Keeping all AMP stored value under the same key is neat but the tradeoff is that it may be confusing to developers who want to access it outside of AMP.

I would prefer to keep it neat and rely on good documentation and examples to explain how to retrieve and change the values. Do you have a preference?

Discussed offline: We agreed that from the feedback from the original issue thread, AMP is not responsible for changes made to the local storage values in the non-AMP case. If vendors/pubs wish to change the values, it will be up to them to correctly decode the values and then encode and store them back in the proper location. B/c this backup CID is stored under the same namespace as other information, faulty encoding could lead to information being dropped (i.e. stored consent info would be lost).

spec/amp-var-substitutions.md

src/service/cid-impl.js

src/service/storage-impl.js

zhouyx

Looks good to me. Left a few nitpicks and test requests. Please also confirm with the owner of individual extensions based on the affected components list you had. Thanks!

src/service/cid-impl.js

src/service/storage-impl.js

src/service/url-replacements-impl.js

test/unit/test-cid.js

test/unit/test-storage.js

test/unit/test-cid.js

zhouyx · 2020-10-07T23:58:09Z

src/service/storage-impl.js

-    return item ? item['v'] : undefined;
+    const timestamp = item ? item['t'] : undefined;
+    const isNotExpired =
+      opt_duration && timestamp != undefined


Is 0 or negative value a valid opt_duration value? We need to handle them specially if so.

They are not valid values, so I don't think we need to handle them specifically.

CLAassistant · 2020-10-08T23:47:35Z

All committers have signed the CLA.

micajuine-ho · 2020-10-14T02:24:23Z

FYI, this will change the CID service to store the CID in local storage as a backup when the document is served from a non-proxy origin.

The following extensions currently use the CID service: amp-subscription, amp-access, amp-user-notification, and amp-story-interactive.

@dvoytenko @erwinmombay @ampproject/wg-stories

gmajoulet · 2020-10-14T17:16:46Z

Thank you for cc'ing us here!

Just to clarify as I don't have so much knowledge on CID things: this change will make the CID a bit more persistent on non-proxy origins? That sounds like a good things for the way we use CIDs in amp-story-interactive.
Thanks!

micajuine-ho · 2020-10-14T17:57:56Z

Just to clarify as I don't have so much knowledge on CID things: this change will make the CID a bit more persistent on non-proxy origins?

Yes, in this scenario, it will backup the scoped CID (amp-story) to local storage to be used if the scoped CID is not found in cookies.

estherkim

presubmit-check change lgtm 👍

micajuine-ho

OWNERS check

micajuine-ho

Owners bump again

rcebulko

owners bump

micajuine-ho · 2020-10-19T18:44:53Z

/cc @jridgewell and @lannka for bundle size check for v0 and ads.

I think it makes sense that the bundle size increases for these files as we have added functionality to the CLINET_ID macro. Please let me know if you don't think this is expected.

jridgewell · 2020-10-20T03:26:33Z

src/service/cid-impl.js

+  if (isBackupCidExpOn && !disableBackup) {
+    return Services.storageForDoc(ampdoc).then((storage) => {
+      const key = getStorageKey(cookieName);
+      return storage.get(key, BASE_CID_MAX_AGE_MILLIS).then((backupCid) => {


Nit: Unnest this promise chain.

…kup_cid

danielrozenberg

Approval for bundle-size

* init * Added experiment flag * Refactor * Better tests

google-cla bot added the cla: yes label Sep 23, 2020

micajuine-ho force-pushed the backup_cid branch 3 times, most recently from a837605 to 65ef96d Compare September 24, 2020 21:58

init

8a1ecb6

micajuine-ho force-pushed the backup_cid branch from 65ef96d to 8a1ecb6 Compare September 24, 2020 22:17

micajuine-ho changed the title ~~[WIP] Backup CID in Storage~~ Backup CID in Storage for non-proxy origin Sep 24, 2020

zhouyx reviewed Sep 24, 2020

View reviewed changes

micajuine-ho force-pushed the backup_cid branch 3 times, most recently from 861464e to 6f25d61 Compare September 28, 2020 21:06

suggested changes

6f62b0f

micajuine-ho force-pushed the backup_cid branch from 6f25d61 to 6f62b0f Compare September 29, 2020 19:29

micajuine-ho marked this pull request as ready for review September 29, 2020 20:17

amp-owners-bot bot requested review from dreamofabear and estherkim September 29, 2020 20:17

Added experiment flag

04374ea

micajuine-ho force-pushed the backup_cid branch from 3d91ae2 to 04374ea Compare September 30, 2020 21:25

zhouyx reviewed Sep 30, 2020

View reviewed changes

spec/amp-var-substitutions.md Outdated Show resolved Hide resolved

src/service/cid-impl.js Outdated Show resolved Hide resolved

src/service/cid-impl.js Outdated Show resolved Hide resolved

src/service/storage-impl.js Outdated Show resolved Hide resolved

Refactor

32b42a2

zhouyx reviewed Oct 2, 2020

View reviewed changes

micajuine-ho force-pushed the backup_cid branch from 420cbe7 to df26520 Compare October 6, 2020 22:57

Better tests

8bdd915

micajuine-ho force-pushed the backup_cid branch from df26520 to 8bdd915 Compare October 6, 2020 22:59

zhouyx reviewed Oct 7, 2020

View reviewed changes

Merge branch 'master' into backup_cid

7eeed8c

estherkim approved these changes Oct 15, 2020

View reviewed changes

micajuine-ho closed this Oct 16, 2020

micajuine-ho reopened this Oct 16, 2020

micajuine-ho commented Oct 16, 2020

View reviewed changes

amp-bundle-size bot requested a review from alanorozco October 16, 2020 18:10

micajuine-ho commented Oct 16, 2020

View reviewed changes

micajuine-ho closed this Oct 19, 2020

micajuine-ho reopened this Oct 19, 2020

rcebulko reviewed Oct 19, 2020

View reviewed changes

micajuine-ho requested review from lannka and jridgewell and removed request for alanorozco and dreamofabear October 19, 2020 18:41

jridgewell approved these changes Oct 20, 2020

View reviewed changes

Micajuine Ho added 2 commits October 20, 2020 12:54

merge

2dfffa9

Merge branch 'backup_cid' of github.com:micajuine-ho/amphtml into bac…

d38344d

…kup_cid

micajuine-ho force-pushed the backup_cid branch from e0716b8 to d38344d Compare October 21, 2020 18:07

jridgewell approved these changes Oct 21, 2020

View reviewed changes

danielrozenberg approved these changes Oct 21, 2020

View reviewed changes

lannka approved these changes Oct 22, 2020

View reviewed changes

micajuine-ho merged commit fb809b0 into ampproject:master Oct 23, 2020

ampprojectbot added the PR use: In Beta / Experimental label Nov 3, 2020

micajuine-ho mentioned this pull request Nov 10, 2020

Turn on amp-backup-cid experiment in canary #31067

Merged

micajuine-ho mentioned this pull request Nov 23, 2020

Turn on amp-cid-backup experiment in prod #31298

Merged

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020

Backup CID in Storage for non-proxy origin (ampproject#30375)

d3ef9c9

* init * Added experiment flag * Refactor * Better tests

micajuine-ho mentioned this pull request Feb 22, 2021

[amp-analytics] googleanalytics and gtag opt out of CID backup #32805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backup CID in Storage for non-proxy origin #30375

Backup CID in Storage for non-proxy origin #30375

micajuine-ho commented Sep 23, 2020 •

edited

Loading

zhouyx Sep 24, 2020 •

edited

Loading

micajuine-ho Sep 25, 2020

zhouyx Sep 25, 2020

micajuine-ho Sep 28, 2020

micajuine-ho Sep 29, 2020

zhouyx left a comment

zhouyx Oct 7, 2020

micajuine-ho Oct 9, 2020

CLAassistant commented Oct 8, 2020 •

edited

Loading

micajuine-ho commented Oct 14, 2020

gmajoulet commented Oct 14, 2020

micajuine-ho commented Oct 14, 2020

estherkim left a comment

micajuine-ho left a comment

micajuine-ho left a comment

rcebulko left a comment

micajuine-ho commented Oct 19, 2020

jridgewell Oct 20, 2020

danielrozenberg left a comment

		@@ -51,6 +51,8 @@ const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout';

		const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptOut';

		const AMP_CID_BACKUP_PREFIX = 'amp-cid-backup-';

Backup CID in Storage for non-proxy origin #30375

Backup CID in Storage for non-proxy origin #30375

Conversation

micajuine-ho commented Sep 23, 2020 • edited Loading

zhouyx Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

micajuine-ho Sep 25, 2020

Choose a reason for hiding this comment

zhouyx Sep 25, 2020

Choose a reason for hiding this comment

micajuine-ho Sep 28, 2020

Choose a reason for hiding this comment

micajuine-ho Sep 29, 2020

Choose a reason for hiding this comment

zhouyx left a comment

Choose a reason for hiding this comment

zhouyx Oct 7, 2020

Choose a reason for hiding this comment

micajuine-ho Oct 9, 2020

Choose a reason for hiding this comment

CLAassistant commented Oct 8, 2020 • edited Loading

micajuine-ho commented Oct 14, 2020

gmajoulet commented Oct 14, 2020

micajuine-ho commented Oct 14, 2020

estherkim left a comment

Choose a reason for hiding this comment

micajuine-ho left a comment

Choose a reason for hiding this comment

micajuine-ho left a comment

Choose a reason for hiding this comment

rcebulko left a comment

Choose a reason for hiding this comment

micajuine-ho commented Oct 19, 2020

jridgewell Oct 20, 2020

Choose a reason for hiding this comment

danielrozenberg left a comment

Choose a reason for hiding this comment

micajuine-ho commented Sep 23, 2020 •

edited

Loading

zhouyx Sep 24, 2020 •

edited

Loading

CLAassistant commented Oct 8, 2020 •

edited

Loading