Telemetry #247
Telemetry #247
Conversation
b3f0bab
to
289619c
This code relies on the insight package: https://www.npmjs.com/package/insight I have a patch to fix this issue here: Use the patch to test this PR. |
2652b45
to
6f0c505
LGTM |
f24b513
to
0e3c6de
LGTM!! |
cfb2838
to
5f0f839
cordova-cli telemetry on | ||
cordova-cli telemetry off | ||
cordova-cli build --no-telemetry |
nikhilkh
May 10, 2016
Contributor
These examples are not necessarily needed here as they are simple and not common scenarios - it will be nice to keep this output as small as it could be.
These examples are not necessarily needed here as they are simple and not common scenarios - it will be nice to keep this output as small as it could be.
@@ -23,6 +23,12 @@ Project Commands | |||
serve .............................. Run project with a local webserver | |||
(including prepare) | |||
|
|||
Telemetry Commands | |||
telemetry on ....................... Turn telemetry collection on |
nikhilkh
May 10, 2016
Contributor
Sounds like telemetry is a "global command" and should go along with create & help. All details about telemetry command e.g. privacy notice, on/off should not be specified here. The goal of this top level help is to be succinct without going into details.
Sounds like telemetry is a "global command" and should go along with create & help. All details about telemetry command e.g. privacy notice, on/off should not be specified here. The goal of this top level help is to be succinct without going into details.
nikhilkh
May 10, 2016
Contributor
Also, we need to document these commands and options here: https://github.com/apache/cordova-cli/blob/master/doc/readme.md
Also, we need to document these commands and options here: https://github.com/apache/cordova-cli/blob/master/doc/readme.md
off ...................... Turns telemetry collection off | ||
|
||
Details | ||
A timed prompt asking the user to opt-in or out is displayed the first time cordova is run. |
nikhilkh
May 10, 2016
Contributor
Can omit "or out"
Can omit "or out"
omefire
May 10, 2016
•
Author
Contributor
It reinforces that they are making a choice, they have options.
It reinforces that they are making a choice, they have options.
A timed prompt asking the user to opt-in or out is displayed the first time cordova is run. | ||
It lasts for 30 seconds, after which the user is automatically opted-out if he doesn't provide any answer. | ||
In CI environments, the `CI` environment variable can be set, which will prevent the prompt from showing up. | ||
Telemetry collection can also be turned off on a single command by using the `--no-telemetry-flag`. |
nikhilkh
May 10, 2016
Contributor
I believe you meant --no-telemetry
flag
I believe you meant --no-telemetry
flag
cordova-cli build --no-telemetry | ||
|
||
For details, see our privacy notice: https://cordova.apache.org/privacy-notice.html | ||
and the proposal: https://github.com/cordova/cordova-discuss/pull/43 |
nikhilkh
May 10, 2016
Contributor
Best not to reference the pull request here which will get closed and the doc itself will eventually get updated.
Best not to reference the pull request here which will get closed and the doc itself will eventually get updated.
cli(["node", "cordova", "-v"]).then(function() { | ||
expect(console.log.mostRecentCall.args[0]).toMatch(version); | ||
done(); | ||
}).fail(function() { |
nikhilkh
May 10, 2016
Contributor
Can you elaborate why these tests had to be changed and made async?
Can you elaborate why these tests had to be changed and made async?
omefire
May 10, 2016
Author
Contributor
They have changed because telemetry.showPrompt()
is async, therefore the return of cli
turned into async, and the tests had to be updated to reflect that.
They have changed because telemetry.showPrompt()
is async, therefore the return of cli
turned into async, and the tests had to be updated to reflect that.
* Has the user already answered the telemetry prompt? (thereby opting in or out?) | ||
*/ | ||
function hasUserOptedInOrOut() { | ||
return !(insight.optOut === undefined); |
nikhilkh
May 10, 2016
Contributor
to test for undefined in javascript you have to use typeof - http://stackoverflow.com/questions/3390396/how-to-check-for-undefined-in-javascript
to test for undefined in javascript you have to use typeof - http://stackoverflow.com/questions/3390396/how-to-check-for-undefined-in-javascript
omefire
May 10, 2016
Author
Contributor
Quesiton: Why do we 'have' to use 'typeof' ? According to the same link, it seems like this way is fine as well, with the difference between the two being that one throws when the variable is undeclared.
Quesiton: Why do we 'have' to use 'typeof' ? According to the same link, it seems like this way is fine as well, with the difference between the two being that one throws when the variable is undeclared.
purplecabbage
May 10, 2016
Contributor
the typeof === discussion is a matter of preference, I personally have never had an issue with
if (x) { // do something }
so I can check for null at the same time as I typically set js vars to null, but I am a freak.
As long as you are sure that the value will never actually be null, this should be fine; otherwise change it to == for a simple truthy comparison.
There is however an anti-pattern that is not obvious here. A boolean is being used to hold 3 values, no-answer, user-opted-in, user-opted-out. When I see return !(insight.optOut === undefined);
my immediate thought would be to correct it to return (insight.optOut !== undefined )
which is not exactly the same meaning.
I would prefer 2 bools in this case. Something like: if( wasPrompted && isOptedIn ) ...
... but again, preference.
the typeof === discussion is a matter of preference, I personally have never had an issue with
if (x) { // do something }
so I can check for null at the same time as I typically set js vars to null, but I am a freak.
As long as you are sure that the value will never actually be null, this should be fine; otherwise change it to == for a simple truthy comparison.
There is however an anti-pattern that is not obvious here. A boolean is being used to hold 3 values, no-answer, user-opted-in, user-opted-out. When I see return !(insight.optOut === undefined);
my immediate thought would be to correct it to return (insight.optOut !== undefined )
which is not exactly the same meaning.
I would prefer 2 bools in this case. Something like: if( wasPrompted && isOptedIn ) ...
... but again, preference.
nikhilkh
May 10, 2016
Contributor
undefined
in JavaScript can be redefined to anything. typeof
prevents that from having an effect
typeof insight.optOut !== "undefined"
undefined
in JavaScript can be redefined to anything. typeof
prevents that from having an effect
typeof insight.optOut !== "undefined"
purplecabbage
May 10, 2016
Contributor
> typeof null === 'undefined'
< false
My concern is only for clarity of intent. If someone decides to redefine undefined, I would think things will break all over.
> typeof null === 'undefined'
< false
My concern is only for clarity of intent. If someone decides to redefine undefined, I would think things will break all over.
omefire
May 11, 2016
Author
Contributor
If undefined
was to be redefined, that wouldn't be our biggest concern. Plus, modern webviews/browsers don't allow that anymore.
If undefined
was to be redefined, that wouldn't be our biggest concern. Plus, modern webviews/browsers don't allow that anymore.
}); | ||
|
||
if (someChecksFailed) throw new CordovaError('Some of requirements check failed'); | ||
}).done(); |
nikhilkh
May 10, 2016
Contributor
Looks like you end up removing "done()" - does that not cause us to lose exceptions?
Looks like you end up removing "done()" - does that not cause us to lose exceptions?
omefire
May 11, 2016
Author
Contributor
Good point. This will end up requiring a rework of how the tests are structured, since calling done()
will end the promise chain.
Good point. This will end up requiring a rework of how the tests are structured, since calling done()
will end the promise chain.
omefire
May 11, 2016
Author
Contributor
I've refactored and introduced done()
into the main function. This required introducing a callback into the main/exported function for testing purposes.
I've refactored and introduced done()
into the main function. This required introducing a callback into the main/exported function for testing purposes.
if (shouldCollectTelemetry) { | ||
telemetry.track(cmd, result.isFulfilled() ? 'successful' : 'unsuccessful'); | ||
} | ||
return result; |
nikhilkh
May 10, 2016
Contributor
why return result? - also, this is not being done consistently.
why return result? - also, this is not being done consistently.
omefire
May 11, 2016
Author
Contributor
returning result is done for consistency's sake.
returning result is done for consistency's sake.
create(); | ||
var result = create(); | ||
return result.finally(function() { | ||
if (shouldCollectTelemetry) { |
nikhilkh
May 10, 2016
Contributor
Instead of duplicating this code - consider the caller taking the promise and calling "finally" and recording telemetry calling "done()"
Instead of duplicating this code - consider the caller taking the promise and calling "finally" and recording telemetry calling "done()"
omefire
May 11, 2016
•
Author
Contributor
As discussed offline, I'm going ahead with this refactoring. but it will require retesting the whole CLI as it affects everything. Looking at things from the benefits/costs perspective, I'm not sure if that's the best route, but let's do it ...
As discussed offline, I'm going ahead with this refactoring. but it will require retesting the whole CLI as it affects everything. Looking at things from the benefits/costs perspective, I'm not sure if that's the best route, but let's do it ...
insight.askPermission(msg, function (unused, optIn) { | ||
var EOL = require('os').EOL; | ||
if (optIn) { | ||
console.log(EOL + "Thanks for opting into telemetry to help us better cordova"); |
nikhilkh
May 10, 2016
Contributor
better = improve. Also end with a period.
better = improve. Also end with a period.
if (optIn) { | ||
console.log(EOL + "Thanks for opting into telemetry to help us better cordova"); | ||
} else { | ||
console.log(EOL + "You have been opted out of telemetry. To change this, run: cordova telemetry on"); |
nikhilkh
May 10, 2016
Contributor
End with a period.
End with a period.
var result = cordova.raw.serve(port); | ||
return result.finally(function() { | ||
if(shouldCollectTelemetry) { | ||
telemetry.track(cmd, result.isFulfilled() ? 'successful' : 'unsuccessful'); |
nikhilkh
May 10, 2016
Contributor
Looks like "track" itself checks if telemetry should should be collected - why should every calling point also check?
Looks like "track" itself checks if telemetry should should be collected - why should every calling point also check?
Related: apache/cordova-docs#599 |
@@ -21,7 +23,7 @@ Project Commands | |||
run ................................ Run project | |||
(including prepare && compile) | |||
serve .............................. Run project with a local webserver | |||
(including prepare) | |||
(including prepare) |
nikhilkh
May 11, 2016
Contributor
Trailing spaces after (including prepare).
Trailing spaces after (including prepare).
4676c01
to
f5133de
ping ... |
I have one question, maybe I missed it in discussion why is this implemented in CLI and not LIB I think it make more sense to implement in the top level of API entry points in LIB like create platform plugin etc. In LIB will be disable by default |
Other CLIs that use lib (as a lib) would not want this code. Off by default is interesting, but I think this is cleaner, and it's done.
|
@@ -5,6 +5,7 @@ Synopsis | |||
Global Commands | |||
create ............................. Create a project | |||
help ............................... Get help for a command | |||
telemetry .......................... Turn telemetry collection on or off |
nikhilkh
May 12, 2016
Contributor
The CLI reference docs need an update.
The CLI reference docs need an update.
omefire
May 12, 2016
Author
Contributor
Hmm, I thought it was generated from these files. Anyway, it's been updated.
Hmm, I thought it was generated from these files. Anyway, it's been updated.
nikhilkh
May 12, 2016
Contributor
They live in doc/readme.md.
They live in doc/readme.md.
omefire
May 12, 2016
•
Author
Contributor
Yes, I've already updated it and pushed to this PR. thanks.
Yes, I've already updated it and pushed to this PR. thanks.
insight.askPermission(msg, function (unused, optIn) { | ||
var EOL = require('os').EOL; | ||
if (optIn) { | ||
console.log(EOL + "Thanks for opting into telemetry to help us improve cordova."); |
nikhilkh
May 12, 2016
Contributor
Perhaps we should log opt-ins too. It will help us determine opt-in % easily.
Perhaps we should log opt-ins too. It will help us determine opt-in % easily.
omefire
May 12, 2016
Author
Contributor
We can derive this percentage, noo ?
We can derive this percentage, noo ?
omefire
May 12, 2016
Author
Contributor
Updated!
Updated!
I first wanted to go that route, but I then realized doing it in CLI minimizes polluting LIB for a functionality that's not intended to be used by downstream tools. It seems cleaner to handle it here, as mentioned by @purplecabbage |
Sounds good to me to proceed then, it was a random idea |
bb1901e
to
f4e1298
LGTM |
Was there ever a specific Jira issue for adding telemetry? I don't see it.
Multiple overlapping lines, could be a bash issue as well. |
@omefire Can you please take a look into this? |
@purplecabbage, I didn't create a JIRA issue to track the telemetry work, I should have. It was an oversight on my part. My bad. I've tested this on MAC(El Capitan) and Windows10, but couldn't repro your issue. |
I am using windows 10, but I typically use the git-bash shell which may be @purplecabbage On Mon, May 16, 2016 at 10:52 AM, Omar Mefire notifications@github.com
|
@purplecabbage , I can't repro this with git-bash shell on Win10 because I'm running into another issue: nodejs/node#3006 The gist of my issue is that the process is not recognized to be running as TTY, which leads to insight ending the process and opting out: Furthermore, commenting that code out, leads to yet another error: SBoudrias/Inquirer.js#290 Insight, which we depend on doesn't play well with git-bash/MinGW. |
@purplecabbage , could you please file a JIRA issue for what you've encountered ? |
Here is the related proposal: apache/cordova-discuss#43