Add Google Analytics #617

gregtatum · 2017-10-17T22:37:42Z

#615 needs to land first.

I opted to go for pageviews on the tabs, as then we can easily gather timing information through the GA reports. I also added instrumentation to timeCode, so that we can get that information and analyze it over a broader audience.

I tried to break out the changes into different commits in case people wanted to look at them individually for feedback.

codecov · 2017-10-17T23:08:21Z

Codecov Report

Merging #617 into master will decrease coverage by 0.11%.
The diff coverage is 41.79%.

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
- Coverage   62.81%   62.69%   -0.12%     
==========================================
  Files         130      131       +1     
  Lines        7296     7345      +49     
  Branches     1616     1623       +7     
==========================================
+ Hits         4583     4605      +22     
- Misses       2390     2411      +21     
- Partials      323      329       +6

Impacted Files	Coverage Δ
src/components/app/UrlManager.js	`0% <0%> (ø)`	⬆️
src/components/app/ProfileSharing.js	`35.89% <0%> (-1.95%)`	⬇️
...omponents/header/ProfileThreadHeaderContextMenu.js	`0% <0%> (ø)`	⬆️
src/index.js	`0% <0%> (ø)`	⬆️
src/actions/app.js	`0% <0%> (ø)`	⬆️
src/actions/profile-view.js	`76.81% <100%> (+8.18%)`	⬆️
src/utils/time-code.js	`33.33% <33.33%> (-6.67%)`	⬇️
src/utils/analytics.js	`75% <75%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d6b910...ed319e3. Read the comment docs.

gregtatum · 2017-10-26T22:10:56Z

The legal bug for permission to analytics is approved. If anyone has thoughts on my implementation please leave them here. Otherwise, @julienw I would appreciate a code review on this so we can started getting some data about our usage patterns

julienw

There are a few needed changes but I think overall this looks good :)

Thanks !

julienw · 2017-10-27T19:57:40Z

flow-typed/overrides/Window.js

+  timingLabel?: string,
+|};
+
+type GoogleAnalytics = ('send', GAEvent | GAPageView | GATiming) => {};


Any reason you declare all this here and not in a separate types file ? Like src/types/analytics.js ?
Then you can import it here yo use it below.

julienw · 2017-10-27T19:57:40Z

server.js

@@ -25,7 +25,8 @@ new WebpackDevServer(webpack(config), {
        'self'
        'sha256-eRTCQnd2fhPykpATDzCv4gdVk/EOdDq+6yzFXaWgGEw='
        'sha256-AdiT28wTL5FNaRVHWQVFC0ic3E20Gu4/PiC9xukS9+E='
-        https://api-ssl.bitly.com;
+        https://api-ssl.bitly.com
+        https://www.google-analytics.com;


Not sure this is needed given we load it in perf.html only ? Though it can be useful while testing.

Maybe not technically, but it does make it easier to verify they are the same.

julienw · 2017-10-27T19:57:40Z

res/analytics.js

+  // Only include on perf-html.io
+  window.location.host.includes('perf-html.io') &&
+  // Observe do not track.
+  !(navigator.doNotTrack || window.doNotTrack || navigator.msDoNotTrack)


doNotTrack values are not boolean. You want to check if the value is "1", maybe something like:

const doNotTrack = (navigator.doNotTrack || window.doNotTrack || navigator.msDoNotTrack) === '1'; if (!doNotTrack) { ... }

Yikes, what an API. Fixing it.

julienw · 2017-10-27T19:57:40Z

res/analytics.js

+  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
+  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
+  })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
+  ga('create', 'UA-35433268-81', 'auto');


wondering if this shouldn't be hidden from our source, and instead we should have a conf file for building on the server ?

I assume this is our ID.

This is our ID, although it's always publicly visible. I'm not sure it's worth the complexity of adding environment keys to our build step yet.

julienw · 2017-10-27T19:57:40Z

res/index.html

@@ -6,6 +6,7 @@
 <head>
 <meta charset="utf-8">
 <title>perf.html</title>
+<script src='analytics.js'></script>


Should you add it in webpack's OfflinePlugin configuration too ?

Actually, you need /analytics.js instead of analytics.js

And you need to copy it to dist.

julienw · 2017-10-27T19:57:40Z

src/actions/profile-view.js

+      ga('send', {
+        hitType: 'event',
+        eventCategory: 'profile',
+        eventAction: 'change hide platform details',


not sure this is the right action string ;) "change implementation filter" would be more appropriate, right ?

Nice catch, thanks!

julienw · 2017-10-27T19:57:40Z

src/components/app/ProfileSharing.js

+            eventCategory: 'profile upload',
+            eventAction: 'failed',
+          });
+        }


I'd create an action (I mean, an action that wouldn't dispatch a redux action) to handle ga calls.

or at least a local function.

I'm curious if the reason you want me to create local functions is the if check. Would you be ok with me stubbing out the analytics function for everywhere? That way we can fire it regardless. The interface itself is just a function call with some parameters, so I don't feel the need to abstract it away further.

julienw · 2017-10-27T19:57:40Z

src/components/app/UrlManager.js

+    updateUrlState,
+    urlSetupDone,
+    show404,
+  }


👍
it always disturbed me

julienw · 2017-10-27T19:57:40Z

src/utils/time-code.js

+
+    // Some portion of users will have timing information sent. Limit this further to
+    // only send a few labels per user.
+    const ga = self.ga;


yet another way of getting the ga function ? :)

window doesn't exist in workers.

Ok, makes sense !

julienw · 2017-10-27T19:57:40Z

src/utils/time-code.js

+    // Some portion of users will have timing information sent. Limit this further to
+    // only send a few labels per user.
+    const ga = self.ga;
+    if (ga && !(_timingsPerLabel[label] > MAX_TIMINGS_PER_LABEL)) {


I think you want _timingsPerLabel[label] < MAX_TIMINGS_PER_LABEL (I mean, remove the ! of course, and use < instead of the current <= because otherwise it happens once too many)

Thanks, not sure why I had it that way.

digitarald

Some general comments and one point on how we track page views

digitarald · 2017-10-30T20:48:58Z

res/analytics.js

+  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
+  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
+  })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
+  ga('create', 'UA-35433268-81', 'auto');


to be more specific, auto could be also perf-html.io to tracking on other domains; this way the check can be removed from the top-level if

I personally like the top level if, since it won't even ping google while I'm working locally, and have spotty internet.

digitarald · 2017-10-30T20:48:58Z

res/analytics.js

+  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
+  })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
+  ga('create', 'UA-35433268-81', 'auto');
+  ga('send', 'pageview');


This should probably not send the current URL but the selectedTab from the profile viewer; otherwise the metrics will be a mix of paths and selected tabs

Seems like that is done in https://github.com/devtools-html/perf.html/pull/617/files#diff-0bc320ad3ce513416aecd6152b478911R56 so this line is probably obsolete

Yes, that was a mistake leaving that in there, thanks for catching it!

digitarald · 2017-10-30T20:48:58Z

src/actions/profile-view.js

+      ga('send', {
+        hitType: 'event',
+        eventCategory: 'profile',
+        eventAction: 'toggle thread',


Could we use hide for the action and the type (content, main, compositor, etc) for the label? This way we know which threads most users hide (if users actually hide threads, might need to be more visible) and might pro-actively show less of them

digitarald · 2017-10-30T20:48:58Z

src/actions/app.js

+      const dataSource = getDataSource(getState());
+      ga('send', {
+        hitType: 'pageview',
+        page: dataSource === 'none' ? 'home' : getSelectedTab(getState()),


What would be interesting here where the user came from. First idea that comes to mind would be adding an event to when the profile is loaded either from whatever mechanism is triggered.

ga('send', { hitType: 'event', eventCategory: 'datasource', eventAction: dataSource, });

julienw

Hey, thanks for the changes, this is much more readable !

There are 2 subtle bugs left:

one related to the doNotTrack flag
another with the MAX_TIMINGS_PER_LABEL logic

please manually type ga in the sendAnalytics function.

julienw · 2017-11-06T14:34:09Z

res/analytics.js

+  // Only include on perf-html.io
+  window.location.host.includes('perf-html.io') &&
+  // Observe do not track.
+  (navigator.doNotTrack || window.doNotTrack || navigator.msDoNotTrack) === '1'


don't you want the opposite ? ;)
It's '1' if the user actually specified "I don't want to be tracked" ;)

That's why my suggested code was:

const doNotTrack = (navigator.doNotTrack || window.doNotTrack || navigator.msDoNotTrack) === '1'; if (!doNotTrack) { ... }

(note the ! :) )

julienw · 2017-11-06T14:34:09Z

src/utils/time-code.js

+
+    // Some portion of users will have timing information sent. Limit this further to
+    // only send a few labels per user.
+    if (_timingsPerLabel[label] <= MAX_TIMINGS_PER_LABEL) {


When _timingsPerLabel[label] is undefined (the first time then), I think the result of this comparison is always false.

I think you need to set it to a temporary const before the condition, something like this:

const sentTimings = _timingsPerLabel[label] || 0; if (sentTimings < MAX_TIMINGS_PER_LABEL) { _timingsPerLabel[label] = sentTimings + 1; ... }

Also note the operator < instead of <=. Otherwise I think it's sent (MAX_TIMINGS_PER_LABEL + 1) times.

Thanks for catching this.

julienw · 2017-11-06T14:34:09Z

src/utils/analytics.js

+export type GoogleAnalytics = ('send', GAPayload) => {};
+
+export function sendAnalytics(payload: GAPayload) {
+  const { ga } = self;


I think there is something wrong after latest Flow upgrade, the typing for window properties seems to be broken.
In the mean time, could you type ga manually here ?

This must be because self is not equivalent to window.

gregtatum · 2017-11-10T17:59:54Z

@julienw this is ready for re-review.

julienw · 2017-11-14T17:04:40Z

res/index.html

@@ -6,6 +6,7 @@
 <head>
 <meta charset="utf-8">
 <title>perf.html</title>
+<script src='analytics.js'></script>


Actually, you need /analytics.js instead of analytics.js

julienw · 2017-11-14T17:04:40Z

res/index.html

@@ -6,6 +6,7 @@
 <head>
 <meta charset="utf-8">
 <title>perf.html</title>
+<script src='analytics.js'></script>


And you need to copy it to dist.

julienw · 2017-11-14T17:04:40Z

res/analytics.js

@@ -9,7 +9,7 @@
    (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
    (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
    m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
-    })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
+    })(window,document,'script','https://www.google-analytics.com/analytics.js','ga');


without this change, CSP blocks the "http" version in the dev environment. 2 possible fixes: either this, or adding the "http" version to the CSP header -- which redirects to https anyway. So I think it's better to always ask for the https version, in this case.

julienw · 2017-11-14T17:04:40Z

res/index.html

@@ -6,7 +6,7 @@
 <head>
 <meta charset="utf-8">
 <title>perf.html</title>
-<script src='analytics.js'></script>
+<script src='/analytics.js'></script>


without this change, when on an URL like http://localhost:4242/public/d3fd7e63272e130fdc3ae54b861e8586f5a0d385/calltree/, this looks for http://localhost:4242/public/d3fd7e63272e130fdc3ae54b861e8586f5a0d385/calltree/analytics.js ;)

julienw · 2017-11-14T17:47:04Z

Note I think we should do the actions from #473 before landing @gregtatum @digitarald. Or at least before deploying.

julienw · 2017-11-17T15:48:26Z

@gregtatum if you're OK with my last commit, please merge :)

gregtatum · 2017-11-17T16:10:32Z

The review is still in the state "changes requested" but I'm using my admin privilege to go ahead and merge based on your written approval.

gregtatum requested review from digitarald, julienw and mstange Oct 17, 2017

julienw requested changes Oct 27, 2017

View changes

digitarald suggested changes Oct 30, 2017

View changes

gregtatum force-pushed the gregtatum:google-analytics branch 2 times, most recently from a7534a5 to 0b5afa6 Nov 2, 2017

julienw requested changes Nov 6, 2017

View changes

gregtatum force-pushed the gregtatum:google-analytics branch from 0b5afa6 to 902f850 Nov 10, 2017

julienw mentioned this pull request Nov 14, 2017

Analytics for understanding UI patterns and general usage numbers #473

Closed

4 of 4 tasks complete

gregtatum and others added 7 commits Oct 17, 2017

Add Google Analytics

dcec6f9

Add page and event tracking

261cccc

Add timing analytics

cf37f3e

Address review

36e10ec

Pull out ga() calls into utils

a1a6ee8

Address review

cb01c9e

Fix loading of Google Analytics

2f89cad

julienw force-pushed the gregtatum:google-analytics branch from 902f850 to 2f89cad Nov 14, 2017

julienw reviewed Nov 14, 2017

View changes

digitarald approved these changes Nov 15, 2017

View changes

Merge branch 'master' into google-analytics

ed319e3

firefox-devtools / profiler

Add Google Analytics #617

Add Google Analytics #617

Conversation

gregtatum commented Oct 17, 2017

codecov bot commented Oct 17, 2017 • edited

Codecov Report

gregtatum commented Oct 26, 2017

julienw left a comment

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

julienw Nov 14, 2017 Contributor

This comment has been minimized.

julienw Nov 14, 2017 Contributor

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Oct 27, 2017 Author Member

This comment has been minimized.

julienw Oct 28, 2017 Contributor

This comment has been minimized.

julienw Oct 27, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

digitarald left a comment

This comment has been minimized.

digitarald Oct 30, 2017 Contributor

This comment has been minimized.

gregtatum Nov 2, 2017 Author Member

This comment has been minimized.

digitarald Oct 30, 2017 Contributor

This comment has been minimized.

digitarald Oct 30, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

This comment has been minimized.

digitarald Oct 30, 2017 Contributor

This comment has been minimized.

digitarald Oct 30, 2017 Contributor

This comment has been minimized.

gregtatum Nov 1, 2017 Author Member

julienw left a comment

This comment has been minimized.

julienw Nov 6, 2017 Contributor

This comment has been minimized.

julienw Nov 6, 2017 Contributor

This comment has been minimized.

gregtatum Nov 10, 2017 Author Member

This comment has been minimized.

julienw Nov 6, 2017 Contributor

This comment has been minimized.

codecov bot commented Oct 17, 2017 •

edited

julienw Oct 27, 2017
Contributor

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Oct 27, 2017
Contributor

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Nov 14, 2017
Contributor

julienw Nov 14, 2017
Contributor

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Oct 27, 2017
Contributor

julienw Oct 27, 2017
Contributor

gregtatum Oct 27, 2017
Author Member

julienw Oct 28, 2017
Contributor

julienw Oct 27, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

digitarald Oct 30, 2017
Contributor

gregtatum Nov 2, 2017
Author Member

digitarald Oct 30, 2017
Contributor

digitarald Oct 30, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

digitarald Oct 30, 2017
Contributor

digitarald Oct 30, 2017
Contributor

gregtatum Nov 1, 2017
Author Member

julienw Nov 6, 2017
Contributor

julienw Nov 6, 2017
Contributor

gregtatum Nov 10, 2017
Author Member

julienw Nov 6, 2017
Contributor

gregtatum Nov 10, 2017
Author Member

julienw Nov 14, 2017
Contributor

julienw Nov 14, 2017
Contributor

julienw Nov 14, 2017
Contributor

julienw Nov 14, 2017
Contributor