Initial load analytics event #7312

dvoytenko · 2017-02-02T21:33:39Z

ini-load event is triggered for a document or an embed when the contents positioned within the first window have been loaded. This event is useful on its own and also will be eventually used for visibility events.

cramforce · 2017-02-02T21:42:24Z

src/service/performance-impl.js

-   */
-  whenReadyToRetrieveResources_() {
-    return this.whenReadyToRetrieveResourcesPromise_;
+    const rect = layoutRectLtwh(


Can this use the signal?

In this case it could. But in general, I wanted to avoid creating this signal by default. E.g. without analytics and CSI reporting, this signal is not needed, but it's currently somewhat expensive. WDYT?

Makes sense.

dvoytenko · 2017-02-16T03:03:46Z

@lannka PTAL. Ready for review.

lannka · 2017-02-16T22:32:05Z

extensions/amp-analytics/amp-analytics.md

+The trigger for an embed or an AMP element has to include `selector` that points to the element:
+```javascript
+"triggers": {
+  "renderStart": {


lannka · 2017-02-16T22:32:41Z

extensions/amp-analytics/amp-analytics.md

+```
+
+#### Initial load trigger (`"on": "ini-load"`)
+This event is triggered when initial contents of an element or a document have been loaded.


i guess it's not supported for all elements right? shall we list the ones that support this?

All elements support this. Although for some it's somewhat weird. E.g. amp-pixel. But for most - it's a good signal and very meaningful.

OK, still, I guess it's only for AMP elements, and AMP docs?

Got it. Correct. Fixed.

lannka · 2017-02-16T22:37:41Z

src/service/resources-impl.js

-    );
+    this.visibilityStateMachine_.setState(this.viewer_.getVisibilityState());
+    if (firstPassAfterDocumentReady) {
+      this.ampdoc.signals().signal('ready-scan');


can you document a bit about what this signal is?

lannka · 2017-02-16T22:38:35Z

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

@@ -166,6 +166,7 @@ export const LIFECYCLE_STAGES = {
  layoutAdPromiseDelay: '18',
  signatureVerifySuccess: '19',
  networkError: '20',
+  friendlyIframeIniLoad: '21',


I discussed this with @keithwrightbos, I believe. Though it could have been @tdrl :)

lannka · 2017-02-16T22:41:27Z

src/service/resources-impl.js

+    // the initially parsed elements.
+    // TODO(jridgewell): this path should be switched to use a future
+    // "layer has been measured" signal.
+    return this.ampdoc.signals().whenSignal('ready-scan').then(() => {


shall we put all signals into an enum and have a centralized place for documentation?

Good ideal. Will do. But this signal is a bit weird, so I'll keep it out. This one is just waiting for layers system to take over. I'd rather not invite its reuse.

dvoytenko · 2017-02-17T00:37:00Z

@lannka All done. PTAL.

lannka

one comment

lannka · 2017-02-17T19:20:50Z

extensions/amp-analytics/amp-analytics.md

+```
+
+#### Initial load trigger (`"on": "ini-load"`)
+This event is triggered when initial contents of an element or a document have been loaded.


OK, still, I guess it's only for AMP elements, and AMP docs?

* Initial load analytics event * docs

lannka · 2019-09-18T16:36:01Z

src/service/resources-impl.js

-      return true;
+
+    // First, wait for the `ready-scan` signal. Waiting for each element
+    // individually is too expensive and `ready-scan` will cover most of


Waiting for each element individually is too expensive

@dvoytenko Why is that? do you mean the cost of creating many Promise instances?

I came across this code and wondered if we can get rid of ready-scan (which has a weird / unclear definition itself).

This method was initially exclusively meant to cover performance measurement and thus tried to be as lightweight as possible. But I can't quite recollect how exactly this signal optimizes this. TBH, I'd think a bigger issue here is to wait for elements to be actually there to do this measurement at all. Otherwise it's very possible to make this call before any AMP elements have been parsed/added.

I see. So you meant that we are actually relying on the side effect of this code (waiting for documentReady_, ampInitialized_ & first pass) to wait for resources additions.

If that's the case, what do you think about switching to:

amphtml/src/service/resources-interface.js

Line 115 in 185d498

whenFirstPass() {}

That'd make sense to me. But not sure how much delta between them in practice.

cramforce reviewed Feb 2, 2017

View reviewed changes

dvoytenko changed the title ~~WORK IN PROGRESS: First-viewport-load analytics event~~ WORK IN PROGRESS: First-win-load analytics event Feb 2, 2017

dvoytenko force-pushed the fie14-interload branch from e7cb840 to 60f7f3f Compare February 9, 2017 21:48

dvoytenko changed the title ~~WORK IN PROGRESS: First-win-load analytics event~~ WORK IN PROGRESS: Initial load analytics event Feb 10, 2017

dvoytenko force-pushed the fie14-interload branch from a686e52 to aaaa518 Compare February 10, 2017 02:23

src-code mentioned this pull request Feb 14, 2017

I2I: Rendering native/simple ads outside amp-ad iframe #5721

Closed

dvoytenko force-pushed the fie14-interload branch from a50b65e to 4298ea2 Compare February 16, 2017 02:57

dvoytenko changed the title ~~WORK IN PROGRESS: Initial load analytics event~~ Initial load analytics event Feb 16, 2017

dvoytenko requested a review from lannka February 16, 2017 03:04

dvoytenko assigned lannka Feb 16, 2017

dvoytenko force-pushed the fie14-interload branch from 2af8c23 to 4d807f5 Compare February 16, 2017 21:23

lannka requested changes Feb 16, 2017

View reviewed changes

Initial load analytics event

72d0d55

dvoytenko force-pushed the fie14-interload branch from 2d908e3 to 72d0d55 Compare February 17, 2017 08:11

lannka approved these changes Feb 17, 2017

View reviewed changes

docs

e103525

dvoytenko merged commit d73d4cb into ampproject:master Feb 17, 2017

dvoytenko deleted the fie14-interload branch February 17, 2017 21:12

src-code mentioned this pull request Feb 18, 2017

amp-analytics should fire visibility beacon for every element matched by tag/class #7654

Closed

camelburrito mentioned this pull request Feb 26, 2017

PROD - TypeError: undefined is not an object (evaluating 'id(a).renderStarted') (anonymous function) #7803

Closed

cramforce mentioned this pull request Feb 27, 2017

Regression in pre-render metrics #7815

Closed

dvoytenko mentioned this pull request Mar 6, 2017

Revert to the old prerenderComplete reporting to re-baseline monitoring #7996

Merged

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Initial load analytics event (ampproject#7312)

852a117

* Initial load analytics event * docs

lannka reviewed Sep 18, 2019

View reviewed changes

ampprojectbot mentioned this pull request Apr 27, 2020

🚨 Error: Cannot read property 'set' of undefined #28058

Closed

ampprojectbot mentioned this pull request Mar 4, 2021

🚨 Error: Assertion failed ampproject/error-reporting#65

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial load analytics event #7312

Initial load analytics event #7312

dvoytenko commented Feb 2, 2017 •

edited

Loading

cramforce Feb 2, 2017

dvoytenko Feb 2, 2017

cramforce Feb 2, 2017

dvoytenko commented Feb 16, 2017

lannka Feb 16, 2017

dvoytenko Feb 17, 2017

lannka Feb 16, 2017

dvoytenko Feb 17, 2017

lannka Feb 17, 2017

dvoytenko Feb 17, 2017

lannka Feb 16, 2017

dvoytenko Feb 17, 2017

lannka Feb 16, 2017

dvoytenko Feb 17, 2017

lannka Feb 16, 2017

dvoytenko Feb 17, 2017

dvoytenko Feb 17, 2017

dvoytenko commented Feb 17, 2017

lannka left a comment

lannka Feb 17, 2017

lannka Sep 18, 2019

dvoytenko Sep 18, 2019

lannka Sep 18, 2019

dvoytenko Sep 19, 2019

Initial load analytics event #7312

Initial load analytics event #7312

Conversation

dvoytenko commented Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Feb 16, 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

dvoytenko commented Feb 17, 2017

lannka 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

dvoytenko commented Feb 2, 2017 •

edited

Loading