Intent to implement: support custom ad (house ad) served from pub's own server

[lannka]

Following the discussion #5442, we want to introduce a new amp-ad impl to support simple image ads served from a custom server. It will not live in an 3P iframe, but rather have simple structure like <a><img></a>. It will load the image src URL and click landing page URL via a single XHR

The tentative HTML will look like:

<amp-ad type="custom" 
    width="300px" height="250px"
    data-url="https://custom.adserver.com/loadad">
</amp-ad>

Implementation plan:

• Introduce a new element impl AmpAdCustom at /extensions/amp-ad/0.1/amp-ad-custom.js.
• Add element upgrade diversion point inside upgradeCallback of /extensions/amp-ad/0.1/amp-ad.js.

@cramforce please confirm if the above plan makes sense
@xgretsch thank you for volunteering. let me know if there's anything unclear.

[cramforce]

@lannka Could this be implemented as a special A4A implementation that is upgraded from amp-ad?

A few things:

• Probably good to support a target on the a tag (default to _top and allow overriding to _blank)
• Must ensure that landing page URL is https URL
• Do we need to support URL replacement on destination URL?

[lannka]

I'm not sure it's a good idea to mix this image ad concept with A4A, and I also don't know how much code can be shared between the 2. To me, a separate upgrade path would be a cleaner approach.

I agree with your other points.

For URL replacement on dest URL, what major use cases are you thinking of? I think we can probably enable it later when requirements are clear.

[cramforce]

All sounds good.

On Wed, Oct 12, 2016 at 2:32 PM, Hongfei Ding notifications@github.com
wrote:

I'm not sure it's a good idea to mix this image ad concept with A4A, and I
also don't know how much code can be shared between the 2. To me, a
separate upgrade path would be a cleaner approach.

I agree with your other points.

For URL replacement on dest URL, what major use cases are you thinking of?
I think we can probably enable it later when requirements are clear.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5541 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeTy1-Ae2NVFoHX3CCSr7E_cKyAFwDks5qzVHsgaJpZM4KUW7N
.

[xgretsch]

OK, so I'm making good progress and I have a basic ad object working, albeit still missing a bunch of stuff. I'm not using A4A, which would in any case be way too complex for me to learn. I need a steer on a few things, however:

(1) I'm trying to provide the facility that when an ad is clicked on, a call is made to Google Analytics to record the event. I can do this from cold easily enough, but I assume that it would be better to use amp-analytics, and I have no idea how to do this. Can someone show me some sample code of how to set up a newly created a tag to fire an event via amp-analytics when it is clicked?

(2) I'm (obviously) going to add something into examples/ads.amp.html, which will need an example ad server. Shall I just use my own production ad server to do this? Or is there an approved place in the AMP world where I can put a couple of files that will be accessed by https? On my default install from github, it all seems to be http only.

(3) Is it best for me to not push anything until the code is what I would consider to be production ready, or to push some code early on (with a bunch of @todo comments) to make sure that I haven't gone down a totally wrong track?

[lannka]

(1) I'm trying to provide the facility that when an ad is clicked on, a call is made to Google Analytics to record the event. I can do this from cold easily enough, but I assume that it would be better to use amp-analytics, and I have no idea how to do this. Can someone show me some sample code of how to set up a newly created a tag to fire an event via amp-analytics when it is clicked?

No code needed for this purpose. Publisher can just setup <amp-analytics> and hook on the anchor click event.

(2) I'm (obviously) going to add something into examples/ads.amp.html, which will need an example ad server. Shall I just use my own production ad server to do this? Or is there an approved place in the AMP world where I can put a couple of files that will be accessed by https? On my default install from github, it all seems to be http only.

No external server needed for this case. We can use the local gulp server. Changes in this file is expected.

(3) Is it best for me to not push anything until the code is what I would consider to be production ready, or to push some code early on (with a bunch of @todo comments) to make sure that I haven't gone down a totally wrong track?

TODO is totally fine. Actually small PR is appreciated.

[xgretsch]

Hi folks - I'm ready to do my first PR (which is also my first ever PR on Github). I'm trying to do it from Github on the Mac, which is telling me "Authentication Failed - You may not have permission to access amphtml". Can someone give me the appropriate permission?

btw Will be away from tomorrow for a week, but will try again on my return.

@lannka - any more detail on how to amend the server.js would be a appreciated - my first attempt resulted in 404 errors.

[lannka]

@xgretsch there's no permission check if you do

1. fork amphtml repo
2. make changes to your own fork,
3. send out a pull request to us.

server.js is using express.js. You'll do something like:

app.get('/path-to-your-ad', function(req, res) {
  res.json({
    clickUrl: 'xxx',
    adSrc: 'xxx',
  });
});

then, you should be able to get the JSON response in your browser: http://localhost:8000/path-to-your-ad

[xgretsch]

@lannka OK, thanks. I thought that's what I did, so I must have got something misconfigured. I'm out of town until Friday, will investigate then. Probably need to try git from the command line without using the Github app.

[lannka]

Per discussion in the other thread (#5721), I'm thinking of expanding the use case to beyond pure image ad. It's still an XHR request returning a JSON object, but we let publisher to render the ad using a template (such as amp-mustache).

The image ad use case becomes:

<amp-ad type="custom" width="300" height="250"
    data-url="https://adserver.com/ad?pubId=1234&slotId=5678&width=300&height=250">
  <div content>
    <template type="amp-mustache">
      <a href="{{clickUrl}}">
        <amp-img src="{{adSrc}}"></amp-img>
      </a>
    </template>
  </div>
</amp-ad>

We can allow pub compose the adrequest data-url using URL replacement.

There shouldn't be new security concern since all the building blocks are already there (very similar to amp-list). For instance, I suppose amp-mustache is responsible for sanitizing any anchor href replacement.

Of course, the supporting ad server will need to implement CORS.

@xgretsch @jasti @cramforce thoughts?

[xgretsch]

@lannka As you've seen, I've just managed my first PR. I hadn't understood the requirement to fork the project first.

I have no objection to imagead being changed to implement a template engine - in which case it would make sense to call it "custom" instead of "imagead". However, I don't how much work it would take to do: I've never used mustache before. I'll take a quick look at it, but it would be nice if you could look at the PR in the mean time.

[cramforce]

@xgretsch I'm wondering: Did you take a look at amp-list? I'd be interested if that would work for you out of the box.

[xgretsch]

@cramforce I have to admit, I hadn't - amp-list sounds like some kind of list box! A quick look suggests that it would work for single ads, but wouldn't give me a way of making a single call to the ad server to fill multiple ad slots (thus ensuring that the same ad doesn't get repeated twice on the same page, as well as improving performance.) Since I've now implemented a working solution (bar the trimmings), it would now seem a pity to go back to that.

[cramforce]

@xgretsch Sorry about that. Yeah, the name is stupid. Could you take the time to take a deeper look? Introducing concepts to AMP is very expensive to us (docs, maintenance, developers have to understand them), so if an existing concept can work, that would be great.

We will rename amp-list at some point. It works totally fine for non-lists :)

[cramforce]

I mean, it is a good question as to whether we should do this, if amp-list
(renamed to amp-render) is all we need.

On Thu, Oct 20, 2016 at 9:20 PM, Malte Ubl malte.ubl@gmail.com wrote:

amp-list (badly named, I repeat myself) can already do that, no?

On Thu, Oct 20, 2016 at 8:29 PM, Hongfei Ding notifications@github.com
wrote:

Per discussion in the other thread (#5721
#5721), I'm thinking of
expanding the use case to beyond pure image ad. It's still an XHR request
returning a JSON object, but we let publisher to render the ad using a
template (such as amp-mustache).

The image ad use case becomes:

We can allow pub compose the adrequest data-url using URL replacement.

There shouldn't be new security concern since all the building blocks are
already there (very similar to amp-list). For instance, I suppose
amp-mustache is responsible for sanitizing any anchor href replacement.

Of course, the supporting ad server will need to implement CORS.

@xgretsch https://github.com/xgretsch @jasti https://github.com/jasti
@cramforce https://github.com/cramforce thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5541 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT8j0qj4AQ0NBto7w3337Y2pzJ_EXks5q2DGugaJpZM4KUW7N
.

[cramforce]

amp-list (badly named, I repeat myself) can already do that, no?

On Thu, Oct 20, 2016 at 8:29 PM, Hongfei Ding notifications@github.com
wrote:

Per discussion in the other thread (#5721
#5721), I'm thinking of
expanding the use case to beyond pure image ad. It's still an XHR request
returning a JSON object, but we let publisher to render the ad using a
template (such as amp-mustache).

The image ad use case becomes:

We can allow pub compose the adrequest data-url using URL replacement.

There shouldn't be new security concern since all the building blocks are
already there (very similar to amp-list). For instance, I suppose
amp-mustache is responsible for sanitizing any anchor href replacement.

Of course, the supporting ad server will need to implement CORS.

@xgretsch https://github.com/xgretsch @jasti https://github.com/jasti
@cramforce https://github.com/cramforce thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5541 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT8j0qj4AQ0NBto7w3337Y2pzJ_EXks5q2DGugaJpZM4KUW7N
.

[lannka]

amp-list can do most of the thing already. For @xgretsch's use case, the only thing missing is the requests batching functionality.

But regardless to it, I still think we should recommend all ads content displayed in amp-ad, including this "house ads" use case.

[xgretsch]

So in theory, either amp-list or, for that matter, amp-iframe or amp-img would do the job, particularly with the addition of the batching class I've just created. But any of these would be just sleight of hand to avoid the policy which says explicitly that you should only use amp-ad for ads. So it seems to me that anything other than putting this into amp-ad only makes sense if you reverse that policy. Which I'm guessing is a far bigger decision that no-one here wants to take right now.

[orrybaram]

Anything happening with this? I need an easy way to test custom amp-ads locally and was hoping this was the solution!

[xgretsch]

Yes - see [https://github.com/ampproject/amphtml/blob/master/ads/custom.md]. It's still labelled "experimental", but it's in production and enabled 100%. See my site [https://bachtrack.com] to look at it in action - go to any article, review or listing page and click "mobile version".

[orrybaram]

This is great, thanks!

[cramforce]

@jasti We should publicize this a little bit :)

Thanks @xgretsch!

[orrybaram]

@xgretsch I checked out the custom docs and have a question for you. We want to be able to inject an iframe instead of an image like you have in your demo. Im assuming that i can do something like:

<amp-ad width=300 height=250
    type="custom"
    data-url="https://mysite/my-ad-server">
    <template type="amp-mustache" id="amp-template-id">
        <amp-iframe layout='fixed' height="300" width="250" src="{{src}}"></amp-iframe>
    </template>
</amp-ad>

The question is, will i have access to parent.context from within the iframe like i would from within a regular ad, or is this equivalent to just having an amp-iframe on the page?

[cramforce]

This is specifically not for iframes. For iframes the only available option that gets access to ad related info is to use amp-ad.

[orrybaram]

Hmm... is there a way to render a custom iframe in amp-ad that doesn't come from a 3rd party? Just need it to test our ads locally. Looking for something like

<amp-ad src='/route/to/my/iframe'></amp-ad>

Thanks for your help!

[cramforce]

@orrybaram What are you actually trying to do? Like what do you want to render in the iframe?

[orrybaram]

@cramforce
Alright, so we build all our ads in-house which eventually get deployed on our server and then served via doubleclick. For ad development, we have a local server running that has an endpoint /iframe that mocks what we'd get back from dfp. We can then dump <iframe src='/iframe'></iframe> into a template and the ad gets rendered. We're looking to have similar control when testing out amp ads, where we can make changes locally to ad templates and test them within a local amp environment. I've tried using amp-embed which works, but doesn't have the same access to things like parent.context which makes it hard to test our ad-hoc (pun intended) tracking lib. Does that make sense?

[cramforce]

@orrybaram Lets move this to a new issue. I'd like to understand why you can't use amp-ad type=doubleclick

[adelinamart]

Hi,
I see some of the issues related to this issue were solved. Are there still any other open question here? Thanks.

[jasti]

Think this launched and can be closed. Pleas reopen if that's not the case.