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 10 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.
+2,064 −30
Diff settings

Always

Just for now

Copy path View file
@@ -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,6 +32,8 @@ module.exports = {

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

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

this.watchActivationOfOptionalPackages()
Copy path View file
@@ -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) {
@@ -37,6 +38,13 @@ const getOsArch = function () {

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

static addCustomEvent (event, eventName) {
store.addCustomEvent(event, eventName)
}
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',
@@ -46,6 +54,7 @@ class Reporter {
if (label != null) { params.el = label }
if (value != null) { params.ev = value }

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

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

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

This comment has been minimized.

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

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

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

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

}

static sendException (description) {
@@ -71,15 +81,18 @@ class Reporter {
}

static sendPaneItem (item) {
const eventName = 'appview'
const params = {
t: 'appview',
t: eventName,

This comment has been minimized.

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

Copy link
@annthurium

annthurium Jun 25, 2018

Author Contributor

cool. done.

cd: this.viewNameForPaneItem(item)
}

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

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

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

this.addCustomEvent(params, 'command')

This comment has been minimized.

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

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

Copy link
@annthurium

annthurium Jun 25, 2018

Author Contributor

sure thing! can make the event name first.

this.send(params)
}

Copy path View file
@@ -0,0 +1,7 @@
const telemetry = require('telemetry-github')

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

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Jun 25, 2018

Member

Seeing that the final arg is a no-op function, I found myself wondering what significance that arg has. Does StatsStore require that callers provide that arg? If so, would it make sense to change StatsStore to provide a default value for that arg so that callers don't have a provide this no-op function?

This comment has been minimized.

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

This comment has been minimized.

Copy link
@annthurium

annthurium Jun 26, 2018

Author Contributor

great. fixed. thank you!

const hasOptedOut = atom.config.get('core.telemetryConsent') !== 'limited'

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Jun 25, 2018

Member

If the user hasn't yet decided whether to send metrics, the atom.config.get('core.telemetryConsent') will return "undecided", and hasOptedOut will be set to true. In this case, the user hasn't truly opted out; they just haven't made a decision yet. That makes me think that the hasOptedOut variable name is potentially misleading for future devs reading this code. What do you think?

This comment has been minimized.

Copy link
@annthurium

annthurium Jun 25, 2018

Author Contributor

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

store.setOptOut(hasOptedOut)

module.exports = store
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.