Improve fake ad network by providing working example, updating doc, a… #9295

jonkeller · 2017-05-12T15:42:27Z

Provide a working example of fake ad network
Document the use of fakesig="true"
Fix bug in which, when fakesig="true" is used, it was expecting a file called something.json but which contained HTML, not JSON.
Fix bug in which 2 network requests were issued for the fake creative

jonkeller · 2017-05-12T15:57:33Z

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

+          }
+          // Prevent issueing a second network request to try to fetch
+          // the same JSON again.
+          this.experimentalNonAmpCreativeRenderMethod_ = XORIGIN_MODE.NAMEFRAME;


@keithwrightbos This isn't allowed since this.experimentalNonAmpCreativeRenderMethod_ is private. The reason I want to do this is so that on

amphtml/extensions/amp-a4a/0.1/amp-a4a.js

Line 1219 in 89320dc

const method = this.experimentalNonAmpCreativeRenderMethod_;

it does NOT call this.renderViaCachedContentIframe_(this.adUrl_); 8 lines later, because that is resulting in a second network request for the creative, and would require re-doing all the JSON stuff I just did above.
So I would like to do an /OK/ to override this linter complaint. Or alternatively make a public setter for this, but that seems sketchy too. I figure you own this, per your comment on line 289 of the linked file. Thoughts?

Disregard - next commit is going to make this moot.

lannka · 2017-05-12T20:58:13Z

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

+    if (getMode().localDev && this.element.getAttribute('type') == 'fake') {
+      // Prevent issuing a second network request to try to fetch
+      // the same JSON again.
+      this.experimentalNonAmpCreativeRenderMethod_ = XORIGIN_MODE.NAMEFRAME;


can this be passed in by an attribute?

My original approach was to try to set it from AmpAdNetworkFakeImpl, but it's private to the parent class. I agree this is hacky, but I think any other approach would also be hacky. We could have AmpAdNetworkFakeImpl set some attribute, and then have the parent class say if (this.theAttribute) { this.experimentalNonAmpCreativeRenderMethod_ = theAttribute; } but I'm really bothered by doing any subclass-specific logic in the parent class...

lannka · 2017-05-12T20:59:11Z

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

-      this.adUrl_ = this.getAdUrl();
-      this.adPromise_ = Promise.resolve();
-      return;
+    if (getMode().localDev && this.element.getAttribute('type') == 'fake') {


getMode().localDev check is not needed. we already checked in amp-ad.js

Happy to remove, but note that this check was here previously. Was trying to make as little change as possible...

Let's keep. I'd like to have 100% guarantee that this code is not in PROD binary.

lannka · 2017-05-12T21:07:20Z

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

+            // Gracefully handle the case in which the fake ad is still
+            // formatted in JSON, which it almost certainly is, since
+            // "gulp serve" won't serve an HTML file under /extensions
+            const json = JSON.parse(deserialized);


I'm wondering how does the production logic right now handle this?

Basically the issue here is that normally it wants JSON like { "creative": ...html..., "signature": ... }
However if you use a fake ad, then it was just rendering everything it gets, so the ad would contain the JSON structure above, not just the html. But if you change to , then it doesn't work because gulp won't serve a .html file from extensions/. So the ugly workaround was to make a .json file which contained no JSON, only HTML. This fix just solves that, and affects only the fake ad network.
A real ad would come in the JSON format, but we would never be in AmpAdNetworkFakeImpl. extractCreativeAndSignature() in the first place.

Sorry, should have made my question clearer. And this is mainly for my own learning. Let me know if any of my understanding went wrong:

In fast fetch, runtime first requests a URL via XHR. It returns a JSON (AMP creative) or HTML (non-AMP creative)

In case of non-AMP or invalid AMP, runtime creates an iframe and having the src= attribute pointing to the same URL (the cache frame rendering path). And we expect it to hit the browser cache to save a round trip.

In case of non-AMP, the HTML is perfectly rendered in the iframe. However, for the invalid AMP case, how do we deal with the cached JSON?

@keithwrightbos

Your understanding sounds correct to me. I'm not sure of the answer re: the invalid AMP caching question. I'd be happy to research that for you, or maybe Keith knows off the top of his head...

We parse the creative HTML out of the JSON in both cases. For non-AMP the creative portion only would be written into safeframe and rendered

@keithwrightbos so you meant if the creative is AMP with a wrong signature, we'll always choose SafeFrame rendering method?

lannka · 2017-05-22T15:35:15Z

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

+            // Gracefully handle the case in which the fake ad is still
+            // formatted in JSON, which it almost certainly is, since
+            // "gulp serve" won't serve an HTML file under /extensions
+            const json = JSON.parse(deserialized);


@keithwrightbos so you meant if the creative is AMP with a wrong signature, we'll always choose SafeFrame rendering method?

lannka · 2017-05-22T15:38:32Z

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

+    if (getMode().localDev && this.element.getAttribute('type') == 'fake') {
+      // Prevent issuing a second network request to try to fetch
+      // the same JSON again.
+      this.experimentalNonAmpCreativeRenderMethod_ = XORIGIN_MODE.NAMEFRAME;


see my other comment. I'm trying to see if we can avoid doing this test only logic. We are making the test widget behave differently than the prod widget, so we're not testing the true logic.

If this does not work, we might be hitting a bug?

I believe the fake ad network should never be used in prod at all...

of course it's not in prod.

my point is: why you have to have this logic in test to make it work? and why prod can work without it? how does prod prevent hitting the json cache?

overall, we want to avoid introducing test-only code in non-test. does it make sense?

Ah, I see what you mean. This is due to the setting of "method" here:

amphtml/extensions/amp-a4a/0.1/amp-a4a.js

Line 611 in 49951ce

const method = fetchResponse.headers.get(RENDERING_TYPE_HEADER) ||

In prod, the HTTP header will be set. But when running locally, it isn't. Without explicitly setting it here, we hit the error on line 615 when testing locally.

so i will suggest we change our local server to set the header, instead of putting extra logic into code to bypass this.

jonkeller · 2017-05-29T18:23:34Z

build-system/app.js

@@ -755,16 +755,16 @@ app.use([cloudflareDataDir], function fakeCors(req, res, next) {
 */
 app.get([ fakeAdNetworkDataDir + '/*', cloudflareDataDir + '/*'], function(req, res) {
  var filePath = req.path;
-  var unwrap = false;
+  var unwrap = true;


@lannka Re: your previous question, it turns out that the problem was here. This was implemented backwards. If the creative is a .json file, then it should be unwrapped (signature and HTML extracted), and if it's a .html file then that is not needed. It was done the opposite way. My other main change here is the line that sets the X-AmpAdRender header, which removes the need for the change to amp-a4a.js which you weren't a fan of. Now,

amphtml/extensions/amp-a4a/0.1/amp-a4a.js

Line 611 in 49951ce

const method = fetchResponse.headers.get(RENDERING_TYPE_HEADER) ||

sets method correctly, and we don't hit the error on line 615.

…nd making the signature workaround not require using a .json file which contains HTML rather than JSON

…ter feedback.

jonkeller · 2017-05-30T17:52:17Z

build-system/app.js

  filePath = pc.cwd() + filePath;
  fs.readFileAsync(filePath).then(file => {
+    res.setHeader('X-AmpAdRender', 'nameframe');


(Copying comment over from outdated code, with slight edits for clarity)
@lannka Re: your previous question, it turns out that the problem was here. "unwrap" was implemented backwards. If the creative is a .json file, then it should be unwrapped (signature and HTML extracted), and if it's a .html file then that is not needed (and the extension should NOT be stripped off). But "unwrap" was done the opposite way. My other main change here is the line that sets the X-AmpAdRender header, which removes the need for the change to amp-a4a.js which you weren't a fan of. Now,

amphtml/extensions/amp-a4a/0.1/amp-a4a.js

Line 611 in 49951ce

const method = fetchResponse.headers.get(RENDERING_TYPE_HEADER) ||

sets method correctly, and we don't hit the error on line 615.

much better. appreciate that.

lannka · 2017-05-31T17:50:25Z

build-system/app.js

  filePath = pc.cwd() + filePath;
  fs.readFileAsync(filePath).then(file => {
+    res.setHeader('X-AmpAdRender', 'nameframe');


much better. appreciate that.

lannka · 2017-05-31T18:02:22Z

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

@@ -59,15 +59,24 @@ export class AmpAdNetworkFakeImpl extends AmpA4A {
          // and the signature is "FAKESIG". This mode is only allowed in
          // `localDev` and primarily used for A4A Envelope for testing.
          // See DEVELOPING.md for more info.
-          const creative = this.transformCreativeLocalDev_(deserialized);
+          let creative = this.transformCreativeLocalDev_(deserialized);


now i re-read the code. can we move the transformCreativeLocalDev_ logic to server side? so that we have a correct AMP ad format over the wire. ideally, this should be part of app.use('/a4a(|-3p)/' middleware.

After that, we don't need this try-catch anymore, since we know the exact format over the wire.

again, more logic in this fake extension, the farther away we are from testing the real things.

As it turns out, this change is no longer necessary - it is redundant due to the change in build-system/app.js. So I'll just remove it.

did you try if it still works with the /a4a/xxx style URL?

I'm not familiar with what that is...can you give me an example or pointer to a doc?

https://github.com/ampproject/amphtml/blob/master/contributing/DEVELOPING.md#a4a-envelope-a4a-a4a-3p

I checked at HEAD, and this does not currently seem to work. Unfortunately none of the team was aware of this URL format, so it's likely been broken for quite some time. If it's alright with you, I would suggest that we create a bug to fix that separately, since the current PR does not appear to be the one to have broken this functionality.

sounds good.

lannka · 2017-06-06T21:15:08Z

examples/fake-ad.amp.html

@@ -0,0 +1,24 @@
+<!doctype html>


actually, would you move this file to test/manual/fakeada4a.amp.html, to sit together with test/manual/fakead3p.amp.html

Do customers know to look there? What is the intended difference between an example and a manual test?

example/ is for AMP page authors.
test/manual is for us.

My preference would be to leave it in examples/ so that an AMP page author that wants to write a page and test including ads in it can use this. But it's up to you. Do you think this is valid/useful?

jonkeller · 2017-06-10T17:00:41Z

Thanks!

…

On Sat, Jun 10, 2017 at 10:03 AM, keithwrightbos ***@***.***> wrote: Merged #9295 <#9295>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9295 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACV2B5psri9hXuwCvcVNm0UGGPnd4YHfks5sCqJFgaJpZM4NZdJP> .

jonkeller requested review from dvoytenko and keithwrightbos May 12, 2017 15:42

googlebot added the cla: yes label May 12, 2017

jonkeller commented May 12, 2017

View reviewed changes

jonkeller mentioned this pull request May 12, 2017

Amp-Analytics 3p vendor obfuscation #8887

Closed

lannka requested changes May 12, 2017

View reviewed changes

lannka reviewed May 22, 2017

View reviewed changes

jonkeller commented May 29, 2017

View reviewed changes

jonkeller requested a review from taymonbeal May 30, 2017 15:24

jonkeller added 4 commits May 30, 2017 13:44

Improve fake ad network by providing working example, updating doc, a…

70f5523

…nd making the signature workaround not require using a .json file which contains HTML rather than JSON

Address some linter feedback here. Ongoing discussion about other lin…

4a2a344

…ter feedback.

Address linter issues

50e39d3

Fixed how app.js serves fake ads, removed hack from amp-a4a.js

1624255

jonkeller commented May 30, 2017

View reviewed changes

lannka reviewed May 31, 2017

View reviewed changes

Removes redundant code

be45a36

lannka approved these changes Jun 6, 2017

View reviewed changes

lannka reviewed Jun 6, 2017

View reviewed changes

keithwrightbos merged commit 1277428 into ampproject:master Jun 10, 2017

keithwrightbos deleted the fix_fake_ad_network branch June 10, 2017 14:03

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

Improve fake ad network by providing working example, updating doc, a… #9295

Improve fake ad network by providing working example, updating doc, a… #9295

Conversation

jonkeller commented May 12, 2017

jonkeller May 12, 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

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

jonkeller commented Jun 10, 2017 via email

jonkeller May 12, 2017 •

edited