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

Shall we generate cookie for Ad CID? #9268

Closed
lannka opened this issue May 11, 2017 · 13 comments
Closed

Shall we generate cookie for Ad CID? #9268

lannka opened this issue May 11, 2017 · 13 comments

Comments

@lannka
Copy link
Contributor

lannka commented May 11, 2017

Code here and here and here implies that we do not generate cookie for Ad CID. Is it a bug or working as intended?

@avimehta @cramforce @dvoytenko @rudygalfi

@lannka lannka added this to the Sprint H1 May milestone May 11, 2017
@lannka lannka self-assigned this May 11, 2017
@avimehta
Copy link
Contributor

WAI. The id is used for GA and Ads data joining and if there is no analytics tag on the page, Ads doesn't need to send it across.

@lannka
Copy link
Contributor Author

lannka commented May 11, 2017

Let amp-ad set cookie can potentially increase the linking coverage, imagine user visited pages without amp-analytics first. My understanding is that in non-AMP world, ads tag also write the cookie if it does not exist yet, right?

Also, I saw one other ad network is specifying a cookie scope: AMP_ECID_EZOIC which probably was an mis-understanding of this feature.

@lannka
Copy link
Contributor Author

lannka commented May 11, 2017

And if you read the README, the window.context.clientId is something we persist for ad network.

@rudygalfi

@cramforce
Copy link
Member

The current feature doesn't support writing a cookie. This could be changed, but the current user doesn't want it.

@lannka
Copy link
Contributor Author

lannka commented May 17, 2017

Then README is misleading.

  • window.context.clientId contains a unique id that is persistently the same for a given user and AMP origin site in their current browser until local data is deleted or the value expires (expiration is currently set to 1 year).
    • Ad networks must register their cid scope in the variable clientIdScope in _config.js. Use clientIdCookieName to provide a cookie name for non-proxy case, otherwise value of clientIdScope is used.
    • Only available on pages that load amp-analytics. The clientId will be null if amp-analytics was not loaded on the given page.

@cramforce
Copy link
Member

You mean to add a bullet point:

  • A cookie if it is not present yet.

?

@lannka
Copy link
Contributor Author

lannka commented May 17, 2017

Or we can fix the behavior. Why don't we just write a cookie if it does not exist. I think it's valuable as ads in 3P frame will have problem to do cookie in safari.

Non-related finding: I saw we have 10 networks calling context.clientId, but only 3 configured clientIdScope in _config.js. Seems this is something few people doing right.

@cramforce
Copy link
Member

I don't have a strong opinion. It would certainly be reasonable to change.

@lannka
Copy link
Contributor Author

lannka commented May 19, 2017

@rudygalfi @avimehta any opinion on this? The behavior is inconsistent on CDN and pub hosted domain, because on CDN ad network can always get an AdCID, but on pub hosted domain, it relies on a cookie written by amp-analytics, which is not always there.

@rudygalfi
Copy link
Contributor

This can be restricted to just write cookies for ad networks that are used right (we don't write a 1P cookie for all ad networks unconditionally)?

@lannka
Copy link
Contributor Author

lannka commented May 23, 2017

we only write cookie if

  • the ad network specified clientIdScope
  • pub is using that ad network

@avimehta
Copy link
Contributor

Just started looking into this. I think it is reasonable to write the cookie for ads loaded in a page on pub domains.

Note that the pub still needs to load amp-analytics to be able to write cookies. If the pub doesn't load amp-analytics, a client id will not be returned in either viewer or pub origin case.

@lannka
Copy link
Contributor Author

lannka commented May 26, 2017

@avimehta sounds good.

and we do plan to promote CID logic from amp-analytics into core #4898

@jasti jasti added this to Bug Backlog in AMP Advertising Jun 1, 2017
@lannka lannka removed the Type: Bug label Jun 1, 2017
@jasti jasti moved this from Bug Backlog to Done in AMP Advertising Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

5 participants