Analytics overhaul #1024

2018-02-16T10:20:09+01:00

liamcardenas commented

2018-02-16 10:20:09 +01:00

(Migrated from github.com)

Unfortunately, due to changes in how we route URLs as well as increasing the power, scope, and privacy of analytics, I had to introduce analytics calls outside of renderer/index.js. That said, all the code is contained to one separate file, so its not bad at all.

neb-b (Migrated from github.com) requested changes 2018-02-16 17:43:51 +01:00

neb-b (Migrated from github.com) left a comment

Nice work. IMO we shouldn't be tracking based on the users the email, especially since we aren't allowing opt out during first run (for now). I would definitely turn off analytics if I knew that was happening. I am in favor of keeping the analytics as anonymized as possible.

Also I think it would also be nice to check if the app is in dev mode and make sure no analytics are taken if that is the case.

Nice work. IMO we shouldn't be tracking based on the users the email, especially since we aren't allowing opt out during first run (for now). I would definitely turn off analytics if I knew that was happening. I am in favor of keeping the analytics as anonymized as possible. Also I think it would also be nice to check if the app is in dev mode and make sure no analytics are taken if that is the case.

src/renderer/analytics.js

					
				@ -0,0 +25,4 @@

				    if(user.id) {

				      mixpanel.identify(user.id);

				    }

				    if(user.primary_email) {

neb-b (Migrated from github.com) commented

2018-02-16 17:30:07 +01:00

Should we be using emails in tracking? I thought it would be only user id to keep it anonymized.

src/renderer/analytics.js

					
				@ -0,0 +33,4 @@

				  },

				  toggle: (enabled: boolean, logDisabled: ?boolean): void => {

				    if(!enabled && logDisabled) {

				      mixpanel.track('DISABLED');

neb-b (Migrated from github.com) commented

2018-02-16 17:33:00 +01:00

What does this do?

neb-b (Migrated from github.com) commented

2018-02-16 17:38:01 +01:00

Oh I'm guessing it's so we can know how many people disable analytics?

liamcardenas (Migrated from github.com) reviewed 2018-02-16 20:53:24 +01:00

src/renderer/analytics.js

					
				@ -0,0 +25,4 @@

				    if(user.id) {

				      mixpanel.identify(user.id);

				    }

				    if(user.primary_email) {

liamcardenas (Migrated from github.com) commented

2018-02-16 20:53:24 +01:00

this is so it only tracks people who have an account, which is reasonable, imo

liamcardenas (Migrated from github.com) reviewed 2018-02-16 20:55:24 +01:00

src/renderer/analytics.js

					
				@ -0,0 +33,4 @@

				  },

				  toggle: (enabled: boolean, logDisabled: ?boolean): void => {

				    if(!enabled && logDisabled) {

				      mixpanel.track('DISABLED');

liamcardenas (Migrated from github.com) commented

2018-02-16 20:55:24 +01:00

yes, so we don't get bad data. For instance, we don't think people drop off and we know, instead, they just opted-out of analytics. Also, the reason for the second argument is so that we don't get multiple DISABLED's logged, thereby telling us whenever the open the app.

kauffj commented

2018-02-17 18:19:50 +01:00

(Migrated from github.com)

@liamcardenas re: analytics calls inside of other logic, please at least make sure it's easy for a fork of the app to work without analytics. I.e., someone forking the app shouldn't have to update each call or write dummy functions to have it work. I think it's coded this way currently, but wasn't 100% sure.

Also, please consider how this will work with lbry-redux.

@liamcardenas re: analytics calls inside of other logic, please at least make sure it's easy for a fork of the app to work without analytics. I.e., someone forking the app shouldn't have to update each call or write dummy functions to have it work. I _think_ it's coded this way currently, but wasn't 100% sure. Also, please consider how this will work with `lbry-redux`.

Sign in to join this conversation.

No reviewers

neb-b