Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integrate telemetry #91

Merged
merged 60 commits into from Jun 29, 2018

Conversation

Projects
None yet
5 participants
@annthurium
Copy link
Contributor

annthurium commented Jun 22, 2018

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

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

@annthurium annthurium requested review from jasonrudolph and sguthals Jun 22, 2018

@sguthals

This comment has been minimized.

Copy link

sguthals commented Jun 22, 2018

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"

@annthurium annthurium requested a review from daviwil Jun 25, 2018

@jasonrudolph
Copy link
Member

jasonrudolph left a comment

👋 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.

@@ -58,6 +67,7 @@ class Reporter {
}

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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.

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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.

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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.).

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

cool. done.

@@ -96,6 +109,7 @@ class Reporter {
ev: this.commandCount[commandName]
}

this.addCustomEvent(params, 'command')

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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?

This comment has been minimized.

@daviwil

daviwil Jun 25, 2018

Member

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.

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

sure thing! can make the event name first.

"version": "0.0.33",
"resolved": "https://registry.npmjs.org/tmp/-/tmp-0.0.33.tgz",
"integrity": "sha512-jRCJlojKnZ3addtTOjdIqoRuPEKBvNXcGYqzO6zWZX8KfKEpnGY5jfggJQ3EjKuu8D4bJRr0y+cYJFmYbImXGw==",
"dev": true,

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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

This comment has been minimized.

@annthurium

annthurium Jun 26, 2018

Author Contributor

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

This comment has been minimized.

@Arcanemagus

Arcanemagus Jun 26, 2018

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.


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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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.)

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

👍

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

👍

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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?

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

ah, good catch. fixed.

beforeEach(async () => {
global.localStorage.setItem('metrics.userId', 'd')
await atom.packages.activatePackage('metrics')

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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?

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

💥

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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

mainModule.shouldIncludePanesAndCommands = false

await conditionPromise(() => Reporter.request.callCount > 0)
})

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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?

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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

@@ -232,28 +263,28 @@ describe('Metrics', async () => {

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'"

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

⚡️


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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 25, 2018

Member

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?

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

great suggestion!

This comment has been minimized.

@daviwil

daviwil Jun 25, 2018

Member

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

This comment has been minimized.

@annthurium

annthurium Jun 26, 2018

Author Contributor

ah, good call. Will change this too.

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 25, 2018

@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. 😄

@sguthals

This comment has been minimized.

Copy link

sguthals commented Jun 25, 2018

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
Copy link
Member

daviwil left a comment

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

@@ -96,6 +109,7 @@ class Reporter {
ev: this.commandCount[commandName]
}

this.addCustomEvent(params, 'command')

This comment has been minimized.

@daviwil

daviwil Jun 25, 2018

Member

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.

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

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

This comment has been minimized.

@daviwil

daviwil Jun 25, 2018

Member

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


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

This comment has been minimized.

@daviwil

daviwil Jun 25, 2018

Member

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

@annthurium
Copy link
Contributor Author

annthurium left a comment

still working on some things - thanks for the feedback!


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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

👍


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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

great suggestion!

@@ -58,6 +67,7 @@ class Reporter {
}

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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.

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

ah, good catch. fixed.

beforeEach(async () => {
global.localStorage.setItem('metrics.userId', 'd')
await atom.packages.activatePackage('metrics')

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

💥


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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

👍

mainModule.shouldIncludePanesAndCommands = false

await conditionPromise(() => Reporter.request.callCount > 0)
})

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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

beforeEach(async () => {
global.localStorage.setItem('metrics.userId', 'd')
await atom.packages.activatePackage('metrics')

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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

const telemetry = require('telemetry-github')

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

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

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

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

cool. done.

@daviwil daviwil referenced this pull request Jun 25, 2018

Closed

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

7 of 12 tasks complete

annthurium added some commits Jun 26, 2018

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

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 28, 2018

@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?
@@ -18,7 +18,8 @@
"dependencies": {
"fs-plus": "^3.0.0",
"grim": "^2.0.1",

This comment has been minimized.

@annthurium

annthurium Jun 28, 2018

Author Contributor

@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?

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 28, 2018

Member

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?

This comment has been minimized.

@annthurium

annthurium Jun 28, 2018

Author Contributor

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

@jasonrudolph
Copy link
Member

jasonrudolph left a comment

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?
}

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 28, 2018

Member

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.

This comment has been minimized.

@annthurium

annthurium Jun 28, 2018

Author Contributor

🆒

this.send(params)
}

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 28, 2018

Member

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.

This comment has been minimized.

@annthurium

annthurium Jun 28, 2018

Author Contributor

👍

}

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

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 28, 2018

Member

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?

This comment has been minimized.

@annthurium

annthurium Jun 28, 2018

Author Contributor

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.

annthurium added some commits Jun 28, 2018

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

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 28, 2018

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
Copy link
Member

jasonrudolph left a comment

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

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Jun 29, 2018

yay, it works, it works!!

screen shot 2018-06-29 at 3 31 52 pm

thanks again @jasonrudolph for all your help with this!

bitmoji

@annthurium annthurium merged commit 503349f into master Jun 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-jun-integrate-telemetry branch Jun 29, 2018

@annthurium annthurium referenced this pull request Jun 29, 2018

Closed

:arrow_up: metrics #17604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.