Add Activity service to calculate engaged time for amp-analytics #1818

britice · 2016-02-05T22:47:30Z

Add new Activity service which can be used to track general user interaction on a page over time.

The first goal of adding this service is to provide active engaged time heuristics for url replacement in amp-analytics. This will address issue #1296.

In order to collect active engaged time heuristics, user activity will need to be collected during the session. Given that user activity will be collected, there is a chance that the Activity service will also be able to expose additional data about general user interaction beyond total engaged time.

britice · 2016-02-05T23:04:01Z

So far the Activity service is only providing a stubbed value for the TOTAL_ENGAGED_TIME url replacement.

Similar to the CID service, the Activity service is not installed in amp-core-services.js. Instead, it is installed in amp-analytics.js.

Let me know if there are suggestions, especially concerning:

The general pattern being used to provide engaged time.
Installing the Activity service in amp-analytics.js rather than as a core service.

cramforce · 2016-02-05T23:04:52Z

@avimehta
Will take a look later. From a glance at the files more tests would be appreciated :)

britice · 2016-02-05T23:07:44Z

From a glance at the files more tests would be appreciated :)

Definitely understood. This PR is still a work in progress. I included at least the one test to demonstrate how the service is used.

rudygalfi · 2016-02-09T20:45:29Z

spec/amp-var-substitutions.md

+### TOTAL_ENGAGED_TIME
+
+Get the total time the user has been enagaged with the page since the page was
+loaded.


Loaded, or became visible?

Might be good to be specific here.

cc @cramforce @avimehta for feedback on this.

Become visible in the current viewport I think is the more accepted definition

yes, lets change this to "become visible in the viewport"

rudygalfi · 2016-02-09T20:51:00Z

This looks like a reasonable start.

Regarding whether the Activity service should be in amp-analytics or core: I can definitely imagine this being relevant to other areas. Perf and ads come to mind, but those are entirely hypothetical from my vantage point. @cramforce do you have any take on this? Is starting in amp-analytics and refactoring to core later an option?

etlgfx · 2016-02-09T21:27:12Z

I'd love to help you guys out with this if you guys think that would be useful?

britice · 2016-02-09T22:54:36Z

I'd love to help you guys out with this if you guys think that would be useful?

Thanks @etlgfx. I'm working to get another commit out tonight or early tomorrow which contains the core functionality for the Activity service. Once I get that out, I'm sure there will be some additional feedback needed.

As was mentioned earlier, I need to take care of testing for this feature. Any input there would be greatly appreciated (e.g. what to test, where to test, how to ensure good test coverage).

britice · 2016-02-11T16:42:30Z

I pushed a new commit which fleshes out the total engaged time functionality.

This went through several iterations before landing on only tracking total engaged time. In one version, all activity times were recorded (with 1 second resolution). This would have allowed for finding engaged time for an arbitrary time period but would require either recalculation of engaged time on each getTotalEngagedTime request or a more complex data structure which stored previous calculations of engaged time intervals. In the end, I opted for a simpler approach which could gather the most important metric with the lowest potential impact on performance.

There is still more to do and I'd appreciate feedback on the functionality. In the mean time, I'll start working on tests before adding other activity listeners.

rudygalfi · 2016-02-13T00:36:23Z

cc @msukmanowsky

britice · 2016-02-16T19:52:37Z

@avimehta @cramforce I added another commit which includes functional tests for the Activity service. It also includes changes which account for user "inactivity" events.

britice · 2016-02-17T23:38:59Z

Brought things up to date with master.

britice · 2016-02-18T22:43:26Z

@avimehta @cramforce After the Travis check is complete, this PR is ready for review.

cramforce · 2016-02-18T23:07:53Z

spec/amp-var-substitutions.md

+Get the total time the user has been enagaged with the page since the page was
+loaded.
+
+For instance:


Maybe we don't need this example here, since that would always be zero.

britice · 2016-02-19T17:07:10Z

Ready for another pass. All PR comments are addressed in the first new commit. There were a few other minor changes in the second two commits. I'm planning to squash everything into one commit when we are satisfied with the updates. If it is preferred that I squash prior to another review, let me know.

britice · 2016-02-19T17:08:21Z

Ooops, hit "Close and Comment" instead of "comment".

rudygalfi · 2016-02-19T17:08:30Z

@britice It seems like this is feature complete? Can you confirm?

britice · 2016-02-19T17:09:38Z

It seems like this is feature complete? Can you confirm?

Yes, this is complete.

cramforce · 2016-02-19T17:20:43Z

Please disallow unwhitelisted usage of installActivityService in presubmit-checks.js.

Would be great if @avimehta could take a look!

avimehta · 2016-02-19T17:26:22Z

looking at it now. will respond in a couple of hours.

avimehta · 2016-02-19T19:46:26Z

src/service/activity-impl.js

+    this.unlistenFuncs_ = [];
+  }
+
+  cleanup_() {


curious about how this gets called. Also, docs for this?

It's not being called at this point. I saw cleanup_ functionality in other components and decided to set this up here as well. Since the activity tracking is intended to be available until the user leaves the page, it didn't seem necessary to ensure it was called anywhere.

I'll add the annotations for all the methods.

avimehta · 2016-02-19T19:59:48Z

@cramforce overall the PR looks good to me. Since I am not an expert at the platform stuff, I'll defer to your lgtm.

cramforce · 2016-02-19T20:06:47Z

src/activity.js

+ * @param {!Window} window
+ * @return {!Promise<?Activity>}
+ */
+export function activityForOrNull(win) {


You can delete this as it is never used.

cramforce · 2016-02-19T20:07:26Z

LGTM. One comment. Please also squash commits.

britice · 2016-02-19T20:24:08Z

Added another commit to address a few more PR comments. I'll squash and force push momentarily.

britice · 2016-02-19T20:38:24Z

This is ready when checks pass.

britice · 2016-02-19T22:20:01Z

Looks like this has gotten fairly far behind master. Let me know if I should rebase prior to merging.

emmettbutler · 2016-02-20T00:05:27Z

This method of tracking engagement is a bit different from what we're doing at Parse.ly, but it all makes sense to me.

rudygalfi · 2016-02-20T00:14:27Z

@emmett9001 Thanks for taking a look! Do you think Parsely would be able to use this implementation for AMP? Main goal is to make sure we have an implementation that's reasonable and works for all.

Would you be good with merging this (and we can enhance further in the future)?

britice · 2016-02-22T17:07:26Z

I brought this up to date with master.

@rudygalfi @emmett9001 Is there any other input from Parsley that needs to be considered before this is merged?

A previous update to Chartbeat's vendor config (#1986) is somewhat dependent on this PR so we'd like to see them combined for the next release.

…ervice

emmettbutler · 2016-02-22T17:41:46Z

@britice @rudygalfi This looks close enough to what Pasre.ly's currently doing that I think it's fine to merge. There may be some small changes I'm not noticing now that come up later, but I don't think any of them will require a major rewrite.

Add Activity service to calculate engaged time for amp-analytics

cramforce assigned avimehta Feb 5, 2016

rudygalfi reviewed Feb 9, 2016
View reviewed changes

breauxc mentioned this pull request Feb 12, 2016

Update Chartbeat vendor config #1986

Merged

britice mentioned this pull request Feb 17, 2016

Get viewer from this.win rather than global window object #2080

Merged

britice force-pushed the engaged-time branch from b169868 to 5d5dedd Compare February 17, 2016 23:38

britice force-pushed the engaged-time branch from 5d5dedd to 1a77326 Compare February 18, 2016 22:39

cramforce reviewed Feb 18, 2016
View reviewed changes

britice force-pushed the engaged-time branch from cba1ca7 to 866a1bb Compare February 19, 2016 16:59

britice closed this Feb 19, 2016

britice reopened this Feb 19, 2016

rudygalfi added the NEEDS REVIEW label Feb 19, 2016

avimehta reviewed Feb 19, 2016
View reviewed changes

cramforce reviewed Feb 19, 2016
View reviewed changes

cramforce added LGTM and removed NEEDS REVIEW labels Feb 19, 2016

britice force-pushed the engaged-time branch from 0c0b031 to 1e00784 Compare February 19, 2016 20:37

britice force-pushed the engaged-time branch from 1e00784 to aa9d994 Compare February 22, 2016 16:59

Provide engaged time metric in url replacement through new Activity s…

0db3ddf

…ervice

britice force-pushed the engaged-time branch from aa9d994 to 0db3ddf Compare February 22, 2016 17:27

rudygalfi added a commit that referenced this pull request Feb 22, 2016

Merge pull request #1818 from britice/engaged-time

388a3d6

Add Activity service to calculate engaged time for amp-analytics

rudygalfi merged commit 388a3d6 into ampproject:master Feb 22, 2016

britice deleted the engaged-time branch February 22, 2016 18:42

britice mentioned this pull request Feb 23, 2016

Scrolling does not contribute to engaged time on mobile devices #2226

Closed

rudygalfi mentioned this pull request Mar 4, 2016

Add active engaged time heuristics for amp-analytics #1296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Activity service to calculate engaged time for amp-analytics #1818

Add Activity service to calculate engaged time for amp-analytics #1818

britice commented Feb 5, 2016

britice commented Feb 5, 2016

cramforce commented Feb 5, 2016

britice commented Feb 5, 2016

rudygalfi Feb 9, 2016

etlgfx Feb 9, 2016

avimehta Feb 19, 2016

rudygalfi commented Feb 9, 2016

etlgfx commented Feb 9, 2016

britice commented Feb 9, 2016

britice commented Feb 11, 2016

rudygalfi commented Feb 13, 2016

britice commented Feb 16, 2016

britice commented Feb 17, 2016

britice commented Feb 18, 2016

cramforce Feb 18, 2016

britice commented Feb 19, 2016

britice commented Feb 19, 2016

rudygalfi commented Feb 19, 2016

britice commented Feb 19, 2016

cramforce commented Feb 19, 2016

avimehta commented Feb 19, 2016

avimehta Feb 19, 2016

britice Feb 19, 2016

avimehta commented Feb 19, 2016

cramforce Feb 19, 2016

cramforce commented Feb 19, 2016

britice commented Feb 19, 2016

britice commented Feb 19, 2016

britice commented Feb 19, 2016

emmettbutler commented Feb 20, 2016

rudygalfi commented Feb 20, 2016

britice commented Feb 22, 2016

emmettbutler commented Feb 22, 2016

Add Activity service to calculate engaged time for amp-analytics #1818

Add Activity service to calculate engaged time for amp-analytics #1818

Conversation

britice commented Feb 5, 2016

britice commented Feb 5, 2016

cramforce commented Feb 5, 2016

britice commented Feb 5, 2016

rudygalfi Feb 9, 2016

Choose a reason for hiding this comment

etlgfx Feb 9, 2016

Choose a reason for hiding this comment

avimehta Feb 19, 2016

Choose a reason for hiding this comment

rudygalfi commented Feb 9, 2016

etlgfx commented Feb 9, 2016

britice commented Feb 9, 2016

britice commented Feb 11, 2016

rudygalfi commented Feb 13, 2016

britice commented Feb 16, 2016

britice commented Feb 17, 2016

britice commented Feb 18, 2016

cramforce Feb 18, 2016

Choose a reason for hiding this comment

britice commented Feb 19, 2016

britice commented Feb 19, 2016

rudygalfi commented Feb 19, 2016

britice commented Feb 19, 2016

cramforce commented Feb 19, 2016

avimehta commented Feb 19, 2016

avimehta Feb 19, 2016

Choose a reason for hiding this comment

britice Feb 19, 2016

Choose a reason for hiding this comment

avimehta commented Feb 19, 2016

cramforce Feb 19, 2016

Choose a reason for hiding this comment

cramforce commented Feb 19, 2016

britice commented Feb 19, 2016

britice commented Feb 19, 2016

britice commented Feb 19, 2016

emmettbutler commented Feb 20, 2016

rudygalfi commented Feb 20, 2016

britice commented Feb 22, 2016

emmettbutler commented Feb 22, 2016