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

RIP Google Analytics #100

Merged
merged 14 commits into from Aug 23, 2018

Conversation

Projects
None yet
2 participants
@annthurium
Contributor

annthurium commented Aug 21, 2018

Description of the Change

Previously, we migrated to send data to GitHub's analytics pipeline. Then we validated the data we are sending to the internal pipeline against Google Analytics to cross check. Everything looks good, so it's time to rip Google Analytics out entirely.

This pull request:

  • Removes the Reporter.post and Reporter.send methods that make requests to Google Analytics.
  • Rips out all references in unit tests to those methods
  • Sends exceptions to GitHub's internal analytics pipeline, and adds unit tests to validate that the exception data is formatted the way we expect.
  • Rips out the code for generating client id / uuid. telemetry handles setting the uuid already so node-uuid is no longer a necessary dependency.

Alternate Designs

So we have a couple of methods that can send data to the stats store and eventually to GitHub's internal analytics pipeline:

Reporter.sendEvent (deprecated)
Reporter.sendTiming (deprecated)
Reporter.addCustomEvent
Reported.addTiming

I'd love to rip out sendEvent and sendTiming entirely. But I found a few instances of community code that calls consumeReporter with older versions of the reporter interface:
https://github.com/search?q=consumeReporter&type=Code

Many of these are dotfiles repos, where users appear to have checked in their .atom file. One of them appears to be a community package: https://github.com/ly1984119/cde-welcome

I chose to err on the more conservative side and avoid the possibility of breaking functionality for our users, even though it leaves this deprecated api cluttering our codebase. If you disagree, let's talk - I'm entirely open to feedback.

I'm also not sure if sending the exceptions to GitHub's analytics pipeline is necessary. Other GitHub apps (such as Desktop) are sending theirs, but Atom already uses BugSnag. Do we really need the exceptions in both places? It could be useful to, for example, compare exception rates of different applications.

Benefits

The Atom community asked us not to send data to Google Analytics anymore and now we're not. Plus, this cleans up a bunch of redundant code in our unit tests, and rips out a now-unncessary dependency.

Possible Drawbacks

I can't think of any but I'm open to feedback, as always.

@daviwil

Beautiful! Love that this PR is mostly deletes.

annthurium added some commits Aug 21, 2018

@annthurium annthurium merged commit d80e56f into master Aug 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-aug-rip-out-ga branch Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment