Amp-analytics for AdSense/Doubleclick A4A Implementations#7619
Amp-analytics for AdSense/Doubleclick A4A Implementations#7619lannka merged 15 commits intoampproject:masterfrom google:a4a_amp_analytics
Conversation
|
Does this basically just auto-inject analytics vs coding it into a4a html? fyi @oliy |
| }); | ||
| scriptElem.textContent = JSON.stringify(config); | ||
| ampAnalyticsElem.appendChild(scriptElem); | ||
| a4a.element.appendChild(ampAnalyticsElem); |
There was a problem hiding this comment.
We need to make sure this amp-analytics is well scoped to only collect the data related to the creative. So I guess there you will have to add the tracker programmatically. @dvoytenko do you have a pointer how should this be done?
There was a problem hiding this comment.
Let's add scoped attribute to amp-analytics element. E.g. <amp-analytics scoped>
There was a problem hiding this comment.
@dvoytenko any way to add the scoped tracker directly (instead of adding to DOM and parse again)?
There was a problem hiding this comment.
Not yet. But we will add.
There was a problem hiding this comment.
Is there any downside to having two amp-analytics elements inside the same amp-ad block, in case, say, the publisher has inserted one in the page source? This code adds a second -- will that be problematic?
There was a problem hiding this comment.
Added scoped attribute.
|
Responding to @dknecht question: this creates an amp-analytics element outside of the creative but within the amp-ad element. The implementation is specific to adsense/doubleclick (meaning it is within their impls as opposed to the abstract base class) and could be replicated by any impl should they want/need it. |
|
@lannka I'm getting the following error on check-types and am not sure why. It seems to be related by my inclusion of import {createIframePromise} from '../../../../testing/iframe'; in extensions/amp-a4a/0.1/test/utils to allow for centralizing a utility function. All tests appear to pass. Any ideas? Error compiling ./3p/integration.js |
|
@keithwrightbos Thanks for explanation. Can you explain why that is needed vs done inside a4a creative? |
|
@dknecht We want to do this outside the creative for two reasons:
|
|
@keithwrightbos the check-types problem is weird. I think we don't actually check test files. |
|
@lannka PTAL, I fixed the type check issue. Thanks |
tdrl
left a comment
There was a problem hiding this comment.
Not quite done with full read-through. Looks reasonable so far, though.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| return this.renderAmpCreative_(creativeMetaData) | ||
| .then(() => { | ||
| protectedOnCreativeRender(true); | ||
| return true; |
There was a problem hiding this comment.
I assume that true is the correct / an acceptable return value here? I tried reading renderAmpCreative_, but it has gotten depressingly convoluted. I think that at this point, it returns the result of protectedEmitLifecycleEvent_? Regardless, I don't think (?) there are any downstream consumers of the return value of this promise, so maybe it's okay to just return with no value?
There was a problem hiding this comment.
No need to return at all
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| return this.renderNonAmpCreative_() | ||
| .then(() => { | ||
| protectedOnCreativeRender(false); | ||
| return true; |
There was a problem hiding this comment.
No need to return at all
| * Callback executed when AMP creative has successfully rendered within the | ||
| * Callback executed when creative has successfully rendered within the | ||
| * publisher page. To be overridden by network implementations as needed. | ||
| * @param {boolean} isVerifiedAmpCreative whether or not the creative was |
There was a problem hiding this comment.
Nit: Blank line between description and @param.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| /** | ||
| * Render non-AMP creative within cross domain iframe. | ||
| * @return {Promise} awaiting ad completed insertion. | ||
| * @return {Promise<boolean>} awaiting ad completed insertion indicating if |
There was a problem hiding this comment.
I'm afraid I can't parse this comment. :-( Rephrase?
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| * AMP creative. | ||
| * @return {!Promise} Whether the creative was successfully | ||
| * rendered. | ||
| * @return {Promise} Whether the creative was successfully rendered. |
There was a problem hiding this comment.
{Promise<boolean>}? And why lose the !? It looks like it returns a promise guaranteed to me -- am I missing a pathway?
There was a problem hiding this comment.
merge issue. fixed
| const ADSENSE_BASE_URL = 'https://googleads.g.doubleclick.net/pagead/ads'; | ||
|
|
||
| /** @type {string} */ | ||
| const TAG = 'AMP-AD-NETWORK-ADSENSE-IMPL'; |
There was a problem hiding this comment.
Should it be @type {string} or @const {string}? Or does it matter in ES6?
| /** @override */ | ||
| extractCreativeAndSignature(responseText, responseHeaders) { | ||
| setGoogleLifecycleVarsFromHeaders(responseHeaders, this.lifecycleReporter_); | ||
| if (responseHeaders.has(AMP_ANALYTICS_HEADER)) { |
There was a problem hiding this comment.
Can this be factored into shared code between AdSense and Doubleclick impls, similar to setGoogleLifecycleVarsFromHeaders? Maybe in ads/google/a4a/utils.js? Ditto below for onCreativeRender.
ads/google/a4a/test/test-utils.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('#injectActiveViewAmpAnalyticsElement', () => { |
There was a problem hiding this comment.
If you're writing this test from scratch, you should probably use the new describes helpers:
describes.sandboxed('#injectActiveViewAmpAnalyticsElement', {}, () => {
describes.fakeWin('some name', {amp: true}, env => {
const win = env.win;
// tests that can access sandbox and win
}
}I think that will provide you everything that setupForAdTesting is currently giving you, as well as most of what createIframePromise gives you.
There was a problem hiding this comment.
I'd like to do this in a later PR as part of moving to describes.sandboxed
tdrl
left a comment
There was a problem hiding this comment.
I suggested a couple additional tests, but otherwise LGTM.
| /** | ||
| * Creates amp-analytics element within a4a element using urls specified | ||
| * with amp-ad closest selector and min 50% visible for 1 sec. | ||
| * @param {!../../../extensions/amp-a4a/0.1/amp-a4a.AmpA4A} a4a |
There was a problem hiding this comment.
Nit: Clarify that it's the a4a class and not the element. (Should be obvious from type, but defensiveness is good...)
| }); | ||
| scriptElem.textContent = JSON.stringify(config); | ||
| ampAnalyticsElem.appendChild(scriptElem); | ||
| a4a.element.appendChild(ampAnalyticsElem); |
There was a problem hiding this comment.
Is there any downside to having two amp-analytics elements inside the same amp-ad block, in case, say, the publisher has inserted one in the page source? This code adds a second -- will that be problematic?
ads/google/a4a/utils.js
Outdated
| * with amp-ad closest selector and min 50% visible for 1 sec. | ||
| * @param {!../../../extensions/amp-a4a/0.1/amp-a4a.AmpA4A} a4a | ||
| * @param {!../../../src/service/extensions-impl.Extensions} extensions | ||
| * @param {!Array<string>} urls |
There was a problem hiding this comment.
If urls is empty, what's the behavior of this function? In that case, should a block be inserted at all, or should it just skip out?
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Additional test cases: 0 urls and 1 url. Also, calling injectActiveViewAmpAnalyticsElement on an amp-ad that already contains an analytics tag (e.g., from publisher page).
There was a problem hiding this comment.
Added 0 urls case (1 is covered by 2 IMO). There is no concern with another amp-analytics element being present.
|
@lannka PTAL and please merge if looks good and build is green. Thanks |
|
I am unable to see a different CLIENT_ID when testing locally. Steps to reproduce. @dvoytenko can you confirm expected behavior? Is the test incorrect? Add the following item to examples/adsense.amp.html within an amp-ad element with and without scoped attribute. Compare client parameter in generated image urls. <script type=application/json> { "transport": {"beacon": false, "xhrpost": false}, "requests": { "endpoint": "https://raw.githubusercontent.com/ampproject/amphtml/master/examples/img/ampicon.png", "base": "${endpoint}?scoped=true&client=${clientId}" }, "triggers": { "timer": { "on": "timer", "timerSpec": { "interval": 5, "maxTimerLength": 20 }, "request": "base", "vars": { "clientId": "CLIENT_ID(AMP_ECID_GOOGLE)" } } } } </script> |
|
Decided to move on this PR, and fix the scoping issue separately. #7724 |
…#7619) * initial changes * add test coverage * remove dependency on createIframePromise causing type check failure * build failures * build fix * fix test failure * change AV config from urls to url * PR review * Add scoped attribute to amp-analytics * fix lint error * fix lint error * fix presubmit error
Allow for AdSense/Doubleclick A4A implementations to create an amp-analytics element within amp-ad element that will fire activeview pings whose URLs are based on ad response header.