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
Merged
Changes from 54 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
4caa32b
install module
Jun 20, 2018
b029027
integrate `telemetry` with `metrics`
Jun 21, 2018
43b1a9a
add tests for new methods the metrics-reporter service
Jun 21, 2018
eb066cd
add test for deprecation events
Jun 21, 2018
0c68953
WIP
Jun 22, 2018
aae1a26
:shirt:
Jun 22, 2018
fbe3a33
add tests for opt-in / opt-out status changes
Jun 22, 2018
17fd25a
add tests for optional package counts
Jun 22, 2018
def4c69
:shirt: again
Jun 22, 2018
a9afa07
someguy => someuser
Jun 22, 2018
8c11981
assertNotCalledHelper -> assertCommandNotReported
Jun 25, 2018
b3a4284
deprecate `sendEvent` in favor of `addCustomEvent`
Jun 25, 2018
fa948e7
make spacing consistent between tests
Jun 25, 2018
c09537b
clean up some unnecessary resetting of spies.
Jun 25, 2018
db150cb
hasOptedOut -> notOptedIn
Jun 25, 2018
6488efa
eventName -> eventType
Jun 25, 2018
ac50915
:arrow_up: telemetry version
Jun 25, 2018
35d6d9b
make `eventType` arg come first in `addCustomEvent`
Jun 25, 2018
05b0b16
:shirt: again.
Jun 25, 2018
e442b41
:arrow_up: telemetry once more with feeling.
Jun 25, 2018
fcc9caf
remove no op function from store constructor
Jun 25, 2018
48b9783
install module
Jun 20, 2018
08834b4
integrate `telemetry` with `metrics`
Jun 21, 2018
746e489
add tests for new methods the metrics-reporter service
Jun 21, 2018
baf0efb
add test for deprecation events
Jun 21, 2018
cdb3700
WIP
Jun 22, 2018
001dd2e
:shirt:
Jun 22, 2018
c8b1b1e
add tests for opt-in / opt-out status changes
Jun 22, 2018
ebce5bd
add tests for optional package counts
Jun 22, 2018
4125774
:shirt: again
Jun 22, 2018
91b5b30
someguy => someuser
Jun 22, 2018
35f2c78
assertNotCalledHelper -> assertCommandNotReported
Jun 25, 2018
1c2395a
deprecate `sendEvent` in favor of `addCustomEvent`
Jun 25, 2018
8487c65
make spacing consistent between tests
Jun 25, 2018
d36422d
clean up some unnecessary resetting of spies.
Jun 25, 2018
b8a40a0
hasOptedOut -> notOptedIn
Jun 25, 2018
216be56
eventName -> eventType
Jun 25, 2018
4627cc9
:arrow_up: telemetry version
Jun 25, 2018
ccd06cb
make `eventType` arg come first in `addCustomEvent`
Jun 25, 2018
4bb9405
:shirt: again.
Jun 25, 2018
f37c312
:arrow_up: telemetry once more with feeling.
Jun 25, 2018
69cdf30
remove no op function from store constructor
Jun 25, 2018
7c21b3f
Merge branch 'tt-18-jun-integrate-telemetry' of github.com:atom/metri…
Jun 25, 2018
77324f5
add unit tests for reporting release channel names.
Jun 25, 2018
0d5ad62
remove TODO
Jun 25, 2018
a05ce8f
make test for non-reporting of pane items test a pane item rather tha…
Jun 25, 2018
8cc3b22
Add package-lock.json to .gitignore
Jun 26, 2018
4acc40a
actuall remove package-lock.json
Jun 26, 2018
d39c5fe
:arrow_up: telemetry
Jun 26, 2018
ff1e9b4
add method for sending timing metrics to `telemetry`
Jun 26, 2018
029a03d
add unit test for methods that send timing metrics.
Jun 27, 2018
77a2969
:shirt:
Jun 27, 2018
7a47c50
sendTimingV2 -> addTiming
Jun 28, 2018
6a5833e
Merge 'master' into tt-18-jun-integrate-telemetry
jasonrudolph Jun 28, 2018
61774cf
nit: match deprecation style.
Jun 28, 2018
4ddf278
:arrow_up: `telemetry` 8th time's a charm
Jun 28, 2018
769efc0
increment version of `metrics-reporter` service
Jun 28, 2018
e36f41f
send event metadata to store
Jun 28, 2018
5cdefc6
Merge branch 'tt-18-jun-integrate-telemetry' of github.com:atom/metri…
Jun 28, 2018
6e7b441
:arrow_up: telemetry
Jun 29, 2018
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+200 −32
Diff settings

Always

Just for now

@@ -1 +1,2 @@
node_modules
package-lock.json
@@ -4,6 +4,7 @@ const path = require('path')
const Reporter = require('./reporter')
const fs = require('fs-plus')
const grim = require('grim')
const store = require('./store')

const IgnoredCommands = {
'vim-mode:move-up': true,
@@ -31,8 +32,11 @@ module.exports = {

provideReporter () {
return {
incrementCounter: Reporter.incrementCounter.bind(Reporter),
addCustomEvent: Reporter.addCustomEvent.bind(Reporter),
sendEvent: Reporter.sendEvent.bind(Reporter),
sendTiming: Reporter.sendTiming.bind(Reporter),
addTiming: Reporter.addTiming.bind(Reporter),
sendException: Reporter.sendException.bind(Reporter)
}
},
@@ -62,6 +66,9 @@ module.exports = {
if (newValue !== 'undecided') {
Reporter.sendEvent('setting', 'core.telemetryConsent', newValue)
}

const notOptedIn = newValue !== 'limited'
store.setOptOut(notOptedIn)
}))

this.watchActivationOfOptionalPackages()
@@ -1,5 +1,6 @@
const path = require('path')
const querystring = require('querystring')
const store = require('./store')

const extend = function (target, ...propertyMaps) {
for (let propertyMap of propertyMaps) {
@@ -38,6 +39,20 @@ const getOsArch = function () {

module.exports =
class Reporter {
static incrementCounter (counterName) {
store.incrementCounter(counterName)
}

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

This comment has been minimized.

Copy link
@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.

Copy link
@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.

}

static addTiming (eventType, durationInMilliseconds, metadata = {}) {
store.addTiming(eventType, durationInMilliseconds, metadata)
}

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

Copy link
@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.

Copy link
@annthurium

annthurium Jun 28, 2018

Author Contributor

🆒

static sendEvent (category, action, label, value) {

This comment has been minimized.

Copy link
@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.

Copy link
@annthurium

annthurium Jun 25, 2018

Author Contributor

great suggestion!

This comment has been minimized.

Copy link
@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.

Copy link
@annthurium

annthurium Jun 26, 2018

Author Contributor

ah, good call. Will change this too.

const params = {
t: 'event',
@@ -47,9 +62,12 @@ class Reporter {
if (label != null) { params.el = label }
if (value != null) { params.ev = value }

this.addCustomEvent(category, params)
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.

Copy link
@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.

Copy link
@annthurium

annthurium Jun 28, 2018

Author Contributor

👍

static sendTiming (category, name, value) {
const params = {
t: 'timing',
@@ -58,6 +76,7 @@ class Reporter {
utt: value
}

this.addTiming(name, value, {category})
this.send(params)
}

@@ -72,31 +91,36 @@ class Reporter {
}

static sendPaneItem (item) {
const eventType = 'appview'
const params = {
t: 'appview',
t: eventType,
cd: this.viewNameForPaneItem(item)
}

const grammarName = item.getGrammar && item.getGrammar() && item.getGrammar().name
if (grammarName) {
params.dt = grammarName
}

this.addCustomEvent(eventType, params)
this.send(params)
}

static sendCommand (commandName) {
const eventType = 'command'
if (this.commandCount == null) { this.commandCount = {} }
if (this.commandCount[commandName] == null) { this.commandCount[commandName] = 0 }
this.commandCount[commandName]++

const params = {
t: 'event',
ec: 'command',
ec: eventType,
ea: commandName.split(':')[0],
el: commandName,
ev: this.commandCount[commandName]
}

this.addCustomEvent(eventType, params)
this.send(params)
}

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

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

module.exports = store
@@ -18,7 +18,8 @@
"dependencies": {
"fs-plus": "^3.0.0",
"grim": "^2.0.1",

This comment has been minimized.

Copy link
@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.

Copy link
@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.

Copy link
@annthurium

annthurium Jun 28, 2018

Author Contributor

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

"node-uuid": "~1.4.7"
"node-uuid": "~1.4.7",
"telemetry-github": "0.0.7"
},
"devDependencies": {
"standard": "*"
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.