Amp analytics 3p vendors, impl side #10185

jonkeller · 2017-06-28T20:31:48Z

Splits PR 9661 in two. This is the impl side. The other side was PRed separately in #10184

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:39:55Z

extensions/amp-analytics/0.1/test/test-transport.js

+
+  it('must create xframe before sending message to it', () => {
+    expect(() => {
+      transport.sendRequest(window, 'https://example.com/test', {


Copying conversation from old PR:

@keithwrightbos: test referencing window makes me nervous and implies your test may not be hermetic...

Me: I need to use a window object, to hang iframes on. Also the existing tests need one, to do stuff like win.navigator.sendBeacon(request, '');
But I agree it would be best to somehow reset it, or use some type of "test window" object that lives in the sinon sandbox. Do you know a way to do this?

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

extensions/amp-analytics/0.1/test/test-transport.js

+    expect(
+        transport.useExistingOrCreateCrossDomainIframe.calledOnce).to.be.true;
+    expect(transport.createCrossDomainIframe.calledOnce).to.be.true;
+    expect(Transport.hasCrossDomainIframe(config.iframe)).to.be.true;


@keithwrightbos Changed these from assert() per your comment in old PR

jonkeller · 2017-06-29T13:42:49Z

src/anchor-click-interceptor.js

@@ -59,6 +60,14 @@ function maybeExpandUrlParams(ampdoc, e) {
    'CLICK_Y': () => {
      return e.pageY;
    },
+    '3PANALYTICS': (frameType, key) => {
+      const responses = ResponseMap.get(ampdoc, frameType,


Copying conversation from old PR:

@keithwrightbos:
Why do you need responseMap? Idea was to use closure to keep track of data where message listener would update ampDoc.getAnchorClickListenerBinding() value.

Me:
I tried that, but it didn't seem like it was going to work. Though maybe I am missing something - I would be happy to pair on it if you'd like.
As it stands now, ResponseMap is now just a convenience wrapper around ampdoc's binding. I didn't want to spread the implementation details across both anchor-click-interceptor and amp-analytics, especially if they should ever change.

(We then discussed it offline some. Pairing is a possibility after Keith's travel, but as it's just an implementation barrier this should not derail getting it merged according to desired schedule.)

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

3p/iframe-messaging-client.js

@@ -101,6 +101,15 @@ export class IframeMessagingClient {
   */
  setupEventListener_() {
    listen(this.win_, 'message', event => {
+      if (this.hostWindowIsActuallyAnIframe_()) {
+        // this.hostWindow_ can now be set to an iframe, after it has been


Copying conversation from old PR:

@lannka:
why hostWindow can be an iframe?

Me:
It's a chicken-and-egg problem. I've created this iframe, and I want to know when it is ready. So I want to say "I am willing to receive messages from this iframe's contentWindow". But the iframe's content window is not defined until it has finished rendering. So basically, if I can only use the window object, then I can't know it's ready unless I can receive messages from it, but I can't receive messages from it until I know it's ready.

jonkeller · 2017-06-29T13:44:50Z

extensions/amp-analytics/0.1/transport.js

+    const frameData = this.useExistingOrCreateCrossDomainIframe(
+        win, frameUrl, transportOptions['extraData']);
+    const iframeMessagingClient = frameData.iframeMessagingClient;
+    iframeMessagingClient.registerCallback(


Copying conversation from old PR:

@lannka:
should you use SubscriptionApi at host side?

Me:
@keithwrightbos , what do you think? It does seem like this would be better on the AMP side. But AFAICT the 3p side would have to revert to window.addEventListener("message", ...) which loses a lot of what we gain from iframeMessagingClient.

I thought about this overnight. It seems to me that there is no huge clear advantage to switching the underlying implementation to SubscriptionApi. Vamsee is strongly wanting this in the next canary. I propose that we go with IframeMessagingClient for now, and create a bug to consider switching to SubscriptionApi in the future. But if there is some big reason why we should do this now, please let me know.

It's by design that SubscriptionApi handles the host side communication, which is to provide the data. IframeMessagingClient is providing a client stub for calling the API. That's to say, the 2 peers are not parity.

There many aspects here, most importantly, IframeMessagingClient lives in 3p/ code, it's used by those libs, but never AMP runtime (inabox is an exception, but it's a different runtime).

I'm also not convinced to pursue that as a deadline. I don't see we can achieve anything by just getting this PR into next Tuesday's canary.

It seems that the purpose of SubscriptionAPI is to broadcast a message to all subscribing iframes. That's not quite what needs to be done here - I need to direct each message to one particular iframe. But I can make this work with SubscriptionAPI by making the messageType be actually { messageType, ID of recipient }. Please let me know if you can think of a better way than this.

keithwrightbos · 2017-06-29T14:26:35Z

3p/iframe-messaging-client.js

+    // If it's a window, it will have .postMessage
+    // We check for that before .contentWindow, since a cross-domain window
+    // may throw if we try to access anything unsafe
+    return (!this.hostWindow_.postMessage && !!this.hostWindow_.contentWindow);


I don't believe we actually need to test for postMessage as any AMP supported browser would have it.

That's not what I'm doing here. I'm testing whether this.hostWindow_ is actually a window, or an iframe. If it has .postMessage then it's a window.

keithwrightbos · 2017-06-29T14:28:08Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @private
+   */
+  flushQueue_() {
+    if (this.isReady_ && Object.keys(this.creativeToPendingMessages_).length) {


might as well use this.count()

Ah, yes. This predated the existence of count(). Fixed.

keithwrightbos · 2017-06-29T14:28:26Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @return {number}
+   * @VisibleForTesting
+   */
+  count() {


More specific name please (e.g. queueSize)

keithwrightbos · 2017-06-29T14:29:19Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   */
+  enqueue(senderId, opt_data) {
+    dev().assert(!this.creativeToPendingMessages_[senderId],
+        'Replacing existing extra data for: ' + senderId);


Is this the correct error message?

Each creative should only specify extraData once, at init time. If it does so again, this indicates a problem. So I would say that yes, this error message is correct.

keithwrightbos · 2017-06-29T14:29:50Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * iframe
+   */
+  enqueue(senderId, opt_data) {
+    dev().assert(!this.creativeToPendingMessages_[senderId],


Might as well use this.messageFor(senderId)

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

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @VisibleForTesting
+   */
+  messageFor(senderId) {
+    return /** @type {string} */ (


I'm surprised you need to type this given creativeToPendingMessages_ is typed as having string based keys

It's typed has having string-based keys, but we are returning the value, which is typed as string|Array<string>. (String for this subclass, Array for the other.) So without the cast:

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js:167: ERROR - inconsistent return type found : (Array<string>|string) required: string return this.creativeToPendingMessages_[senderId];

keithwrightbos · 2017-06-29T14:31:17Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @param {!string} data The data to be enqueued and then sent to the iframe
+   */
+  enqueue(senderId, data) {
+    if (!this.creativeToPendingMessages_.hasOwnProperty(senderId)) {


this.creativeToPendingMessages_[senderId] = creativeToPendingMessages_[senderId] || [];

Thanks. Leftover from when it was initalized as map() instead of {}. Done.

keithwrightbos · 2017-06-29T14:32:57Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+   * @override
+   * @VisibleForTesting
+   */
+  buildMessage() {


buildMessage and messageFor is duplicated across both classes. Any way to save that?

For buildMessage()...they are building different types of messages: AmpAnalytics3pNewCreativeMessageQueue.buildMessage() builds a AmpAnalytics3pNewCreative message, whereas AmpAnalytics3pEventMessageQueue.buildMessage() builds a AmpAnalytics3pEvent.

Also note that AmpAnalytics3pNewCreativeMessageQueue has messageFor() whereas AmpAnalytics3pEventMessageQueue has messagesFor(), since a creative will only init once but can send any number of events.

I originally had just the superclass and was jumping through hoops to enforce these things, but pivoted and created the subclasses for these reasons.

jonkeller · 2017-06-29T15:21:26Z

src/3p-analytics-common.js

+// The sentinel value will be present when received but the sender doesn't
+// need to add it, this is done by iframe-messaging-client.
+
+/**


Warning: Note that due to splitting the PR, this new file must exist in both. But only lines 1-90 exist in 10184, whereas the full file must exist in 10185. Therefore when we merge, 10184 must be merged first!

lannka · 2017-06-29T17:44:45Z

extensions/amp-analytics/0.1/transport.js

+    const frameData = this.useExistingOrCreateCrossDomainIframe(
+        win, frameUrl, transportOptions['extraData']);
+    const iframeMessagingClient = frameData.iframeMessagingClient;
+    iframeMessagingClient.registerCallback(


It's by design that SubscriptionApi handles the host side communication, which is to provide the data. IframeMessagingClient is providing a client stub for calling the API. That's to say, the 2 peers are not parity.

There many aspects here, most importantly, IframeMessagingClient lives in 3p/ code, it's used by those libs, but never AMP runtime (inabox is an exception, but it's a different runtime).

I'm also not convinced to pursue that as a deadline. I don't see we can achieve anything by just getting this PR into next Tuesday's canary.

…ueueSize()

lannka · 2017-06-29T18:08:02Z

extensions/amp-analytics/0.1/amp-analytics-3p-message-queue.js

+ * @visibleForTesting
+ * @abstract
+ */
+class AbstractAmpAnalytics3pMessageQueue {


We plan to handle the batching use case in a different level. @zhouyx has started looking into it.

Ideally, pub should be able to do

transport: { beacon: true, xhrpost: true, image: true, batchRequestsInMs: 100, }

For example, we can enable it for the GoogleAnalytics post API, which does support batching.

So can you hold the message queue code for now, and assume throttling will happen in a later phase.

Interesting...are there docs for that which I could look at?

not yet. i was over simplifying the spec. there will be challenges of how pub can specify different endpoint for batch request.

nevertheless, we can further scope down this PR by moving batching out.

lannka · 2017-07-11T15:50:26Z

extensions/amp-analytics/0.1/transport.js

+    setStyles(frame,
+      {width: 0, height: 0, display: 'none',
+        position: 'absolute', top: 0, left: 0});
+    const frameSubscribed = () => {}; // Don't care. SubscriptionAPI is


you can use this callback to do the first flush(), instead of try to flush on every enqueue.

Cool, thanks

lannka · 2017-07-11T15:54:13Z

src/3p-analytics-common.js

+  READY: 'R',
+  CREATIVE: 'C',
+  EVENT: 'E',
+  RESPONSE: 'A',


can you explain a bit about those types?

The basic messaging flow is:

xframe sends message informing the parent page that it is ready (R). This /could/ be done with the page load callback, but Keith or Justin, I forget which, prefers sending a message instead. This is because sending a message can happen sooner, i.e. doesn't necessarily require every other (unrelated) resource to be loaded first

Creative sends extra config data to xframe (C). Since multiple creatives may use the same xframe, it doesn't make sense to specify this on the URL/name attr/etc. (We could save a few ms for the first creative to use the xframe I suppose, but another method is definitely required for subsequent creatives to say "here is my extra config data")

Creative sends events (E) to xframe

After some amount of time, or after some number of events, xframe may choose to send a response (A, since R was already taken) to the creative.

lannka · 2017-07-11T15:57:52Z

extensions/amp-analytics/0.1/transport.js

+   * of the amp-analytics config object
+   * @param {function(!string,
+   *   !../../../src/3p-analytics-common.AmpAnalytics3pResponse)=}
+   *   opt_processResponse An optional function to receive any response


is it needed? i don't remember seeing it in the original design

This is essential for one consumer of the API

jonkeller · 2017-07-14T16:49:44Z

Closing this PR, as it has been replaced by a simplified version at #10442

Splits PR 9661 in two

8c4a13d

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

googlebot added the cla: yes label Jun 28, 2017

This was referenced Jun 28, 2017

Amp analytics 3p vendors, 3p side #10184

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

jonkeller commented Jun 29, 2017

View reviewed changes

Addresses review feedback

bf49f83

lannka requested changes Jun 29, 2017

View reviewed changes

Corrects a test still calling count() after that method was renamed q…

f41dea9

…ueueSize()

lannka reviewed Jun 29, 2017

View reviewed changes

DOES NOT WORK. Work in progress only.

e63d05b

lannka requested changes Jul 11, 2017

View reviewed changes

jonkeller mentioned this pull request Jul 14, 2017

Introduce iframe transport to amp-analytics #10442

Merged

jonkeller closed this Jul 14, 2017

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

Amp analytics 3p vendors, impl side #10185

Amp analytics 3p vendors, impl side #10185

Conversation

jonkeller commented Jun 28, 2017

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

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

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

jonkeller Jul 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeller commented Jul 14, 2017

jonkeller Jun 29, 2017 •

edited

jonkeller Jun 29, 2017 •

edited

jonkeller Jul 11, 2017 •

edited