Add SRA support for Doubleclick Fast Fetch Traffic #9563

keithwrightbos · 2017-05-25T21:10:33Z

Still a WIP but contains the following:

If page url includes force_sra=true query parameter, doubleclick request will be sent via SRA
combines doubleclick amp-ad elements by network ID (sends separate SRA requests for different networks)
expect line delimited response format where odd lines are JSON formatted headers and event lines are the creative content
uses streaming fetch method so that blocks can be rendered without waiting for entire SRA response (renders each block as their pair of lines are retrieved) where possible (requires native fetch API) otherwise falls back to fetching entire response prior to processing.
in the event of error, slots are collapsed and frame get fallback method is disabled
resets global SRA fetch executor in unlayoutCallback allowing for resumeCallback initiated chain to refetch via SRA
separates parameters into block and page level. Assumes that only block level parameters included in COMBINER global functions are required. TODO: verify and implement exceptions (e.g. block fields within googleBlockParameters such as ifi, adf, and act).

TODO:

allow for selecting into SRA by publisher and/or experiment
update CSI metrics to properly capture SRA behavior
consider if object key iteration is sufficient to guarantee parameter order (current testing suggests order by insertion is enforced)

cc/ @ampproject/a4a

keithwrightbos · 2017-05-25T21:11:14Z

Looking for initial feedback while tests are updated

Fixes #9115

jridgewell

This looks like two unrelated PRs got merged together? Can you split them up?

jridgewell · 2017-05-26T22:19:47Z

ads/google/a4a/utils.js

+  return googlePageParameters(a4a.win, a4a.getAmpDoc(), startTime)
+    .then(pageLevelParameters => {
+      Object.assign(parameters, blockLevelParameters);
+      Object.assign(parameters, pageLevelParameters);


This can be done in a single call:

Object.assign(parameters, blockLevel, pageLevel);

jridgewell · 2017-05-26T22:21:09Z

ads/google/a4a/utils.js

+  const blockLevelParameters = googleBlockParameters(a4a, opt_experimentIds);
+  return googlePageParameters(a4a.win, a4a.getAmpDoc(), startTime)
+    .then(pageLevelParameters => {
+      Object.assign(parameters, blockLevelParameters);


It's weird to mutate an object you don't own.

I suppose that's fair but given the mutated object is constructed within this function and never exposed, I see no reason not to use it as opposed to creating a copy.

jridgewell · 2017-05-26T22:28:18Z

src/service/xhr-impl.js

@@ -493,6 +493,9 @@ export class FetchResponse {

    /** @type {boolean} */
    this.bodyUsed = false;
+
+    /** @type {?ReadableStream} */
+    this.body = this.xhr_.body;


If you're worried about polyfill not having a body, set to null directly.

isn't this essentially the same end result?

Not really. You're accessing a property here that doesn't exist and isn't expected to exist. Setting it to null makes that clear.

jridgewell · 2017-05-26T22:30:10Z

ads/google/a4a/line-delimited-response-handler.js

+  * @return {!Promise<!../../../src/service/xhr-impl.FetchResponse>} response,
+  *    note that accessing body will cause error as its consumed in streaming.
+  */
+ export function fetchLineDelimitedChunks(xhr, url, chunkHandler, opt_init) {


Where is this used?

https://github.com/google/amphtml/blob/dbclk_sra/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js#L500

jridgewell · 2017-05-26T22:41:57Z

ads/google/a4a/line-delimited-response-handler.js

+     .then(response => {
+       if (!response.body || !xhr.win.TextDecoder) {
+         // TODO(keithwrightbos) - TextDecoder polyfill?
+         response.text().then(content =>


Return here, to avoid nesting in the else (else can be dropped).

jridgewell · 2017-05-26T23:23:56Z

ads/google/a4a/line-delimited-response-handler.js

+         handleFetchResponseStream_(
+           response.body.getReader(), chunkCallback, new TextDecoder('utf-8'));
+       }
+       return response;


Is return necessary?

I wanted to allow access to the SRA request's headers and status code

jridgewell · 2017-05-26T23:38:34Z

ads/google/a4a/line-delimited-response-handler.js

+             snippet += chunk.substr(pos, end - pos);
+             pos = hasDelimiter ? delimIndex + 1 : end;
+             if (hasDelimiter) {
+               if (!metaData) {


Please flip.

jridgewell · 2017-05-26T23:41:46Z

ads/google/a4a/line-delimited-response-handler.js

+         const chunkCallback = (chunk, done) => {
+           let hasDelimiter;
+           let pos = 0;
+           do {


Done though I had to extend the RegExp to handle the case where a line could have no content as this would occur when no creative could be found for a given request.

jridgewell · 2017-05-26T23:42:06Z

ads/google/a4a/line-delimited-response-handler.js

+               // the last slash is used to escape a character that we haven't
+               // received yet. So we skip the last byte in this iteration and
+               // wait for the next chunk to see what is being escaped.
+               end--;


This doesn't make sense.

jridgewell · 2017-05-26T23:48:15Z

ads/google/a4a/line-delimited-response-handler.js

+
+ /**
+  * Creates an XHR streaming request with expected response format of repeated
+  * pairs of JSON object metadata then string HTML delimited by line returns.


This is a lot of code to support streaming line delimited responses. If these single lines are very long and there are very few lines, you're likely not seeing any benefit from streaming the lines separately.

Streaming bodies makes sense when the HTML can be parsed incrementally. As it is now, we're not doing that, we're streaming lines of text.

True and I may look into doing sub-line chunking in the future but currently both of our rendering methods (friendly frame srcdoc and safeframe passing the entire creative as the name attribute) require the full creative prior to rendering start. I plan to look into possible service worker support which would allow streaming (will email to discuss outside this PR).

However, note that in testing the chunk size is 32k (matches gzip window) and its often that can account for 1-2 creative responses. Doubleclick SRA requests commonly have many blocks (happy to share data offline) so I do believe this solution to have meaning impact as it will allow blocks to render prior to receiving the entire SRA response.

I created a sample. Both streaming and all-text paths seem to complete in about ~10ms (using 7 responses, some taking 16k, others taking 8k). I still think this is way over-engineering.

The improvement comes from lower bandwidth usage cases. For instance, if you patch this PR and go to http://localhost:8000/proxy/amp.timeinc.net/time/4458554/best-video-games-all-time/?amp_js_v=0.1&force_sra=true#exp=a4a%3A-1 and emulate 3g, you'll find that the delay between the first and last chunk is ~150 ms which is certainly meaningful in the ad space. The hope is to make SRA the default behavior as it allows for better compression across requests, reduced server usage, and possible downstream optimizations. However, it means that the delay imposed on the first slot's time to render must be as minimal as possible. Chunking helps ensure this and I hope in the future to combine SRA with service workers allowing the results to be streamed into the frame as you suggest.

keithwrightbos · 2017-05-31T14:35:14Z

@jridgewell PTAL. Regarding your comment why this appears to be two PRs instead of one, did you review/see the changes to amp-ad-network-doubleclick-impl.js?

jridgewell · 2017-05-31T18:39:18Z

ads/google/a4a/line-delimited-response-handler.js

+       };
+       handleFetchResponseStream_(
+         response.body.getReader(), chunkCallback, new TextDecoder('utf-8'));
+       return response;


Let's return an object instead of the response. Since this body is being used, it's just going to lead to problems later on if we anyone tries to use it again.

jridgewell · 2017-05-31T18:39:55Z

src/service/xhr-impl.js

@@ -493,6 +493,9 @@ export class FetchResponse {

    /** @type {boolean} */
    this.bodyUsed = false;
+
+    /** @type {?ReadableStream} */
+    this.body = this.xhr_.body;


Not really. You're accessing a property here that doesn't exist and isn't expected to exist. Setting it to null makes that clear.

jridgewell · 2017-05-31T18:45:21Z

ads/google/a4a/line-delimited-response-handler.js

+ export function fetchLineDelimitedChunks(xhr, url, chunkHandler, opt_init) {
+   return xhr.fetch(url, opt_init)
+     .then(response => {
+       if (!response.body || !xhr.win.TextDecoder) {


Actually, can we just send this down the same path? It'll require splitting this method into two:

A check for fast path (body and TextDecoder) or slow path

Extracting lines from the string

jridgewell · 2017-05-31T23:02:10Z

ads/google/a4a/line-delimited-response-handler.js

+
+ /**
+  * Creates an XHR streaming request with expected response format of repeated
+  * pairs of JSON object metadata then string HTML delimited by line returns.


I created a sample. Both streaming and all-text paths seem to complete in about ~10ms (using 7 responses, some taking 16k, others taking 8k). I still think this is way over-engineering.

bradfrizzell · 2017-06-01T19:51:34Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+      'scp': serializeTargeting(
+          this.jsonTargeting_['targeting'] || null,
+          this.jsonTargeting_['categoryExclusions'] || null),
+    }, googleBlockParameters(this));;


please remove second ; (as you just noticed while looking over my shoulder)

bradfrizzell · 2017-06-02T14:17:16Z

ads/google/a4a/utils.js

+ */
+export function groupAmpAdsByType(win, type, groupFn) {
+  return resourcesForDoc(win.document).getMeasuredResources(win,
+      r => r.element.tagName == 'AMP-AD' &&


nit: can you either add {} around the body of the anonymous function or make it all one line?

If I do that then I need to add a return as well... this is so pretty

bradfrizzell · 2017-06-02T14:23:06Z

ads/google/a4a/utils.js

+  return googlePageParameters(a4a.win, a4a.getAmpDoc(), startTime)
+    .then(pageLevelParameters => {
+      Object.assign(parameters, blockLevelParameters, pageLevelParameters);
+      Object.assign(parameters, pageLevelParameters);


Why is the second assign necessary?

bradfrizzell · 2017-06-02T14:25:27Z

extensions/amp-a4a/0.1/test/utils.js

@@ -16,6 +16,7 @@

 import {AmpA4A} from '../amp-a4a';
 import {base64UrlDecodeToBytes} from '../../../../src/utils/base64';
+import '../../../amp-ad/0.1/amp-ad-xorigin-iframe-handler';


Why is this import needed?

removed: Was running into test failures due to assert in amp-a4a

bradfrizzell · 2017-06-02T14:32:27Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+ * @private @const
+ * {!Array<!function(AmpAdNetworkDoubleclickImpl>):?Object<string,string>}
+ */
+const BLOCK_SRA_COMBINERS_ = [


Please add some documentation on this array / what the different functions within it are for

bradfrizzell · 2017-06-02T14:36:37Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

-    const tfcd = jsonParameters['tagForChildDirectedTreatment'];
-    const adTestOn = isInManualExperiment(this.element);
-
+    this.jsonTargeting_ = rawJson ? JSON.parse(rawJson) : {};


If this throws from bad json on the element, is that handled already somewhere else? Otherwise you should use tryParseJson here

That code previously existed but I fixed!

keithwrightbos · 2017-06-02T19:49:42Z

@jridgewell & @bradfrizzell PTAL. I am working on adding more tests to test-amp-ad-network-doubleclick-impl.js to cover combiner array and SRA XHR flow.

jridgewell · 2017-06-02T21:44:53Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+const TFCD_ = 'tagForChildDirectedTreatment';
+
+/** @private {?Promise} */
+let SraRequests = null;


jridgewell · 2017-06-02T21:47:55Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+            // non-SRA request (benefit is it allows direct cache method).
+            if (typeInstances.length == 1) {
+              dev().info(TAG, `single block in network ${networkId}`);
+              typeInstances[0].sraResponseResolver_(null);


Yes and no. It'll work, because Closure doesn't enforce private. But private properties are only for this instance, you can't touch another instances private properties.

jridgewell · 2017-06-02T21:48:51Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+                });
+              })
+              .then(response => lineDelimitedStreamer(this.win, response,
+                metaJsonCreativeGrouper((creative, headers) => {


Let's define this as a function declaration, to avoid this super nesting.

jridgewell · 2017-06-02T21:49:24Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+                  // Force safeframe rendering method.
+                  headers[RENDERING_TYPE_HEADER] = XORIGIN_MODE.SAFEFRAME;
+                  // Need to convert to FetchResponse object?
+                  const fetchResponseHeaders =


Can you just construct a FetchResponseHeaders?

Problem is that the constructor expects !XMLHttpRequest|!XDomainRequest. I could modify the class to take object as an optional object or can I create a mock XMLHttpRequest... thoughts?

jridgewell · 2017-06-02T21:49:38Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+                      has: name => !!headers[name],
+                    });
+                  const fetchResponse =
+                    /** @type {?../../../src/service/xhr-impl.FetchResponse} */


jridgewell · 2017-06-02T21:51:35Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+                  // explicit is required!
+                  instance.resetAdUrl();
+                  if (!canceled) {
+                    instance.forceCollapse();


#forceCollapse should be a privileged API (causes page jumps). Isn't there one that collapses conditionally like #attemptChangeHeight?

bradfrizzell · 2017-06-05T13:56:43Z

ads/google/a4a/utils.js

+ */
+export function groupAmpAdsByType(win, type, groupFn) {
+  return resourcesForDoc(win.document).getMeasuredResources(win,
+      r => r.element.tagName == 'AMP-AD' &&


bradfrizzell · 2017-06-05T14:55:26Z

ads/google/a4a/line-delimited-response-handler.js

+       callback(
+         unescapeLineDelimitedHtml_(line),
+         /** @type {!Object<string, *>} */(tryParseJson(first) || {}),
+         done);


you're calling the callback with 3 arguments, but I'm only seeing callbacks that take 2 arguments getting passed in.

True as I don't actually care if a given chunk is the last. However, I have yet to update CSI beacons and I can see the total processing time for an SRA request being important.

bradfrizzell · 2017-06-05T15:00:18Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+    if (!this.useSra_) {
+      return super.sendXhrRequest(adUrl);
+    }
+    // Wait for SRA request which will ncall response promise when this block's


nit: fix typo 'ncall'

keithwrightbos · 2017-06-07T03:04:19Z

@jridgewell & @bradfrizzell PTAL. I added test coverage for amp-ad-network-doubleclick-impl.js changes. Only remaining items as I see it (and would like to cover in a later PR) are:

integration test coverage
CSI beacon modifications
configure experiment to test and/or possible beta publisher integration

bradfrizzell · 2017-06-07T17:52:04Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

+    let fixture;
+    let xhrMock;
+
+    function createInstance(networkId) {


createA4aSraInstance instead?

bradfrizzell · 2017-06-07T17:56:21Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

+     *    networkId:number,
+     *    instances:number,
+     *    xhrFail:boolean|undefined,
+     *    invalidInstances:number}}>} networkIds


doc incorrect, should be items here or change items to networkIds below. Please add some additional documentation for this function too, or at least the parameter

bradfrizzell · 2017-06-07T18:04:13Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

+        };
+        createInstances(network.instances);
+        createInstances(network.invalidInstances, true);
+        allInvalidNetworks[network.networkId] =


this name isn't really right, seeing as all networks are included in this regardless of whether or not they are invalid. Maybe something like networkValidity?

bradfrizzell · 2017-06-07T18:04:32Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

+        createInstances(network.invalidInstances, true);
+        allInvalidNetworks[network.networkId] =
+          network.invalidInstances && !network.instances;
+        xhrFailingNetworks[network.networkId] = !!network.xhrFail;


same with this

bradfrizzell · 2017-06-07T18:14:41Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

+    function executeTest(items) {
+      const xhrFailingNetworks = {};
+      const allInvalidNetworks = {};
+      const instances = [];


could use a more descriptive name here

bradfrizzell

removing approval

bradfrizzell

lgtm, but add a test of assignAdUrlToError

keithwrightbos added 8 commits May 23, 2017 14:15

Initial implementation of Doubleclick SRA

1144b42

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

733f31d

continued development

457a628

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

798585c

modify url building to pass parameters as object

2ebc596

fix type error

49d42ee

fix type errors

28f728f

reset SraRequests global in unlayoutCallback

616c2ab

keithwrightbos added the WG: monetization label May 25, 2017

keithwrightbos self-assigned this May 25, 2017

keithwrightbos requested review from dvoytenko, bradfrizzell and glevitzky May 25, 2017 21:10

googlebot added the cla: yes label May 25, 2017

keithwrightbos added 4 commits May 26, 2017 11:15

test fixes

6c8b1d6

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

ae90234

line-delimited-response-handler test coverage

911de44

remove console log

a136616

jridgewell requested changes May 26, 2017

View reviewed changes

keithwrightbos added 3 commits May 31, 2017 10:27

PR feedback

46d0865

PR feedback

4788276

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

e3a5115

jridgewell reviewed May 31, 2017

View reviewed changes

keithwrightbos added 2 commits June 1, 2017 14:55

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

9b9cbe6

PR review

a32cf99

bradfrizzell reviewed Jun 1, 2017

View reviewed changes

keithwrightbos added 2 commits June 1, 2017 16:31

PR review, test fixes

154b5a5

small refactor, test fix

b75d821

bradfrizzell reviewed Jun 2, 2017

View reviewed changes

PR feedback

d3de0bb

jridgewell reviewed Jun 2, 2017

View reviewed changes

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

0608e05

bradfrizzell reviewed Jun 5, 2017

View reviewed changes

keithwrightbos added 2 commits June 6, 2017 16:34

PR Review and additional test coverage

e4d8874

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

ff3a2fe

keithwrightbos added 3 commits June 6, 2017 23:13

fix gulp presubmit failures

1bb1fb8

Test fixes/additions

28278a8

slight tweak to how tests are called

1b2a657

bradfrizzell reviewed Jun 7, 2017

View reviewed changes

keithwrightbos added 2 commits June 7, 2017 14:37

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

15c8c09

PR feedback

1b34e91

jridgewell approved these changes Jun 7, 2017

View reviewed changes

keithwrightbos added 3 commits June 7, 2017 15:40

lint fix

77c1106

update sfv parameter

2d1a66e

add still current check after xhr response; add comments

cb3fa07

bradfrizzell approved these changes Jun 7, 2017

View reviewed changes

fix issue with error logging

4c6ed8b

bradfrizzell suggested changes Jun 7, 2017

View reviewed changes

bradfrizzell approved these changes Jun 7, 2017

View reviewed changes

keithwrightbos added 3 commits June 8, 2017 11:09

Additional test coverage

fa74e12

Merge branch 'master' of github.com:ampproject/amphtml into dbclk_sra

9b45a1d

remove only from test runner

e651878

keithwrightbos merged commit 772e029 into ampproject:master Jun 8, 2017

keithwrightbos deleted the dbclk_sra branch June 8, 2017 16:37

ampprojectbot added the PR use: In Beta / Experimental label Jun 20, 2017

keithwrightbos mentioned this pull request Jun 21, 2017

AdSense Fast Fetch: include iu parameter based on data-ad-slot #10072

Merged

Add SRA support for Doubleclick Fast Fetch Traffic #9563

Add SRA support for Doubleclick Fast Fetch Traffic #9563

Conversation

keithwrightbos commented May 25, 2017 • edited

keithwrightbos commented May 25, 2017

jridgewell left a 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

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 commented May 31, 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

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

keithwrightbos commented Jun 2, 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

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 commented Jun 7, 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

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

bradfrizzell left a comment

Choose a reason for hiding this comment

bradfrizzell left a comment

keithwrightbos commented May 25, 2017 •

edited