integrate telemetry by annthurium · Pull Request #91 · atom/metrics

annthurium · 2018-06-22T00:22:51Z

this pull request integrates the new telemetry package for sending metrics to GitHub's internal analytics pipeline, rather than Google Analytics. For more context on this project: http://blog.atom.io/2018/06/20/atom-metrics.html

More specifically:

adds the new telemetry lib as a dependency.
instantiates a StatsStore to hold data which will eventually be sent to GitHub's internal analytics piepline.
adds some methods to the metrics reporter service that expose telemetry apis directly so that other Atom packages can consume them.
modifies the existing methods of the metrics reporter service so that when they are called, data is added to telemetry. The plan is, for a while we'll be in a state where we are sending data to both Google Analytics and the internal pipeline. That way, we can cross check both sources. When we are confident that the internal pipeline data is solid, we will deprecate sending data to Google Analytics.
adds unit tests for new functionality
increases unit test coverage for existing code a tiny bit.
- tests opt in / opt out events
- tests timing events
clarifies language in some of the tests.

Manual testing plan:

Verify that data is still being sent to Google Analytics from Atom using the network tab of the console when new files are opened.
Manually call incrementCounter and addCustomEvent from the console. Manually call store.reportStats() and verify that the stats are successfully sent to central.
verify that Atom tries to send stats when the reporting loop interval has passed

drguthals · 2018-06-22T19:51:06Z

Could you give guidance on what you're looking back in terms of feedback?

My assumptions are, at minimum you're looking for:
@jasonrudolph to give an overview of the code and provide feedback
@sguthals to be aware of what is being built and the scope
Are these assumptions correct?

For context, in the future, a review ask might also include "can you dogfood this" or "can you test this thoroughly"

jasonrudolph

👋 Hi @annthurium: Thanks for getting this going. ⚡

I took a first pass through this diff, and I've offered some suggestions and noted a few questions below. I hope this helps.

jasonrudolph · 2018-06-25T16:20:53Z

lib/reporter.js

    }

    this.send(params)
+    // todo (tt, 6/2018): send timing to telemetry.


Is this something that will be implemented in this pull request? Or would it happen in a follow-up pull request?

this is something that would happen in a follow up pull request. If you think it's better not to track things like that in TODOs I'd be happy to remove it.

@annthurium: As a general guideline, my personal preference is to track TODOs in issues, as opposed to code comments. 😺

cool. I sometimes add TODOs if I think other folks reading/modifying this code would want to be aware of imminent changes. Fine with removing this though.

jasonrudolph · 2018-06-25T16:31:15Z

lib/reporter.js

+    const eventName = 'appview'
    const params = {
-      t: 'appview',
+      t: eventName,


In Google Analytics terms, t refers to the "hit type". Possible values are pageview, event, exception, timing, and just a few others.

With that in mind, I'd recommend renaming eventName to eventType to convey that it's a fairly course-grained/high-level concept, whereas eventName might imply something much more granular (e.g., commit, consent-change, etc.).

cool. done.

jasonrudolph · 2018-06-25T16:31:55Z

lib/reporter.js

      ev: this.commandCount[commandName]
    }

+    this.addCustomEvent(params, 'command')


I feel rotten bringing this up again, but I keep finding myself confused by this interface. (Sorry! 😦) Here, I was expecting the event name to be the first arg, followed by the event metadata, but I see that the order is the opposite.

We talked about an alternative interface in atom/telemetry#12 (comment), and you mentioned plans to run this API few some more folks for feedback. Did you have a chance to do so?

I agree, having the event name first makes more sense to me and seems to be common practice in API design. Typically my principle for argument ordering is "most important/relevant parameter first" and I think in this case the event name is the most important parameter.

sure thing! can make the event name first.

jasonrudolph · 2018-06-25T16:35:15Z

package-lock.json

+      "dev": true
+    }
+  }
+}


For consistency with the other packages, would you mind deleting this file and adding package-lock.json to .gitignore?

A couple threads for reference:

atom/atom#16493
atom/teletype#125

oh, thanks! I had no idea I was not supposed to check in package.lock files for Atom packages.

Since apm currently ignores them they aren't giving us any benefit right now, and simply add a bunch of extra maintenance work at the moment.

jasonrudolph · 2018-06-25T16:36:30Z

spec/metrics-spec.js

-
-        atom.commands.dispatch(workspaceElement, 'editor:move-to-end-of-line', null)
-        expect(Reporter.request).not.toHaveBeenCalled()
+        assertNotCalledHelper('core:move-up')


To make this more expressive, I recommend renaming assertNotCalledHelper to assertCommandNotReported. I think that would more clearly convey the intent. (The fact that the method is a helper is an implementation detail that I don't think we need to expose in the name of the method.)

jasonrudolph · 2018-06-25T16:58:26Z

spec/metrics-spec.js

+    spyOn(store, 'setOptOut')
+    spyOn(Reporter, 'sendEvent')
+    await atom.config.set('core.telemetryConsent', 'limited')
+


Tiny observation/question: Is the whitespace in this test intentionally different from the whitespace in the previous test? If not, can I talk you using the same whitespace in both tests?

ah, good catch. fixed.

jasonrudolph · 2018-06-25T17:04:49Z

spec/metrics-spec.js


        const {mainModule} = atom.packages.getLoadedPackage('metrics')
        mainModule.shouldIncludePanesAndCommands = true
+        Reporter.addCustomEvent.reset()


Given that we're calling this in the afterEach function for all tests in this file, can we remove this line, or do we need it?

Removed the reset from all tests in this file, as it's unnecessary, and left it here.

jasonrudolph · 2018-06-25T17:05:00Z

spec/metrics-spec.js

      })

      it('will not report pane items', async () => {
+        Reporter.addCustomEvent.reset()


Given that we're calling this in the afterEach function for all tests in this file, can we remove this line, or do we need it?

ah, these were some debug code trying to get to the bottom of my comment re file open events. Will remove them.

jasonrudolph · 2018-06-25T17:09:29Z

spec/metrics-spec.js

    describe('when there are paths in the exception', () => {
      it('strips unix paths surrounded in quotes', () => {
-        let message = "Error: ENOENT, unlink '/Users/someguy/path/file.js'"
+        let message = "Error: ENOENT, unlink '/Users/someuser/path/file.js'"


jasonrudolph · 2018-06-25T17:27:31Z

lib/reporter.js

+  static addCustomEvent (event, eventName) {
+    store.addCustomEvent(event, eventName)
+  }
  static sendEvent (category, action, label, value) {


I'm guessing that you'd want anyone writing new code to use addCustomEvent instead of sendEvent. Is that correct? If so, I'd recommend marking sendEvent as deprecated [example]. What do you think?

great suggestion!

Also, could/should it be called sendCustomEvent? To me, add sounds like we're registering an event type rather than sending some information

ah, good call. Will change this too.

annthurium · 2018-06-25T18:36:37Z

@sguthals you asked to be added to code reviews as a non blocking reviewer awhile back. I didn't have specific feedback I wanted from you with regards to this pull request, as I know I can count on @jasonrudolph for a thorough code review. 😄

drguthals · 2018-06-25T18:51:44Z

Awesome! That is what I assumed 😄

In general, I'd like to get in the habit of including in the PR description what you're looking for in terms of feedback. At some point it will be nice to provide feedback on things like usability) and if we get in the habit of being specific about the type of feedback we're looking for I think both pr author and reviewer will be happier 👍

daviwil

Looking good! I've added some feedback and +1'ed some of Jason's comments

daviwil · 2018-06-25T20:07:05Z

lib/reporter.js

      ev: this.commandCount[commandName]
    }

+    this.addCustomEvent(params, 'command')


I agree, having the event name first makes more sense to me and seems to be common practice in API design. Typically my principle for argument ordering is "most important/relevant parameter first" and I think in this case the event name is the most important parameter.

daviwil · 2018-06-25T20:07:36Z

lib/store.js

@@ -0,0 +1,7 @@
+const telemetry = require('telemetry-github')
+
+const store = new telemetry.StatsStore('atom', atom.getVersion(), atom.inDevMode(), () => '')


I'd also make that parameter optional and do the "right thing" when the user doesn't pass anything.

daviwil · 2018-06-25T20:19:29Z

lib/reporter.js

+  static addCustomEvent (event, eventName) {
+    store.addCustomEvent(event, eventName)
+  }
  static sendEvent (category, action, label, value) {


Also, could/should it be called sendCustomEvent? To me, add sounds like we're registering an event type rather than sending some information

annthurium

still working on some things - thanks for the feedback!

annthurium · 2018-06-25T18:56:21Z

spec/metrics-spec.js

-
-        atom.commands.dispatch(workspaceElement, 'editor:move-to-end-of-line', null)
-        expect(Reporter.request).not.toHaveBeenCalled()
+        assertNotCalledHelper('core:move-up')


annthurium · 2018-06-25T18:57:37Z

lib/reporter.js

+  static addCustomEvent (event, eventName) {
+    store.addCustomEvent(event, eventName)
+  }
  static sendEvent (category, action, label, value) {


great suggestion!

annthurium · 2018-06-25T19:00:28Z

lib/reporter.js

    }

    this.send(params)
+    // todo (tt, 6/2018): send timing to telemetry.


this is something that would happen in a follow up pull request. If you think it's better not to track things like that in TODOs I'd be happy to remove it.

annthurium · 2018-06-25T20:20:08Z

spec/metrics-spec.js

+    spyOn(store, 'setOptOut')
+    spyOn(Reporter, 'sendEvent')
+    await atom.config.set('core.telemetryConsent', 'limited')
+


ah, good catch. fixed.

annthurium · 2018-06-25T20:22:28Z

spec/metrics-spec.js


        const {mainModule} = atom.packages.getLoadedPackage('metrics')
        mainModule.shouldIncludePanesAndCommands = true
+        Reporter.addCustomEvent.reset()


annthurium · 2018-06-25T20:22:58Z

spec/metrics-spec.js

-
-        atom.commands.dispatch(workspaceElement, 'editor:move-to-end-of-line', null)
-        expect(Reporter.request).not.toHaveBeenCalled()
+        assertNotCalledHelper('core:move-up')


annthurium · 2018-06-25T20:26:54Z

spec/metrics-spec.js

      })

      it('will not report pane items', async () => {
+        Reporter.addCustomEvent.reset()


ah, these were some debug code trying to get to the bottom of my comment re file open events. Will remove them.

annthurium · 2018-06-25T20:29:25Z

spec/metrics-spec.js


        const {mainModule} = atom.packages.getLoadedPackage('metrics')
        mainModule.shouldIncludePanesAndCommands = true
+        Reporter.addCustomEvent.reset()


Removed the reset from all tests in this file, as it's unnecessary, and left it here.

annthurium · 2018-06-25T20:31:35Z

lib/store.js

+const telemetry = require('telemetry-github')
+
+const store = new telemetry.StatsStore('atom', atom.getVersion(), atom.inDevMode(), () => '')
+const hasOptedOut = atom.config.get('core.telemetryConsent') !== 'limited'


I could call it notOptedIn because that's really what telemetry cares about. Will update.

annthurium · 2018-06-25T20:36:11Z

lib/reporter.js

+    const eventName = 'appview'
    const params = {
-      t: 'appview',
+      t: eventName,


cool. done.

…cs into tt-18-jun-integrate-telemetry

…n a file open event.

I really need to set up in-editor linting for this repo.

annthurium · 2018-06-28T00:17:11Z

@jasonrudolph : I believe I've addressed all the comments. Thank you for your feedback! Would you be willing to take a second pass through this? Given that this code is important and I'm still new, I'd like an explicit 👍 before I merge.

As far as what kind of feedback I'm looking for:

does my naming make sense?
are there any style nits that would enhance readability / consistency?
is there anything I should be unit testing but I'm not?
is there anything missing from the manual testing plan?

annthurium · 2018-06-28T18:07:15Z

package.json

@@ -18,7 +18,8 @@
  "dependencies": {
    "fs-plus": "^3.0.0",
    "grim": "^2.0.1",


@jasonrudolph : I'm assuming that I don't need to increment the version of metrics-reporter in providedServices here since I'm just adding new methods and the old ones are still supported. Is that the case?

Since this is an additive change, I think it makes sense to increment the version from 1.0.0 to 1.1.0.

Since the new methods didn't exist in version 1.0.0 of the service, and since you want a way for packages to consume the new methods, those packages need to be able to declare a dependency on a version of the service that includes the new methods. By incrementing the version to 1.1.0, existing consumers (e.g., the welcome package) should continue to work, and packages that want to use the new methods can declare a minimum dependency of 1.1.0.

Does that sound reasonable?

got it, thanks! Have incremented the provideReporter version here.

jasonrudolph

As far as what kind of feedback I'm looking for:

Thanks for enumerating the kind of feedback you're looking for! 😻

does my naming make sense?

It makes sense to me.

are there any style nits that would enhance readability / consistency?

I noted a couple tiny suggestions inline below.

is there anything I should be unit testing but I'm not?

I don't think so.

is there anything missing from the manual testing plan?

A few questions:

All of the stated steps call for using Atom in dev mode. Is there something noteworthy about dev mode as it relates to testing this functionality? Or would you expect these verification steps to also succeed when running Atom in normal (non-dev) mode?
As far as I can tell, there's no production code that currently uses incrementCounter. Will you manually invoke incrementCounter and verify that it has the desired effect?
Am I correct in thinking that sendException will continue to send info only to Google Analytics, and will intentionally not send data to Telemetry?

jasonrudolph · 2018-06-28T17:55:07Z

lib/reporter.js

+  }
+
+  // Deprecated: this is the old API that was used to send events to Google Analytics.
+  // new callers should use addCustomEvent instead.


are there any style nits that would enhance readability / consistency?

For style consistency with other deprecation comments (e.g., https://git.io/f4b4H), I'd recommend writing this as:

// Deprecated: Use addCustomEvent instead.

jasonrudolph · 2018-06-28T17:55:14Z

lib/reporter.js

  }

+  // Deprecated: this is the old API that was used to send events to Google Analytics.
+  // new callers should use addTiming instead.


are there any style nits that would enhance readability / consistency?

For style consistency with other deprecation comments (e.g., https://git.io/f4b4H), I'd recommend writing this as:

// Deprecated: Use addCustomEvent instead.

jasonrudolph · 2018-06-28T18:25:16Z

lib/reporter.js

+  }
+
+  static addCustomEvent (eventType, event) {
+    store.addCustomEvent(eventType, event)


Today, if you call sendEvent, sendTiming, sendPaneItem, sendCommand, the package automatically gathers a handful of dimensions defined in consentedParams and includes those dimensions when reporting the metric to Google Analytics:

metrics/lib/reporter.js

Lines 163 to 175 in 6a5833e

static consentedParams () {

const memUse = process.memoryUsage()

return {

// cd1: was start date, removed

cd2: getOsArch(),

cd3: process.arch,

cm1: memUse.heapUsed >> 20, // Convert bytes to megabytes

cm2: Math.round((memUse.heapUsed / memUse.heapTotal) * 100),

sr: `${window.screen.width}x${window.screen.height}`,

vp: `${window.innerWidth}x${window.innerHeight}`,

aiid: getReleaseChannel()

}

}

I'm thinking that any metrics sent via Telemetry should also automatically include those dimensions. What do you think?

hmmm. I think having that data would be ideal, but I worry about request size.

The max size of the post body in a request in Rails is 2GB. So we have a lot of characters to go through before we hit the size limit. (Although, depending on how long a request takes to process, we might well run into timeout problems before we bump up against size limits.)

Anyway, let's try sending the data with the request and see how it goes. We can always revisit in the future if it becomes a problem.

when calling `addCustomEvent` or `addTiming`, send the same metadata to the store that we send to Google Analytics (processor, etc.)

…cs into tt-18-jun-integrate-telemetry

annthurium · 2018-06-28T23:01:05Z

Thanks for the second pass, @jasonrudolph!

To answer your questions about the test plan:

All of the stated steps call for using Atom in dev mode. Is there something noteworthy about dev mode as it relates to testing this functionality? Or would you expect these verification steps to also succeed when running Atom in normal (non-dev) mode?

Correct - these steps would also succeed in non-dev mode. I suppose specifying that I tested in dev mode was rather pedantic of me. 😛 I am happy to change it if you think it could be confusing.

As far as I can tell, there's no production code that currently uses incrementCounter. Will you manually invoke incrementCounter and verify that it has the desired effect?

Correct, I will manually invoke incrementCounter.

Am I correct in thinking that sendException will continue to send info only to Google Analytics, and will intentionally not send data to Telemetry?

That is also correct - telemetry doesn't handle exceptions. I wanted to get the metrics working first and then worry about exceptions.

jasonrudolph

@annthurium: Thanks for the clarifications and for the latest code updates. 👍

annthurium · 2018-06-29T22:33:25Z

yay, it works, it works!!

thanks again @jasonrudolph for all your help with this!

Tilde Ann Thurium added 9 commits June 20, 2018 11:41

install module

4caa32b

integrate telemetry with metrics

b029027

add tests for new methods the metrics-reporter service

43b1a9a

add test for deprecation events

eb066cd

WIP

0c68953

👕

aae1a26

add tests for opt-in / opt-out status changes

fbe3a33

add tests for optional package counts

17fd25a

👕 again

def4c69

annthurium changed the title ~~integrate telemetry [wip]~~ integrate telemetry Jun 22, 2018

someguy => someuser

a9afa07

annthurium requested review from drguthals and jasonrudolph June 22, 2018 16:57

annthurium requested a review from daviwil June 25, 2018 17:32

jasonrudolph reviewed Jun 25, 2018

View reviewed changes

annthurium mentioned this pull request Jun 25, 2018

Report arbitrary release channel names #92

Merged

Tilde Ann Thurium added 3 commits June 25, 2018 11:55

assertNotCalledHelper -> assertCommandNotReported

8c11981

deprecate sendEvent in favor of addCustomEvent

b3a4284

make spacing consistent between tests

fa948e7

daviwil reviewed Jun 25, 2018

View reviewed changes

Tilde Ann Thurium added 3 commits June 25, 2018 13:28

clean up some unnecessary resetting of spies.

c09537b

hasOptedOut -> notOptedIn

db150cb

eventName -> eventType

6488efa

annthurium commented Jun 25, 2018

View reviewed changes

Tilde Ann Thurium added 3 commits June 25, 2018 13:48

⬆️ telemetry version

ac50915

make eventType arg come first in addCustomEvent

35d6d9b

👕 again.

05b0b16

Tilde Ann Thurium added 6 commits June 25, 2018 14:27

⬆️ telemetry once more with feeling.

f37c312

remove no op function from store constructor

69cdf30

Merge branch 'tt-18-jun-integrate-telemetry' of github.com:atom/metri…

7c21b3f

…cs into tt-18-jun-integrate-telemetry

add unit tests for reporting release channel names.

77324f5

remove TODO

0d5ad62

make test for non-reporting of pane items test a pane item rather tha…

a05ce8f

…n a file open event.

daviwil mentioned this pull request Jun 25, 2018

Iteration Plan: June 25 - July 6, 2018 atom/atom#17579

Closed

12 tasks

Tilde Ann Thurium added 7 commits June 26, 2018 10:05

Add package-lock.json to .gitignore

8cc3b22

actuall remove package-lock.json

4acc40a

⬆️ telemetry

d39c5fe

add method for sending timing metrics to telemetry

ff1e9b4

add unit test for methods that send timing metrics.

029a03d

👕

77a2969

I really need to set up in-editor linting for this repo.

sendTimingV2 -> addTiming

7a47c50

Merge 'master' into tt-18-jun-integrate-telemetry

6a5833e

annthurium commented Jun 28, 2018

View reviewed changes

jasonrudolph reviewed Jun 28, 2018

View reviewed changes

Tilde Ann Thurium added 5 commits June 28, 2018 14:02

nit: match deprecation style.

61774cf

⬆️ telemetry 8th time's a charm

4ddf278

increment version of metrics-reporter service

769efc0

send event metadata to store

e36f41f

when calling `addCustomEvent` or `addTiming`, send the same metadata to the store that we send to Google Analytics (processor, etc.)

Merge branch 'tt-18-jun-integrate-telemetry' of github.com:atom/metri…

5cdefc6

…cs into tt-18-jun-integrate-telemetry

jasonrudolph approved these changes Jun 29, 2018

View reviewed changes

⬆️ telemetry

6e7b441

annthurium merged commit 503349f into master Jun 29, 2018

annthurium deleted the tt-18-jun-integrate-telemetry branch June 29, 2018 22:39

annthurium mentioned this pull request Jun 29, 2018

⬆️ metrics atom/atom#17604

Closed

		@@ -0,0 +1,7 @@
		const telemetry = require('telemetry-github')

		const store = new telemetry.StatsStore('atom', atom.getVersion(), atom.inDevMode(), () => '')

	static consentedParams () {
	const memUse = process.memoryUsage()
	return {
	// cd1: was start date, removed
	cd2: getOsArch(),
	cd3: process.arch,
	cm1: memUse.heapUsed >> 20, // Convert bytes to megabytes
	cm2: Math.round((memUse.heapUsed / memUse.heapTotal) * 100),
	sr: `${window.screen.width}x${window.screen.height}`,
	vp: `${window.innerWidth}x${window.innerHeight}`,
	aiid: getReleaseChannel()
	}
	}

Conversation

annthurium commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drguthals commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonrudolph left a comment

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!

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!

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!

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!

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!

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!

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!

annthurium commented Jun 25, 2018

Uh oh!

drguthals commented Jun 25, 2018

Uh oh!

daviwil left a comment

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!

annthurium left a comment

Choose a reason for hiding this comment

Uh oh!

annthurium commented Jun 22, 2018 •

edited

Loading

drguthals commented Jun 22, 2018 •

edited

Loading