First big project commit as pull request #4

jaredhirsch · 2017-01-24T06:48:16Z

I've added the whole team as admins on this repo. Comments and questions very welcome.

Take a look at my branch to see the formatted README and API docs: https://github.com/6a68/testpilot-metrics/tree/test

I'll plan to land this EOD Tuesday, unless there are major concerns :-)

pdehaan · 2017-01-24T16:56:20Z

examples/sdk/README.md

+- make it a mobile app, not a website
+- go into 'real-time', then 'events', to see pings trickle in as you click the toolbar button:
+
+![GA dashboard screenshot](https://www.dropbox.com/s/2i0e206q2qwcpx1/Screen%20Shot%202017-01-22%20at%209.35.39%20PM.png?dl=0&raw=true)


Should we add this image to this repo instead of pointing out to Dropboxen?

pdehaan · 2017-01-24T17:07:25Z

package.json

+  "main": "index.js",
+  "scripts": {
+    "test": "./node_modules/.bin/mocha test/*",
+    "build-docs": "./node_modules/.bin/documentation build testpilot-metrics.js -f md -o API.md"


Techically, ./node_modules/.bin/ should be added to the $PATH when using npm run ..., so you should be able to simplify these to mocha test/* and documentation build ... (without the node_modules prefixes).

pdehaan · 2017-01-24T17:48:32Z

examples/webextension/background.js

@@ -0,0 +1,15 @@
+const TRACKING_ID = 'UA-XXXXXXXX-YY';
+
+const { sendEvent } = new Metrics({


I'm not 100% familiar w/ web extension background scripts, but do we need to import Metrics class/object before using it here? Or do we automatically get it in-scope since it's defined in the manifest.json's background.scripts[] array?

Great question. WebExtensions background scripts seem to just get loaded into the auto-generated background page, script tag style (implicit global binding). Trying to require() the file threw an error. ES6 modules might work, but I didn't try that: I didn't want to lock experiment authors into dealing with ES6 in their build scripts.

Yeah, AFAIK, there is no module system built into WebExtensions. We're using rollup & babel to build Snooze Tabs - but it is good not to assume that will be the case.

meandavejustice · 2017-01-24T20:29:51Z

Some experiments (min-vid for example) send extra fields to metrics. We should add another parameter to handle this data. example data

in progress branch to integrate testpilot-metrics on min-vid

meandavejustice · 2017-01-24T20:34:19Z

README.md

+
+const { sendEvent } = new Metrics({
+  id: '@my-addon',
+  cid: 'some-non-PII-user-ID',


It may be nice to add an example of how to get this cid, in the testpilot addon we are currently setting this in a simple-storage store. and sending it along with each metric event.

initialization in exports.main

// set our unique identifier for metrics if (!store.clientUUID) { store.clientUUID = require('sdk/util/uuid').uuid().toString().slice(1, -1); }

sendEvent call

const { sendEvent } = new Metrics({ id: '@'+manifest.name, cid: store.clientUUID });

👍 Thanks for the suggestion. I've gone back and forth on this point; not having node-style module support in WebExtensions makes it really, really hard to find an off-the-shelf library that's easy to use. But I guess I could roll my own, using window.crypto to get pseudorandom data, and resort to the hidden window trick to get the crypto API for SDK/bootstrapped add-ons. I'll add that now

Rather than add a uid generator, I'll just file a bug to document suggestions for generating the user id.

jaredhirsch · 2017-01-25T02:32:47Z

Some experiments (min-vid for example) send extra fields to metrics. We should add another parameter to handle this data.

Good point. The simplest solution here seems to be: let experiments pass extra key/value pairs in the object. This works fine for Firefox client metrics, but GA doesn't support random extra named parameters.

GA does support custom dimensions, though they are just named cd1 up to cd200. This seems like a rather fancy use case, compared with "does event/object demonstrate DAU growth".

Maybe the best thing is to let fancy experiment authors solve their own problems: I'll add support for a transform function parameter that is called to transform the ping object into a set of GA parameters ready to be encoded and sent. Adding transform would solve a related issue where testpilot-metrics, currently, only uses the GA event type, while some experiments might want to use other GA hit types. I'll add that tonight.

meandavejustice · 2017-01-25T17:23:39Z

@6a68 That sounds like a good compromise, thanks Jared

ckprice · 2017-01-25T19:52:56Z

testpilot-metrics.js

+    };
+    if (study && variant) {
+      data.xid = study;
+      data.xvar = variant;


You may want to use custom dimensions here as these seem like environment variables which maps well to dimensions I think (technical docs, and more information on dimensions and metrics).

FWIW I think the measurement protocol is definitely the way to go, and it's worked fine in the Test Pilot implementation.

Good call. I didn't realize how involved GA Content Experiments were: https://support.google.com/analytics/answer/1745154?hl=en&ref_topic=1745208. I don't think that's what we want for our A/B tests.

ckprice · 2017-01-25T20:18:49Z

examples/webextension/README.md

+
+## installation
+
+- Edit `background.js`, set the value of the `TRACKING_ID` const to your GA tracking ID


RE: Mozilla ID vs. personal ID

If we're making Firefox-y decisions based on this, maybe Mozilla ID

Mozilla ID adds another dependency on the analytics team (filing a bug)

Personal ID has bus factor/SPOF risks

Personal ID could also reduce overhead on graduation (e.g. don't have to transfer things, although transferring ownership doesn't seem like a huge deal)

Maybe we suggest starting up with personal for quickness, then require a transfer to Mozilla before launch?

👍 I'll add this to the top-level README docs

@ckprice Actually, Wil's metrics PR suggested leaving the author as owner throughout, but granting Mozilla access while it's in Test Pilot

lmorchard · 2017-01-25T21:16:34Z

testpilot-metrics.js

+  _initTransports: function() {
+    if (this.type === 'webextension') {
+      try {
+        this._channel = new BroadcastChannel(this.topic);


Potential problem in WebExtension land here: The BroadcastChannel has to be named testpilot-telemetry, but it looks like it will be named just testpilot because this.topic gets set to that.

doh. great catch, thank you

lmorchard · 2017-01-25T21:20:25Z

testpilot-metrics.js

+  // pings to Telemetry. If the add-on ID matches the test pilot add-on, use
+  // the special nsIObserver topic. Otherwise, use the topic shared by all
+  // experiments.
+  this.topic = (id === '@testpilot-addon') ? 'testpilot' : 'testpilottest';


Maybe change this to:

if (type === 'webextension') { this.topic = 'testpilot-telemetry' } else if (id === '@testpilot-addon') { this.topic = 'testpilot'; } else { this.topic = 'testpilottest'; }

lmorchard · 2017-01-25T21:50:34Z

I'll add support for a transform function parameter that is called to transform the ping object into a set of GA parameters ready to be encoded and sent.

Does that mean, conversely, that whatever object gets passed in to sendEvent() will be sent as-is to Telemetry? (i.e. rather than stripped down to id, event, & object?) If so then 👍

(Also guessing a transform function would resolve my confusion about the GA params too, if I'm assembling them in my own code.)

lmorchard · 2017-01-25T22:03:36Z

testpilot-metrics.js

+      cid: this.cid,
+      t: 'event',
+      ec: category,
+      ea: object,


This mapping seems a little confusing. In the GA docs and in the realtime view, this ea param is called "Event Action" - but it's mapped here from object. And the el is "Event Label" - mapped here from event.

Confused me a little bit, because I was trying to send events like this:

{ event: 'panel-opened', object: 'snooze-tabs' } { event: 'snoozed', object: 'snooze-tabs' } { event: 'woke', object: 'snooze-tabs' }

And all I was getting were snooze-tabs under "Event Action" - and "Event Label" is not even listed on the realtime table.

Thanks for pointing this out. I've switched the code to make the event the 'event action'. That real-time event graph is the place to be.

lmorchard · 2017-01-25T22:10:49Z

FWIW, I've got a hacky start at integrating this into Snooze Tabs. The parameters seem a little confusing to me, but I was able to get some data flowing to GA in the realtime view:

jaredhirsch · 2017-01-26T07:21:46Z

Does that mean, conversely, that whatever object gets passed in to sendEvent() will be sent as-is to Telemetry?

Yup! That's the idea...excluding the transform function, of course ;-)

jaredhirsch · 2017-01-26T07:48:10Z

Made some changes, just about to squash, push, and merge. Just FYI for those who reviewed:

Fixed all the little review comments, thanks for those 👍
Send the entire object to Telemetry
Bunch of annoying little parameter changes:
- Renamed 'cid' to 'uid', after realizing that 'cid' is always a uuidv4, but 'uid' can be whatever
- Removed 'study', just made 'variant' the lone A/B testing parameter, with a new convention of using the first GA Custom Dimension for it. As ckprice pointed out, the GA experiment stuff is really complex and not what we want.
- Add a mandatory 'version' (addon version) parameter to the constructor, and pass it to GA as the app version parameter. This assumes we are using the app GA view, not the web GA view. Documented this assumption in the GA setup steps in the README (which are new).
- Rearrange the GA values corresponding to 'event' and 'object'. Event should be the action, and object the label, not the other way around.
- Added the transform sendEvent parameter, and documented its use.
- Updated the example docs and tests to use the new parameters.
The GA docs specifically mention using %20, not +, to encode spaces, even though the Measurement Protocol uses the x-www-formurlencodedcontent-type, so, switched back to using%20` for spaces.

jaredhirsch force-pushed the test branch from 9da94d2 to 2725aa3 Compare January 24, 2017 06:48

pdehaan reviewed Jan 24, 2017

View reviewed changes

meandavejustice reviewed Jan 24, 2017

View reviewed changes

ckprice reviewed Jan 25, 2017

View reviewed changes

ckprice mentioned this pull request Jan 25, 2017

add metrics overview mozilla/testpilot#1662

Merged

lmorchard reviewed Jan 25, 2017

View reviewed changes

Initial project commit

1d6a8a0

jaredhirsch force-pushed the test branch from 2725aa3 to 1d6a8a0 Compare January 26, 2017 07:48

jaredhirsch merged commit babf04a into mozilla:master Jan 26, 2017

jaredhirsch deleted the test branch January 26, 2017 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First big project commit as pull request #4

First big project commit as pull request #4

jaredhirsch commented Jan 24, 2017 •

edited

pdehaan Jan 24, 2017

jaredhirsch Jan 24, 2017

pdehaan Jan 24, 2017

jaredhirsch Jan 24, 2017

pdehaan Jan 24, 2017

jaredhirsch Jan 24, 2017

lmorchard Jan 24, 2017 •

edited

meandavejustice commented Jan 24, 2017

meandavejustice Jan 24, 2017

jaredhirsch Jan 25, 2017

jaredhirsch Jan 26, 2017

jaredhirsch commented Jan 25, 2017

meandavejustice commented Jan 25, 2017

ckprice Jan 25, 2017

jaredhirsch Jan 25, 2017

ckprice Jan 25, 2017

jaredhirsch Jan 25, 2017

jaredhirsch Jan 25, 2017

lmorchard Jan 25, 2017 •

edited

jaredhirsch Jan 26, 2017

lmorchard Jan 25, 2017 •

edited

jaredhirsch Jan 26, 2017

lmorchard commented Jan 25, 2017 •

edited

lmorchard Jan 25, 2017 •

edited

jaredhirsch Jan 26, 2017

lmorchard commented Jan 25, 2017 •

edited

jaredhirsch commented Jan 26, 2017

jaredhirsch commented Jan 26, 2017

		@@ -0,0 +1,15 @@
		const TRACKING_ID = 'UA-XXXXXXXX-YY';

		const { sendEvent } = new Metrics({


		## installation

		- Edit `background.js`, set the value of the `TRACKING_ID` const to your GA tracking ID

First big project commit as pull request #4

First big project commit as pull request #4

Conversation

jaredhirsch commented Jan 24, 2017 • edited

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

lmorchard Jan 24, 2017 • edited

Choose a reason for hiding this comment

meandavejustice commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredhirsch commented Jan 25, 2017

meandavejustice commented Jan 25, 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

lmorchard Jan 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard Jan 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard commented Jan 25, 2017 • edited

lmorchard Jan 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard commented Jan 25, 2017 • edited

jaredhirsch commented Jan 26, 2017

jaredhirsch commented Jan 26, 2017

jaredhirsch commented Jan 24, 2017 •

edited

lmorchard Jan 24, 2017 •

edited

lmorchard Jan 25, 2017 •

edited

lmorchard Jan 25, 2017 •

edited

lmorchard commented Jan 25, 2017 •

edited

lmorchard Jan 25, 2017 •

edited

lmorchard commented Jan 25, 2017 •

edited