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

Only send JS errors+stats in production [no merge] #1987

Merged
merged 6 commits into from
Feb 20, 2015

Conversation

viddo
Copy link
Contributor

@viddo viddo commented Jan 29, 2015

Fixes #1892

In addition to the minor code change:

TrackJS:

  • Removed trackAjaxFail: false since trackJs lib yields a warning that property is not supported anyway
  • Load trackJs on all environments (that has a token set in app_config.yml), but only send errors to trackJs if the config has enabled set to true, i.e.:
trackjs: 
  enabled: true

For Mixpanel:

  • Always load mixpanel on all environments (that has a token set in app_config.yml)
  • Per Mixpanel's own proposal use a "dev/staging" token for non-production environments to not pollute statistics.

Will talk to devops about how to config changes for staging+production environments.

So, effectively we would catch errors at latest in staging, and for devs touching frontend should have the tokens in their dev env app_config.yml to catch errors even earlier.

edit: removed Rails.env usage to load TrackJS

Load lib if token is given, but only send errors if application is
loaded for production. Also removed unnecessary wrapper
@viddo
Copy link
Contributor Author

viddo commented Jan 29, 2015

@CartoDB/frontend sounds reasonable?

@xavijam
Copy link
Contributor

xavijam commented Jan 29, 2015

Sounds good dude 👍 💃

@viddo
Copy link
Contributor Author

viddo commented Jan 29, 2015

@zenitraM @lbosque @santisaez do you have any objections or is there anything I might have overlooked? If everything is OK I would need help from you with the following two tasks:

  1. set the production TrackJs token for staging's app_config.yml
  2. create a "dev token" for Mixpanel and set it for staging's app_config.yml (and pass the token to frontend@ mailing list so "frontend devs" have it too.

@javisantana
Copy link
Contributor

for me it should send stats when we have config for that, doing things in function of rails env gave a lot of problems in the past

@javisantana
Copy link
Contributor

please update news if needed and merge

@viddo
Copy link
Contributor Author

viddo commented Feb 17, 2015

Re: rails env that's totally reasonable 👍, I'll make sure to fix it.

The reason why it have not been merged yet is because I need help from someone who has admin privileges to staging/production environments and mixpanel to do the necessary changes required by this PR (asked several occasions). I'll get back to this one as soon as I finished the other issues with higher priority related to the new dashboard.

Re: update NEWS, will do once this is ready to be merged.

@viddo viddo changed the title Only send errors/stats for production Only send errors/stats for production (do not merge) Feb 17, 2015
@viddo viddo changed the title Only send errors/stats for production (do not merge) Only send errors/stats for production [no merge] Feb 17, 2015
@viddo viddo changed the title Only send errors/stats for production [no merge] Only send JS errors+stats in production [no merge] Feb 19, 2015
@viddo
Copy link
Contributor Author

viddo commented Feb 19, 2015

Based on feedback have done some additional changes. To be able to merge & release this branch I would need the following changes to config/app_config.yml:

For staging:

mixpanel:
   api_key: 'dev-api-key'
   api_secret: 'dev-api-secret'
   token: 'dev-token'
trackjs:
  customer: 'key-used-in-production'
  enabled: false

For production:

trackjs:
  enabled: true

Again, the "dev-*_" strings should be created through Mixpanel's dashboard. @matallo It have come to my attention that you might be able to help me with this? ;)

For the config changes, can someone of Devops @santisaez @zenitraM @azamorano help me out tomorrow Fri to finish this off finally?

@viddo
Copy link
Contributor Author

viddo commented Feb 20, 2015

retest this please

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

viddo added a commit that referenced this pull request Feb 20, 2015
Only send JS errors+stats in production [no merge]
@viddo viddo merged commit 4aa225b into master Feb 20, 2015
@viddo viddo deleted the 1892-fix-mixpanel-trackjs-staging branch February 20, 2015 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure mixpanel and trackJs works for dev/staging env
4 participants