amp-analytics fires only occasionally in amp-ad-custom

[mwakkiy]

What's the issue?

amp-analytics fires only occasionally in amp-ad-custom

How do we reproduce the issue?

I've made this sample.
https://test.ms-rd.com/amp/amp-ad-custom/test01.html

1. Reload page several times

2. amp-analytics fires only occasionally
   When amp-analytics is successful, there is a request at the following URL.
   https://b94.yahoo.co.jp/1/m?
   https://b94.yahoo.co.jp/1/v?

What browsers are affected?

All browsers

Which AMP version is affected?

Version 2001281851410

[micajuine-ho]

@lannka We took a look at this issue, but think that it's probably ads related rather than analytics related. Would you mind taking a look?

[lannka]

sure, looking

[lannka]

while still looking, some early finding is a race condition between the loading of amp-analytics.js script and the injection of <amp-analytics> in FIE.

[lannka]

it is because the factory method for amp-analytics's services didn't get run in that case

amphtml/src/service/extensions-impl.js

Lines 404 to 418 in 036ab52

	addDocFactory(factory, opt_forName) {
	const holder = this.getCurrentExtensionHolder_(opt_forName);
	holder.docFactories.push(factory);
	// If a single-doc mode, run factory right away if it's included by the doc.
	if (this.currentExtensionId_ && this.ampdocService_.isSingleDoc()) {
	const ampdoc = this.ampdocService_.getAmpDoc(this.win.document);
	const extensionId = dev().assertString(this.currentExtensionId_);
	// Note that this won't trigger for FIE extensions that are not present
	// in the parent doc.
	if (ampdoc.declaresExtension(extensionId) || holder.auto) {
	factory(ampdoc);
	}
	}
	}

It happens when the amp-analytics.js script gets loaded before the <amp-analytics> element gets injected in the FIE iframe.

because <amp-analytics> element is injected dynamically, it missed the opportunity to get stubbed during stubElementsForDoc time.

amphtml/src/custom-element.js

Lines 795 to 804 in 036ab52

	if (
	isStub(this.implementation_) &&
	!ampdoc.declaresExtension(extensionId)
	) {
	Services.extensionsFor(win).installExtensionForDoc(
	ampdoc,
	extensionId
	);
	}
	}

so <amp-analytics> is not stubbed, so the isStub call returns false which prevents installExtensionForDoc from being run.

@dvoytenko @choumx did I miss anything here? is a dynamically injected AMP element supposed to be stubbed if the extension is already fully loaded on the page?

[dreamofabear]

Sorry I'm not familiar with amp-analytics injection in FIE. Is the code above the correct, non-racy path? FIE doesn't actually call installExtensionForDoc itself and relies on custom-element.js?

[dvoytenko]

@lannka I don't think this is the reason here. I'll need to debug this. Do you have a good localdev repro instructions?

[lannka]

@dvoytenko I pushed a branch for testing. pls check out #28882

and thanks for helping!

[lannka]

Discussed with @dvoytenko offline, we both believe the launch of ampdoc-fie experiment will fix the issue. It's not worth at this time to add additional hacks to fix amp-ad-custom. We will prioritize the launch of ampdoc-fie instead.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.