📈 Initial StorySpec Implementation by Enriqe · Pull Request #23030 · ampproject/amphtml

Enriqe · 2019-06-26T14:36:24Z

Enriqe · 2019-06-26T14:45:21Z

examples/analytics.amp.html

-      "extraUrlParams": {
-        "param1": "Another value"
-      }
+  <script type="application/json">


This file change is just fixing the indentation.

Enriqe · 2019-06-26T15:23:29Z

Hey @cvializ @zhouyx! I know you guys worked on the VideoSpec/VisibilitySpec, I was wondering if you could review the implementation I'm proposing here for the new AmpStorySpec. You can read more context in the issue #23032.

I'm mostly interested in your feedback around the events.js file and if I'm making a correct transition from the CustomEventTracker class to the new AmpStoryEventTracker class I created (which is very similar to the VideoEventTracker).

/cc @calebcordry

cvializ

Nice! A few questions

extensions/amp-story/1.0/story-analytics.js

extensions/amp-analytics/0.1/events.js

zhouyx · 2019-06-26T17:03:21Z

extensions/amp-analytics/0.1/events.js

+        return;
+      }
+
+      if (repeat === false && eventsForPage[type] > 1) {


Instead of passing the eventsForPage as an array, can we pass eventsForPage[type] with the event details instead?

Not sure I understand, can you elaborate more please?

Sorry about the confusion.
If I understand it correctly, this.eventsPerPage_ is used to track how much time an event has occurred for each page here in amp-analytics. All we need is a boolean value indicating if the event has occurred before or not.
Is it possible for the amp-story to pass in that value whenever the analytics event is triggered, along with event.details?

SGTM about using a boolean instead of a number, but I think @cvializ preferred (comment: aa90b03#r297751948) if this logic lived here, instead of it being in story-analytics.js.

@cvializ , are you OK with me moving it back to the story-analytics.js file?

#23030 (comment)

What @cvializ describes also makes sense to me. But when it comes to how many times an event has been triggered, the story knows the best.

I figure a special case can help us decide which way to proceed.
Think about the case where one put two <amp-analytics> element on the page, both listens to the story-page-view event for example, and has repeat equals to false

Say <amp-analytics id=1> got laid out very early, and had the listener ready before the first story-page-view event arrives. While <amp-analytics id=2> only got laid out after the first event arrives. (In theory this won't be an issue, because we buffer all events. But there's still a 10 seconds buffer time) But it still got the second the story-page-view.

What do you think the expected behavior should be. <amp-analytics id=2> still send out the event or not? Personally I'm leaning towards having story keep the count because there could be multiple amp-analytics elements, but only one story that's triggering the events.

That's a good point and a use case I didn't think about (having two amp-analytics elements). I will change it back so that amp-story has the count of events per page - and change it to be a boolean instead of a number. Does this sound good to you? @zhouyx @cvializ

👍 That sounds good. I thought repeat was a number and not a boolean

extensions/amp-analytics/0.1/events.js

Enriqe

Thanks for the quick reviews! 😄

I tried to address all the comments, let me know if I missed something.

extensions/amp-analytics/0.1/events.js

extensions/amp-story/1.0/story-analytics.js

Enriqe · 2019-06-26T18:18:52Z

extensions/amp-analytics/0.1/events.js

+        return;
+      }
+
+      if (repeat === false && eventsForPage[type] > 1) {


Not sure I understand, can you elaborate more please?

Enriqe · 2019-06-28T18:15:25Z

Friendly ping :)

cvializ

LGTM. I don't see any tests, could be good to add in a follow up

Enriqe · 2019-07-08T16:01:55Z

Rebasing this PR since it's been sitting for a while. Will add tests in a follow-up :)

zhouyx

Sorry about the delay. Have two comments, but LGTM.

extensions/amp-analytics/0.1/events.js

zhouyx · 2019-07-08T17:30:59Z

extensions/amp-analytics/0.1/events.js

+        return;
+      }
+
+      if (repeat === false && eventsForPage[type] > 1) {


Sorry about the confusion.
If I understand it correctly, this.eventsPerPage_ is used to track how much time an event has occurred for each page here in amp-analytics. All we need is a boolean value indicating if the event has occurred before or not.
Is it possible for the amp-story to pass in that value whenever the analytics event is triggered, along with event.details?

Enriqe · 2019-07-15T22:05:17Z

Sorry just got back to this, @zhouyx can you please take a look?

extensions/amp-analytics/0.1/events.js

zhouyx · 2019-07-16T02:48:39Z

extensions/amp-analytics/0.1/events.js

+      const details = /** @type {?JsonObject|undefined} */ (getData(event));
+      const detailsForPage = details['detailsForPage'];
+
+      if (repeat === false && detailsForPage['repeated']) {


Thank you for the change!

Enriqe · 2019-07-16T20:45:53Z

Ready for another review @zhouyx 👍

zhouyx

LGTM. But I just noticed there's no unit test for the change, any idea on how to add test coverage?

* Improve readability. * Use map() for eventsPerPage_ * Move logic of updating event counts per page to the tracker.

Enriqe · 2019-07-18T15:38:42Z

Thanks! Was going to add them as a follow up but I figured there's no rush. Just added them in the latest commit 👍

extensions/amp-story/1.0/test/test-analytics.js

gmajoulet · 2019-07-18T16:01:40Z

extensions/amp-analytics/0.1/test/test-events.js

+      const event = new Event('story-page-visible');
+      event.data = defaultVars;
+
+      tracker.sessionObservable_.fire(event);


Re: using private properties during tests
Is there a way to dispatch the event rather than triggering the listener yourself?

I tried triggering the event myself but it wasn't working, the observable wasn't receiving the event. Let me know if you have any other ideas.

I'm not very familiar with the analytics code, but the fact that the listener isn't receiving the event in the should listen on story events test is pretty worrying :(
Just wanted to point this out, maybe it's fine, I'll let you and the owners decide :)

Not super familiar with the analytics code either, but I did see usage of some private properties in this file, which is why I thought it was fine. @zhouyx do you have any thoughts on this?

Could you try tracker.trigger()?

Hmm, the AmpStoryEventTracker doesn't have that method, am I missing something?

oops, sorry i got confused with the customEventTracker. Nvm of the trigger() method.

I tried triggering the event myself but it wasn't working

Could you explain how you are triggering the event? My quick guess is the event is not dispatched from within the analytics root, but from the window.

const rootEl = root.getRootElement(); rootEl.dispatchEvent(event); expect(analyticsEvent).to.be.calledOnce; // Not being called.

Tried different variations of this, let me know if I'm doing something wrong.

Oh, it works using root.getRoot() but not with root.getRootElement() 🤔 I should've known better since the listener is installed with root.getRoot(), I just thought that root.getRootElement() would be in the same scope... my bad.

Enriqe · 2019-07-19T19:47:24Z

If there aren't any other comments I'll go ahead and merge this, thanks all for the reviews! 👍

* initial storyspec implementation * Reviews * Improve readability. * Use map() for eventsPerPage_ * Move logic of updating event counts per page to the tracker. * fix dep error by moving StoryAnalytics to src/analytics * assert absence of selector and default to :root * fix typo * move getdetails back to the story side * reviews * add tests * split tests, only assert repeated field * dispatch the event using getRoot in the test

This reverts commit 1a3eb26.

* Revert "📈 Initial StorySpec Implementation (#23030)" This reverts commit 1a3eb26. * fix import * fix test

…cript-img-with-http-protocol * 'master' of github.com:ampproject/amphtml: (1326 commits) Fix and enable e2e tests for AMPHTML ads FIE rendering mode (ampproject#23995) 🏗 Update WorkerDOM to 0.17.0 (ampproject#24024) Make DocInfo.pageViewId64 async (ampproject#23998) 🐛 Updates amp-sidebar in amp-story (ampproject#23956) Revert "Revert "📖Update documentation for carousel 0.2 (ampproject#23840)" (ampproject#23967)" (ampproject#24016) 🔥 Revert "📈 Initial StorySpec Implementation (ampproject#23030)" (ampproject#24013) Extension skeleton code for payment widgets (ampproject#23045) 🏗🐛 Don't call `travisBuildNumber()` in the global scope (ampproject#24021) Remove suppressTypes from amp-mustache. (ampproject#23993) 🐛 Move `terser` from `dependencies` to `devDependencies` (ampproject#24018) Revert "Revert "Set the new loaders experiment to 1% of traffic. (ampproject#23780)" (ampproject#23963)" (ampproject#24014) SwG release 0.1.22.63 (ampproject#23997) Resolve navTiming variable earlier if possible (ampproject#23580) 🏗 Don't run all the runtime tests for validator-only changes (ampproject#24010) Collect document ready signal (ampproject#23981) Validator rollup (ampproject#24000) Remove flaky story branching test. (ampproject#23994) Include amp-base-carousel in amp-carousel's build. (ampproject#23984) Partial validator rollup (ampproject#23996) amp-bind: Rate-limit history operations (ampproject#23938) ...

Enriqe added the WG: stories label Jun 26, 2019

Enriqe self-assigned this Jun 26, 2019

googlebot added the cla: yes label Jun 26, 2019

Enriqe commented Jun 26, 2019

View reviewed changes

Enriqe force-pushed the story-analytics1 branch 2 times, most recently from f05615e to ae433e8 Compare June 26, 2019 15:14

Enriqe marked this pull request as ready for review June 26, 2019 15:17

Enriqe requested review from cvializ, gmajoulet, newmuis and zhouyx June 26, 2019 15:23

cvializ reviewed Jun 26, 2019

View reviewed changes

zhouyx reviewed Jun 26, 2019

View reviewed changes

Enriqe commented Jun 26, 2019

View reviewed changes

cvializ approved these changes Jun 28, 2019

View reviewed changes

Enriqe force-pushed the story-analytics1 branch from 08df324 to 9166277 Compare July 8, 2019 16:01

zhouyx reviewed Jul 8, 2019

View reviewed changes

Enriqe force-pushed the story-analytics1 branch from 9e74827 to 26926bb Compare July 15, 2019 22:01

zhouyx suggested changes Jul 16, 2019

View reviewed changes

zhouyx approved these changes Jul 16, 2019

View reviewed changes

Enriqe added 5 commits July 17, 2019 16:27

initial storyspec implementation

3fc0460

Reviews

3191092

* Improve readability. * Use map() for eventsPerPage_ * Move logic of updating event counts per page to the tracker.

fix dep error by moving StoryAnalytics to src/analytics

3f59128

assert absence of selector and default to :root

f0d982c

fix typo

d4a9fb8

Enriqe added 3 commits July 17, 2019 16:27

move getdetails back to the story side

d9a34e8

reviews

76dcf52

add tests

ad267ba

Enriqe force-pushed the story-analytics1 branch from c269beb to ad267ba Compare July 18, 2019 15:38

gmajoulet reviewed Jul 18, 2019

View reviewed changes

Enriqe added 2 commits July 18, 2019 13:52

split tests, only assert repeated field

444c19e

dispatch the event using getRoot in the test

330af77

Enriqe merged commit 1a3eb26 into ampproject:master Jul 19, 2019

Enriqe mentioned this pull request Aug 16, 2019

Delay firing early analytic events #24007

Closed

Enriqe added a commit to Enriqe/amphtml that referenced this pull request Aug 16, 2019

Revert "📈 Initial StorySpec Implementation (ampproject#23030)"

f6a7877

This reverts commit 1a3eb26.

This was referenced Aug 16, 2019

🔥 Revert "📈 Initial StorySpec Implementation (#23030)" #24013

Merged

Some analytics for stories are broken #24017

Closed

🌸 Cherry pick request for #24017 into #23939 (Approved) #24020

Closed

Enriqe added a commit that referenced this pull request Aug 16, 2019

🔥 Revert "📈 Initial StorySpec Implementation (#23030)" (#24013)

2918462

* Revert "📈 Initial StorySpec Implementation (#23030)" This reverts commit 1a3eb26. * fix import * fix test

dreamofabear pushed a commit that referenced this pull request Aug 16, 2019

🔥 Revert "📈 Initial StorySpec Implementation (#23030)" (#24013)

714d80f

* Revert "📈 Initial StorySpec Implementation (#23030)" This reverts commit 1a3eb26. * fix import * fix test

dreamofabear pushed a commit that referenced this pull request Aug 16, 2019

🔥 Revert "📈 Initial StorySpec Implementation (#23030)" (#24013)

7014745

* Revert "📈 Initial StorySpec Implementation (#23030)" This reverts commit 1a3eb26. * fix import * fix test

This was referenced Sep 16, 2019

📈 AmpStoryEventTracker & Integration tests #24548

Merged

📈 ✨ Reintroduce storySpec with repeat option #24634

Merged

Conversation

Enriqe commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvializ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Enriqe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jun 28, 2019

Uh oh!

cvializ left a comment

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jul 8, 2019

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jul 15, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jul 16, 2019

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jul 18, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe commented Jun 26, 2019 •

edited

Loading

Enriqe commented Jun 26, 2019 •

edited

Loading

Enriqe Jul 8, 2019 •

edited

Loading

Enriqe Jul 18, 2019 •

edited

Loading

Enriqe commented Jul 19, 2019 •

edited

Loading