Track app usage #25

joelanman · 2018-02-07T18:40:01Z

UPDATE

I had misunderstood custom dimensions, and we should use them to track operating system, node version, prototype kit version

https://docs.google.com/document/d/13or0ac7H8TdfWfMSt8xnsGgOuTV8jp7CuEtPa6qcP9U/edit#

This work adds opt-in tracking of app usage, using Google Analytics.

It asks for user's permission when first run.

The app then records the user's answer in usage-data-config.js.

If the answer was yes:

It also records a unique user ID in the same file.

It sends an Event to Google Analytics, with:

Category: 'Start'
Action: the version of the prototype kit
Value: the operating system type (eg Windows) and version

I've refactored start.js slightly into a series of functions, this makes it a lot easier to handle the correct flow, based on whether the user is opted in or out.

Known issues

Not tracking Node version
The prompt is a bit ugly and needs @amyhupe to check the text
I think we need a documentation page in the kit to tell people exactly what usage data is recorded, possibly linked to from the prompt

Screens

When recorded in GA, the event looks like this:

NickColley

Left some comments around how we can try to make sure our users know what they're opting into.

I think this file should be restructured since there's so much going on.

Probably something like a 'start' directory with the chunks of logic in separate file with the start.js file orchestrating the chunks?

This is assuming we'd want to port this to the live prototype kit, if this is only for private beta nevermind.

NickColley · 2018-02-08T10:42:17Z

start.js

+  prompt.message = ''
+  prompt.delimiter = ''
+
+  prompt.get([{


What happens if someone does not read this and presses enter, which one is the default?

Normally CLI applications will do something like

'(y/N)' where the capitalised answer is what would be sent when pressing enter.

there's no default. It says 'Please enter y or n' - this is the same as when we prompt for a new port number if a kit is already running

NickColley · 2018-02-08T10:42:56Z

start.js

+
+  prompt.get([{
+    name: 'answer',
+    description: 'Help us improve the Prototpe Kit\nDo you give permission for the kit to record anonymous usage data? (y/n)',


Should we link to some form of privacy policy that:

details our strategy for tracking e.g. using Google Analytics

what details we are capturing

how to opt-out if you've opted-in?

hannalaakso

I think we should specify what data we are collecting. According to npm, it's "Pageviews, Screenviews, Events, Exceptions, User timings, Transactions"

I wonder if once the user has accepted tracking, the notification that they're being tracked should be shown in the console every time they run the kit, instead of being shown just the once when they accept? It would make it a bit more visible.

I agree with @NickColley that there's a lot going on in start.js and the functionality should be structured into smaller chunks. But if this is just proof of concept then it's cool.

hannalaakso · 2018-02-12T12:57:28Z

start.js

+  trackingUser.set('uip','1.1.1.1')
+
+  const kitVersion = packageJson.version
+  const operatingSystem = os.platform() + ' ' + os.release()


Here, it would be good to check that os is not null before getting platform and release. If querying for os fails for any reason and it's null, this throws up a fatal error.

not sure why os would ever be null? It's a core Node module that's required further up in the file - we don't normally check those are null

Spoke to @joelanman. I like to check for and catch null variables whenever they might arise. However I recognise in this case that's excessive since something's gone fundamentally wrong if os returns null. Added a ticket to Dev Time as it would be good to understand how other team members handle errors and then add something to docs.

hannalaakso · 2018-02-12T13:03:53Z

start.js

+  startTracking()
+} else {
+  // opted out
+  checkFiles()


It looks like checkFiles() always runs regardless of the fork here (both askForUsageDataPermission() and startTracking() contain it) so maybe it could come after the if/else if block and the else condition can be omitted?

we need to look for three states here and manage each one separately:

undefined - we don't have a record of permission either way, and we need to ask
true - we have permission and we should startTracking
else (false) - we don't have permission and we skip over tracking, straight to checkFiles

Thanks @joelanman. I'm not sure the state matters in any way since checkFiles() always runs and we don't pass any arguments to it. Maybe I'm missing something 🤔

Thanks @joelanman for talking me through this 🙂As askForUsageDataPermission() contains an asynchronous call, running checkFiles() which starts the kit is not the right thing to do.

There might be some opportunities here to refactor the functions, maybe by creating a new function startKit() that calls both checkFiles() and runGulp() to make it clearer what's happening. And doing:

if (usageDataConfig.collectUsageData === undefined){ askForUsageDataPermission() } else { if (usageDataConfig.collectUsageData === true) { // opted in startTracking() } // run the kit checkFiles() }

But that's just extra niceness and shouldn't block the PR from being merged.

hannalaakso · 2018-02-12T14:12:56Z

start.js

+
+  const description = fs.readFileSync(path.join(__dirname, 'lib/usage-data-prompt.txt'), 'utf8')
+
+  prompt.get([{


Could this only happen if description is not null?

why would description be null?

Same as above - let's discuss error handling with the team.

joelanman · 2018-02-13T15:11:31Z

@hannalaakso The data recorded is documented here: https://github.com/alphagov/govuk-prototype-kit-private-beta/blob/d8b51e8a92fce5a231370ddf1c9b6f0f995c385e/docs/documentation/usage-data.md

Nice point about reminding people every start, I'll look into that.

joelanman · 2018-02-13T18:59:29Z

On reviewing this with @alex-ju, I'm not sure the Google Analytics approach can work.

We want to know for example, what % of our users are on Windows. GA does this with Dimensions, which I don't think we can use (custom dimensions are managed by hand, and only have 200 different values).

GA Events (the approach taken in this PR) are not reported on as above. They are reported on as Total events, or Unique events. Either one one show a user starting the kit twice on Windows to have 2 events, and a user starting once on OSX to have 1. This does not give us the data we need.

NickColley · 2018-02-21T11:25:48Z

Would this stop heroku from deploying the prototype kit since it could get stuck on this question?

joelanman · 2018-02-21T12:06:09Z

@NickColley it's a good question, but Heroku doesn't run npm start - it runs procfile instead

alex-ju · 2018-02-28T17:05:15Z

Updated with using custom dimensions for storing persistent data (Operating System, Prototype Kit Version and Node Version).

joelanman · 2018-02-28T17:14:48Z

start.js

+  const nodeVersion = process.versions.node
+
+  // Anonymise the IP
+  trackingUser.set('anonymizeIp', 1)


Do you think it's better to anonymise at our end, rather than asking Google to do it?

Depending on what we want to achieve by anonymising. By setting an IP ourselves we'll introduce an artificial location but we're sure the IP doesn't go to GA. If we anonymise IP we're not storing the IP but we get some insights that come with that (e.g. location). It's worth having a chat about that.

joelanman · 2018-02-28T17:15:05Z

start.js

+  trackingUser.set('cd3', nodeVersion)
+
+  // Trigger start event
+  trackingUser.event('State', 'Start', 'Application started').send()


Do we need 'Application started'?

Well.. we need a label.. I'm not sure what's the best copy for it though.
The format is trackingUser.event(Category, Action, Label).send()

This page says label is not required
https://developers.google.com/analytics/devguides/collection/analyticsjs/events

That's true, not required. So if it's not helpful for us in the GA interface, I'll remove it :)

alex-ju · 2018-03-06T11:27:00Z

Updated with using custom dimensions for storing persistent data (Operating System, Prototype Kit Version and Node Version).

Refactored the code. @NickColley can you take a look when you have time (no pressure)?

NickColley · 2018-03-09T10:43:48Z

docs/documentation/usage-data.md

@@ -0,0 +1,27 @@
+# Collecting usage data


Excellent work.

Since third-parties are involved a user may consent to this data being collected but not consent to the third-party being involved in the process.

Would be good to be specific how this data is collected with which third-parties.

Added the third-party to the 'How it works' and a bit more details to 'Data we collect'.
usage-data.md

NickColley · 2018-03-09T10:45:47Z

start.js

+
+if (usageDataConfig.collectUsageData === undefined) {
+  // didn't opted
+  let promptPromise = new Promise(usageData.askForUsageDataPermission)


Would this block a heroku server from starting?

As @joelanman pointed out in a comment above it seems like Heroku relies on node ./node_modules/gulp/bin/gulp generate-assets && node server.js and all our tracking work is in start.js, then start.js uses Gulp to call server.js via nodemon.
Short answer, no, it shouldn't.

NickColley

Looking better 👍

One question about blocking heroku and specifying how we will track users and some non-blocking comments.

NickColley · 2018-03-09T10:47:03Z

start.js

+
+if (usageDataConfig.collectUsageData === undefined) {
+  // didn't opted
+  let promptPromise = new Promise(usageData.askForUsageDataPermission)


Non-blocking: I'd expect this 'new Promise' to live inside 'askForUsageDataPermission'

Something like

let promptPromise = usageData.askForUsageDataPermission()

Updated, moved the promise inside the helper function.

alex-ju · 2018-03-12T10:50:34Z

@NickColley thanks for the review. tried to address your comments. please let me know if you feel it's good to go or you have any concerns.

NickColley · 2018-03-12T11:34:54Z

@alex-ju looks great 👍

joelanman · 2018-03-12T12:00:47Z

Please don't merge while it has my test GA code on it :)

alex-ju · 2018-03-12T13:12:02Z

@joelanman I'm aware. We can update the tracking code tomorrow. It's up to you when's the right time to merge it, since you opened this PR, just trying to make we have it prepared.

alex-ju · 2018-03-27T08:46:06Z

@joelanman updated the tracking code. Looking at the Real-time events seems to be tracking well. I'll check again in a few minutes in the Event section to make sure they're stored correctly.

Updated: records well! good to be merged from my side

joelanman · 2018-03-28T12:03:15Z

start.js

-  console.error('ERROR: Node module folder missing. Try running `npm install`')
-  process.exit(0)
+// Local dependencies
+const usageData = require('./lib/usage_data')


This still breaks the npm install check, as usage_data then requires modules.

I'd recommend calling checkFiles before this, outside of the if block below (all the cases call it, so it's good to refactor this anyway)

joelanman · 2018-03-28T12:15:04Z

start.js

+const usageDataConfig = usageData.getUsageDataConfig()
+
+if (usageDataConfig.collectUsageData === undefined) {
+  // didn't opted


this isn't right - suggest
// No recorded answer, so ask for permission

joelanman · 2018-03-28T12:16:47Z

start.js

+  // opted in
+  checkFiles()
+  runGulp()
+  usageData.startTracking(usageDataConfig)


I think maybe we should startTracking before running gulp, makes more sense that way in my mind, but not sure it matters?

alex-ju · 2018-03-28T13:06:04Z

@joelanman thanks for the review! updated.

joelanman · 2018-03-28T13:35:22Z

@alex-ju it looks like checkFiles still happens after the require I highlighted?

edwardhorsford · 2018-04-18T11:49:39Z

docs/documentation/usage-data.md

+This helps the team working on the Kit understand how it's being used, in order
+to improve it - please don't turn off usage data unless you have to.
+
+## How it works


Would be good to mention if this just happens locally, or if it will also happen on deployed versions.

edwardhorsford · 2018-04-18T11:58:42Z

It might be worth storing the date the user accepted the opt-in. Might be useful if you ever want to decide whether to send a new opt-in request.

joelanman · 2018-04-18T12:03:00Z

@edwardhorsford I think thats a good idea, but I think we can add it later. For now the file created date should be ok. It raises the question of whether the 'permission proof' living with the user is sufficient. If they ever lost it there would be no proof. I wonder whether in this case we could prove that data is only sent if a user explicitly opts in.

alex-ju · 2018-04-18T12:49:29Z

@NickColley, can you give us a hand with the final review? I'm aware you already approved, but there were some small changes since then.

NickColley

Gave it a once over and looks good 👍

joelanman requested a review from NickColley February 7, 2018 18:42

NickColley suggested changes Feb 8, 2018

View reviewed changes

hannalaakso reviewed Feb 12, 2018

View reviewed changes

joelanman commented Feb 28, 2018

View reviewed changes

alex-ju force-pushed the track-app branch from 3eec4cd to 81a9e21 Compare March 1, 2018 09:48

NickColley reviewed Mar 9, 2018

View reviewed changes

NickColley approved these changes Mar 9, 2018

View reviewed changes

alex-ju force-pushed the track-app branch from 877f1ac to ecbd0d5 Compare March 12, 2018 10:38

alex-ju changed the title ~~DO NOT MERGE Track app usage~~ Track app usage Mar 12, 2018

alex-ju force-pushed the track-app branch from ecbd0d5 to f19b864 Compare March 12, 2018 10:50

alex-ju force-pushed the track-app branch 2 times, most recently from 721a653 to e9349c6 Compare March 27, 2018 08:39

alex-ju force-pushed the track-app branch 2 times, most recently from 365a7c7 to 24d2b33 Compare March 28, 2018 10:56

joelanman commented Mar 28, 2018

View reviewed changes

alex-ju force-pushed the track-app branch from 24d2b33 to 891405d Compare March 28, 2018 13:05

alex-ju force-pushed the track-app branch from 891405d to 5f422e0 Compare April 12, 2018 09:18

edwardhorsford reviewed Apr 18, 2018

View reviewed changes

joelanman and others added 5 commits April 18, 2018 13:18

Add universal-analytics npm module

3612760

Add uuid npm module

2ec6744

Setup usage tracking and add documentation

6ff2abe

Remove .env message

f042135

update docs to say it won't send data from Heroku

0276cd9

joelanman force-pushed the track-app branch from 1ffed91 to 0276cd9 Compare April 18, 2018 12:18

update CHANGELOG

7c1532e

NickColley approved these changes Apr 18, 2018

View reviewed changes

alex-ju merged commit 81f6d9d into master Apr 18, 2018

alex-ju deleted the track-app branch April 18, 2018 13:16

kr8n3r mentioned this pull request May 18, 2018

Release 7.0.0-beta.6 #43

Merged

This was referenced Jun 25, 2018

7.0.0: Track app usage alphagov/govuk-prototype-kit#515

Closed

Track app alphagov/govuk-prototype-kit#523

Merged


		const description = fs.readFileSync(path.join(__dirname, 'lib/usage-data-prompt.txt'), 'utf8')

		prompt.get([{

Track app usage #25

Track app usage #25

Conversation

joelanman commented Feb 7, 2018 • edited Loading

Known issues

Screens

NickColley 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

hannalaakso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanman Feb 13, 2018 • edited Loading

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

joelanman commented Feb 13, 2018

joelanman commented Feb 13, 2018

NickColley commented Feb 21, 2018

joelanman commented Feb 21, 2018

alex-ju commented Feb 28, 2018

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

alex-ju commented Mar 6, 2018 • edited Loading

NickColley Mar 9, 2018 • edited Loading

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

NickColley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju commented Mar 12, 2018

NickColley commented Mar 12, 2018

joelanman commented Mar 12, 2018

alex-ju commented Mar 12, 2018

alex-ju commented Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju commented Mar 28, 2018

joelanman commented Mar 28, 2018

Choose a reason for hiding this comment

edwardhorsford commented Apr 18, 2018

joelanman commented Apr 18, 2018

alex-ju commented Apr 18, 2018

NickColley left a comment

Choose a reason for hiding this comment

joelanman commented Feb 7, 2018 •

edited

Loading

hannalaakso left a comment •

edited

Loading

joelanman Feb 13, 2018 •

edited

Loading

alex-ju commented Mar 6, 2018 •

edited

Loading

NickColley Mar 9, 2018 •

edited

Loading

NickColley left a comment •

edited

Loading

alex-ju commented Mar 27, 2018 •

edited

Loading