-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Amp analytics support taboola #32803
Amp analytics support taboola #32803
Conversation
to/ @micajuine-ho for review |
"transport": { | ||
"beacon": false, | ||
"xhrpost": false, | ||
"image": true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amp-analytics handles defaulting from beacon => xhrpost => image. Just making sure that you want image
requests to always be sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho Reviewed the docs again. We dont need any overrides. So removed that section. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho After testing I need the image GET request
"pageview": { | ||
"on": "visible", | ||
"request": "url", | ||
"iframePing": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iframePing
is used as a workaround for vendors transitioning to amp-analytics integration. Is this true for your case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho did not understand that. My bad. So removed that configuration.
@@ -476,6 +476,9 @@ | |||
"selfDescribingEvent": "https://!collectorHost/i?p=web&tv=amp-1.0.2&e=ue&ue_pr=%7B%22schema%22%3A%22iglu%3Acom.snowplowanalytics.snowplow%2Funstruct_event%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22schema%22%3A%22iglu%3A!customEventSchemaVendor%2F!customEventSchemaName%2Fjsonschema%2F!customEventSchemaVersion%22%2C%22data%22%3A!customEventSchemaData%7D%7D&url=_ampdoc_url_&page=_title_&res=_screen_width_x_screen_height_&dtm=_timestamp_&tz=_timezone_code_&aid=!appId&cd=_screen_color_depth_&cs=_document_charset_&lang=_browser_language_&refr=_document_referrer_&vp=_viewport_width_x_viewport_height_&ua=_user_agent_&ds=_scroll_width_x_scroll_height_&uid=!userId&co=%7B%22schema%22%3A%22iglu%3Acom.snowplowanalytics.snowplow%2Fcontexts%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%5B%7B%22schema%22%3A%22iglu%3Adev.amp.snowplow%2Famp_id%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22ampClientId%22%3A%22_client_id(_sp_ampid)_%22%2C%20%22domainUserid%22%3A%20%22_if(_substr(_query_param(_sp)_%2C0%2C36)_%2C_substr(_query_param(_sp)_%2C0%2C36)_%2C_if(_linker_param(sp_amp_linker%2C_sp_duid)_%2C_linker_param(sp_amp_linker%2C_sp_duid)_%2C_cookie(_sp_duid)_)_)_%22%2C%20%22userId%22%3A%20%22!userId%22%7D%7D%2C_replace(!customContexts%2C%5E%2C*(.%2B%3F)%2C*%24%2C%241%2C)_%7B%22schema%22%3A%22iglu%3Adev.amp.snowplow%2Famp_web_page%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22ampPageViewId%22%3A%22_page_view_id_64_%22%7D%7D%5D%7D", | |||
"ampPagePing": "https://!collectorHost/i?p=web&tv=amp-1.0.2&e=ue&ue_pr=%7B%22schema%22%3A%22iglu%3Acom.snowplowanalytics.snowplow%2Funstruct_event%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22schema%22%3A%22iglu%3Adev.amp.snowplow%2Famp_page_ping%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22scrollLeft%22%3A_scroll_left_%2C%22scrollWidth%22%3A_scroll_width_%2C%22viewportWidth%22%3A_viewport_width_%2C%22scrollTop%22%3A_scroll_top_%2C%22scrollHeight%22%3A_scroll_height_%2C%22viewportHeight%22%3A_viewport_height_%2C%22totalEngagedTime%22%3A_total_engaged_time_%7D%7D%7D&url=_ampdoc_url_&page=_title_&res=_screen_width_x_screen_height_&dtm=_timestamp_&tz=_timezone_code_&aid=!appId&cd=_screen_color_depth_&cs=_document_charset_&lang=_browser_language_&refr=_document_referrer_&vp=_viewport_width_x_viewport_height_&ua=_user_agent_&ds=_scroll_width_x_scroll_height_&uid=!userId&co=%7B%22schema%22%3A%22iglu%3Acom.snowplowanalytics.snowplow%2Fcontexts%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%5B%7B%22schema%22%3A%22iglu%3Adev.amp.snowplow%2Famp_id%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22ampClientId%22%3A%22_client_id(_sp_ampid)_%22%2C%20%22domainUserid%22%3A%20%22_if(_substr(_query_param(_sp)_%2C0%2C36)_%2C_substr(_query_param(_sp)_%2C0%2C36)_%2C_if(_linker_param(sp_amp_linker%2C_sp_duid)_%2C_linker_param(sp_amp_linker%2C_sp_duid)_%2C_cookie(_sp_duid)_)_)_%22%2C%20%22userId%22%3A%20%22!userId%22%7D%7D%2C_replace(!customContexts%2C%5E%2C*(.%2B%3F)%2C*%24%2C%241%2C)_%7B%22schema%22%3A%22iglu%3Adev.amp.snowplow%2Famp_web_page%2Fjsonschema%2F1-0-0%22%2C%22data%22%3A%7B%22ampPageViewId%22%3A%22_page_view_id_64_%22%7D%7D%5D%7D" | |||
}, | |||
"taboola": { | |||
"url": "https://3p.ampproject.net/custom/amp-analytics-taboola.html?url=${canonicalUrl}&aid=${aid}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho I need this file https://3p.ampproject.net/custom/amp-analytics-taboola.html to be checked in or hosted at 3p.ampproject.net. Is there any documentation on how to do it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @micajuine-ho, what would you recommend us to do here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need it to be hosted on that URL rather than https://cdn.ampproject.org/v0/analytics-vendors/taboola.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho part of the config in taboola.json is to load that html file which in turn loads our tracking javascript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about this?
"requests": {
"url": "https://3p.ampproject.net/custom/amp-analytics-taboola.html?url=${canonicalUrl}&aid=${aid}"
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho Running it locally http://localhost:8000/examples/analytics-vendors.amp.html?type=taboola. The error shows up in console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see, on that test page, the CORB error is occurring with all the vendors. If you isolate your analytics example by moving it to it's own test page, you won't see the error anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho yeah. I tested it on one of our publisher page and works fine. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@micajuine-ho what are the next steps for getting it merged ?
@udaya-m-taboola Please feel free to ping me on this thread if I don't respond in 1-2 days. Sometimes these things just slip through the cracks. |
"aid": "" | ||
}, | ||
"requests": { | ||
"url": "https://c3.taboola.com/amp/amp-analytics-taboola.html?url=${canonicalUrl}&aid=${aid}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest you do a more descriptive request name than, url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to loadamptrkhtml instead
Please rebase. |
Your unit tests are failing. Please fix before we can merge. |
@micajuine-ho thanks for merging it. Usually what is the timeframe for it to go live ? |
2 weeks. You can check on https://amp-release-calendar.appspot.com/ |
No description provided.