Amp analytics 3p vendors, 3p side #10184

jonkeller · 2017-06-28T20:26:08Z

Splits PR 9661 in two. This is the 3p lib side. The other side was PRed separately in #10185

Enables amp-analytics tag to use third-party vendors. These vendors' code will be run in a non-AMP iframe.

One-pager: https://docs.google.com/document/d/1mCW7HbQBWQONTWA2q_rKNJyTEdqGOyhX-x-ces4JHWc/edit#

jonkeller · 2017-06-29T13:33:07Z

3p/ampanalytics-lib.js

+      dev().warn(TAG_, 'Attempted to send messages when no listener' +
+        ' configured. Be sure to first call' +
+        ' registerAmpAnalytics3pEventsListener()');
+    }


@keithwrightbos You commented here in the previous PR. I addressed what you said, except an aside: "We could also try and recover from previous invalid/missing onNewAmpAnalyticsInstance here"
Copying over my response from the old PR:
Given the fix to your previous comment, it is no longer possible to reach here without implementing onNewAmpAnalyticsInstance, as the enclosing AmpAnalytics3pMessageRouter will now not have been created.
However, this could still happen if they have done the first line here but not the second:

window.onNewAmpAnalyticsInstance = ampAnalytics => { ampAnalytics.registerAmpAnalytics3pEventsListener(events => {

Unfortunately we can't go and create that for them, so I just made the dev().warn message explicitly say "Attempted to send messages when no listener configured. Be sure to first call registerAmpAnalytics3pEventsListener()"

See previous comment that onNewAmpAnalyticsInstance should be called for each new creative message. I suggest moving onNewAmpAnalyticsInstance check into message receiver and if its not present, warn and do not create message router instance.

jonkeller · 2017-06-29T13:36:45Z

examples/analytics-3p-remote-frame.html

+      // Load the script specified in the iframe’s name attribute:
+      script = document.createElement('script');
+      script.src = 'https://3p.ampproject.net' +
+        JSON.parse(window.name).scriptSrc;


@keithwrightbos Addressed your comment here from the previous PR, re: potential XSS exploit.
However, we don't/can't have getMode().localDev here yet. Would it be sufficient check whether document.location.host == 'localhost' in order to unbreak local testing? Or do you know of a better way?

Let's add a slash after .net just to be safe. Ideally we would also ensure that window.now doesn't include something like scriptSrc: ':javascript()'}
Regarding local testing, I'm curious what @lannka thinks as it appears that remote HTML doesn't support it

Done.
transport.js sends

getMode().localDev ? 'dist.3p/current/ampanalytics-lib.js' : '$internalRuntimeVersion$/ampanalytics-v0.js';

This file takes what was received and sets script.src like so:

script.src = (document.location.host == 'localhost' ? '/' : 'https://3p.ampproject.net/') + JSON.parse(window.name).scriptSrc;

Thus we are guaranteed that in the non-local case we have the full https://3p.ampproject.net/

jonkeller · 2017-06-29T13:54:56Z

build-system/tasks/presubmit-checks.js

@@ -317,6 +318,7 @@ var forbiddenTerms = {
      '3p/iframe-messaging-client.js',
      '3p/ampcontext.js',
      '3p/ampcontext-integration.js',
+      '3p/ampanalytics-lib.js',


Note that this file is changed in both this PR and the accompanying 10185.

keithwrightbos · 2017-06-29T14:16:25Z

3p/ampanalytics-lib.js

+  try {
+    window.onNewAmpAnalyticsInstance(router);
+  } catch (e) {
+    dev().error(TAG_, 'Exception thrown by onNewAmpAnalyticsInstance in' +


I believe if you just include the error as another parameter AMP formats it correctly for reporting (e.g. dev().error(TAG_, 'Exception... ' + this.win_.location.href, e);)

keithwrightbos · 2017-06-29T14:21:04Z

3p/ampanalytics-lib.js

+  }
+}
+
+if (window.onNewAmpAnalyticsInstance) {


OK so we now require that onNewAmpAnalyticsInstance exist BEFORE adding the library? It's not only getting called once per frame? That can't be correct... we want it to be called for every new amp-analytics type= instance encountered on the publisher page. The flow should be:

on new creative message check for onNewAmpAnalyticsInstance and if not present do not create router and warn. Note that you will get event messages. It's up to you how to handle them but I think its fine to just drop with warn message (perhaps worded indicated it may not have been able to be created)

create instance of message router and pass to onNewAmpAnalyticsInstance

keithwrightbos · 2017-06-29T14:21:46Z

3p/ampanalytics-lib.js

+   */
+  sendMessagesToListener(messages) {
+    if (!messages.length) {
+      dev().warn(TAG_, 'Attempted to send zero messages. Ignoring.');


Make sure messages indicate which frame failed the action as there could be multiple on the page

keithwrightbos · 2017-06-29T14:22:34Z

3p/ampanalytics-lib.js

+      dev().warn(TAG_, 'Attempted to send messages when no listener' +
+        ' configured. Be sure to first call' +
+        ' registerAmpAnalytics3pEventsListener()');
+    }


See previous comment that onNewAmpAnalyticsInstance should be called for each new creative message. I suggest moving onNewAmpAnalyticsInstance check into message receiver and if its not present, warn and do not create message router instance.

keithwrightbos · 2017-06-29T14:24:38Z

examples/analytics-3p-remote-frame.html

+      // Load the script specified in the iframe’s name attribute:
+      script = document.createElement('script');
+      script.src = 'https://3p.ampproject.net' +
+        JSON.parse(window.name).scriptSrc;


Let's add a slash after .net just to be safe. Ideally we would also ensure that window.now doesn't include something like scriptSrc: ':javascript()'}
Regarding local testing, I'm curious what @lannka thinks as it appears that remote HTML doesn't support it

keithwrightbos · 2017-06-29T14:25:30Z

src/3p-analytics-common.js

+/**
+ * A class for holding AMP Analytics third-party vendors responses to frames.
+ */
+export class ResponseMap {


This should live in the other PR as it is not part of the client lib (I'm also hoping we can get rid of it completely in favor of a closure based solution...)

Hmm, true. But the exports need to live in both PRs.

keithwrightbos

I also do not see any tests

jonkeller · 2017-07-14T17:07:07Z

Closing this PR, replacing with https://github.com/google/amphtml/pull/20/files which simplifies things and switches to SubscriptionAPI.

Splits PR 9661 in two

2655e56

jonkeller requested review from keithwrightbos and lannka June 28, 2017 20:26

googlebot added the cla: yes label Jun 28, 2017

This was referenced Jun 28, 2017

Amp analytics 3p vendors, impl side #10185

Closed

Amp analytics 3p vendors #9661

Closed

jonkeller added the WG: monetization label Jun 28, 2017

jonkeller commented Jun 29, 2017

View reviewed changes

keithwrightbos requested changes Jun 29, 2017

View reviewed changes

keithwrightbos reviewed Jun 29, 2017

View reviewed changes

jonkeller added 2 commits June 29, 2017 13:16

Addresses review feedback

7d1aa5f

DOES NOT WORK. Work in progress only.

a202eee

jonkeller closed this Jul 14, 2017

jonkeller deleted the amp_analytics_3p_vendors_lib branch July 24, 2017 15:38

jonkeller mentioned this pull request Jul 24, 2017

Introduce iframe transport to amp-analytics - 3p side #10596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amp analytics 3p vendors, 3p side #10184

Amp analytics 3p vendors, 3p side #10184

jonkeller commented Jun 28, 2017 •

edited

jonkeller Jun 29, 2017

keithwrightbos Jun 29, 2017

jonkeller Jun 29, 2017 •

edited

keithwrightbos Jun 29, 2017

jonkeller Jun 29, 2017

jonkeller Jun 29, 2017

keithwrightbos Jun 29, 2017

jonkeller Jun 29, 2017

keithwrightbos Jun 29, 2017

keithwrightbos Jun 29, 2017

jonkeller Jun 29, 2017

keithwrightbos Jun 29, 2017

keithwrightbos Jun 29, 2017

keithwrightbos Jun 29, 2017

jonkeller Jun 29, 2017

keithwrightbos left a comment

jonkeller commented Jul 14, 2017

Amp analytics 3p vendors, 3p side #10184

Amp analytics 3p vendors, 3p side #10184

Conversation

jonkeller commented Jun 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeller Jun 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithwrightbos left a comment

Choose a reason for hiding this comment

jonkeller commented Jul 14, 2017

jonkeller commented Jun 28, 2017 •

edited

jonkeller Jun 29, 2017 •

edited