Implement telemetry in cli #1226

jbcarpanelli · 2019-09-10T21:48:39Z

This PR implements telemetry to the cli package.

TODO:

~~Add tests.~~
~~Disable interactive prompt for telemetry (this is the reason why tests are failing)~~.
~~Disable telemetry in CI environments~~.
~~Write document explaining data collected and how it's anonymized~~. @frangio should we update this PR description with the document you wrote?
~~Do not hash the network name~~.

Out of scope:

Batch telemetry reports while offline.
Postpone first telemetry prompt until after running a couple of commands.

frangio · 2019-09-18T15:36:00Z

packages/cli/src/telemetry.ts

+    const { telemetry } = await inquirer.prompt({
+      name: 'telemetry',
+      type: 'confirm',
+      message: 'telemetry?',


I want advice from legal on what should be the wording here.

ylv-io

Hey @frangio and @jcarpanelli! Amazing job in a such short time! 🚢 🍾
Tests are especially well written!
I've left a few comments mostly TypeScript related - Make SKD Typed Again!

ylv-io · 2019-09-18T15:53:09Z

packages/cli/src/models/config/TruffleConfig.ts

@@ -24,6 +24,8 @@ const TruffleConfig = {
    if (!networkList[network])
      throw Error(`Given network '${network}' is not defined in your ${this.getTruffleConfigFileName(path)} file`);
    config.network = network;
+    config.networkId = config.network_id;


This looks like textbook bad code. Why do we need networkId and network_id fields on the same object?

Also I feel TruffleConfig is important enough to receive a type instead.

Why do we need networkId and network_id fields on the same object?

We considered deleting network_id, but decided to keep it in case it would break something else. I would be okay with deleting it. Would that be better for you?

packages/cli/src/telemetry.ts

ylv-io · 2019-09-18T17:51:52Z

packages/cli/src/telemetry.ts

+
+function hashField(field: Field, salt: string): string {
+  const hash = crypto.createHash('sha256');
+  hash.update(salt);


Let's check here if (salt) because if files will be corrupted we salt with empty string.

What kind of scenario would we be trying to cover for here?

I'm not sure it's worth checking for the empty string. It would be just as bad if we were using any other non-random salt, and there's no way to check for all of them.

packages/cli/src/telemetry.ts

frangio · 2019-09-19T22:10:07Z

I was thinking we might want to also log the version of the CLI that is running.

frangio · 2019-10-11T18:50:22Z

The only thing failing is an unrelated test about solc. I have no idea why. It's not failing on master.

packages/cli/src/commands/init.ts

packages/cli/src/telemetry/index.ts

Co-authored-by: Francisco Giordano <[email protected]>

Co-authored-by Francisco Giordano <[email protected]>

Co-authored-by: Francisco Giordano <[email protected]>

Changes have been implemented

frangio · 2019-10-18T22:42:15Z

Thanks everyone! 🚀

jbcarpanelli marked this pull request as ready for review September 18, 2019 15:28

jbcarpanelli requested review from ylv-io and frangio September 18, 2019 15:29

jbcarpanelli force-pushed the feature/telemetry branch from 1b287d6 to afca374 Compare September 18, 2019 15:32

frangio reviewed Sep 18, 2019

View reviewed changes

ylv-io previously requested changes Sep 18, 2019

View reviewed changes

frangio force-pushed the feature/telemetry branch 2 times, most recently from ca43e44 to c19b5ee Compare October 10, 2019 18:35

nventuro reviewed Oct 17, 2019

View reviewed changes

nventuro reviewed Oct 18, 2019

View reviewed changes

packages/cli/src/telemetry/index.ts Show resolved Hide resolved

spalladino added the status:changes-requested PR needs changes before approving label Oct 18, 2019

jbcarpanelli and others added 17 commits October 18, 2019 17:59

Normalize network id to camelcase

ce97c77

Co-authored-by: Francisco Giordano <[email protected]>

Add env-paths library

113dd8d

Co-authored-by: Francisco Giordano <[email protected]>

Scaffold telemetry functions

c06b9d7

Co-authored-by: Francisco Giordano <[email protected]>

Add telemetryOptIn field to ProjectFile

bda3bf4

Co-authored-by: Francisco Giordano <[email protected]>

Check telemetry opt in on local and global files

ef90d3d

Co-authored-by: Francisco Giordano <[email protected]>

Add telemetry function call in create command

3540a4e

Co-authored-by: Francisco Giordano <[email protected]>

Install uuid and firebase libraries

024c18c

Co-authored-by: Francisco Giordano <[email protected]>

First attempt to send command options to firebase

2d24bcb

Co-authored-by: Francisco Giordano <[email protected]>

Install lodash.mapvalues

300e452

Co-authored-by: Francisco Giordano <[email protected]>

Create user document if it does not exist

63f81b5

Co-authored-by: Francisco Giordano <[email protected]>

Conceal command data before sending it to firebase

0168eb7

Co-authored-by: Francisco Giordano <[email protected]>

Close firebase connections

986d0ec

Co-authored-by: Francisco Giordano <[email protected]>

Handle undefined fields properly on conceal

97fd12f

Co-authored-by: Francisco Giordano <[email protected]>

Avoid telemetry reports on implicit command calls

6ea7fac

Co-authored-by: Francisco Giordano <[email protected]>

Add telemetry to update command

a28123e

Co-authored-by: Francisco Giordano <[email protected]>

Avoid concealing network name if present

525c78e

Co-authored-by Francisco Giordano <[email protected]>

Send interactive flag value to telemetry#report fn

0682e49

Co-authored-by: Francisco Giordano <[email protected]>

frangio added 20 commits October 18, 2019 17:59

make telemetry reporting non-blocking

71225cb

update telemetry opt-in message

34e7bcd

match path basename against string

16e0c04

lint

3274d6b

fix failing test

e791e33

add missing assertion for prompt

ac8f657

enable interactivity for telemetry tests

53ddc60

add telemetry to all missing commands

e6f08d4

fix compile stack too deep error

24454e9

ensure that firebase connections are closed

add2d46

fix balance command tests

f058ea4

remove unused import

a3d8a21

move firebase request off the cli process

25986c2

move reference to dom types and add an explanation

9cb1ffc

silence any potential child process output

a847d7b

add comment explaining why we export an object

94a1fb9

rename variables for clarity

fb6fa2b

remove mutation of object argument

be28984

do not write project file in telemetry check if it doesn't exist yet

cc1500a

add FAQ entry for telemetry

49ec734

frangio force-pushed the feature/telemetry branch from 1603978 to 49ec734 Compare October 18, 2019 20:59

frangio added 2 commits October 18, 2019 18:06

remove only from tests

da9d46c

install yargs explicitly as dev dependency

63e9640

frangio added status:ready-to-merge Order mergify to merge and removed status:changes-requested PR needs changes before approving labels Oct 18, 2019

nventuro approved these changes Oct 18, 2019

View reviewed changes

frangio merged commit 7502b9d into master Oct 18, 2019

frangio deleted the feature/telemetry branch October 18, 2019 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement telemetry in cli #1226

Implement telemetry in cli #1226

jbcarpanelli commented Sep 10, 2019 •

edited

Loading

frangio Sep 18, 2019 •

edited

Loading

ylv-io left a comment

ylv-io Sep 18, 2019

ylv-io Sep 18, 2019

frangio Sep 18, 2019

ylv-io Sep 18, 2019 •

edited

Loading

frangio Sep 19, 2019

frangio commented Sep 19, 2019

frangio commented Oct 11, 2019

frangio commented Oct 18, 2019

Implement telemetry in cli #1226

Implement telemetry in cli #1226

Conversation

jbcarpanelli commented Sep 10, 2019 • edited Loading

TODO:

Out of scope:

frangio Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

ylv-io left a comment

Choose a reason for hiding this comment

ylv-io Sep 18, 2019

Choose a reason for hiding this comment

ylv-io Sep 18, 2019

Choose a reason for hiding this comment

frangio Sep 18, 2019

Choose a reason for hiding this comment

ylv-io Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

frangio Sep 19, 2019

Choose a reason for hiding this comment

frangio commented Sep 19, 2019

frangio commented Oct 11, 2019

frangio commented Oct 18, 2019

jbcarpanelli commented Sep 10, 2019 •

edited

Loading

frangio Sep 18, 2019 •

edited

Loading

ylv-io Sep 18, 2019 •

edited

Loading