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

Track JS errors in GA #189

Merged
merged 17 commits into from
Feb 27, 2018
Merged

Track JS errors in GA #189

merged 17 commits into from
Feb 27, 2018

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Feb 12, 2018

Fix #39.

TLDR: Using raven-js to capture errors and context info, then sending it to GA instead of sentry. It's a little out there, but it should be informative.

I did a pretty thorough write-up in src/utils/tracking/README.md, which lays out the changes in this PR. It might make sense to move some of the README to the Jaeger docs. They need to be updated to reflect gaTrackingIDtracking.gaID and the addition of tracking.trackErrors, anyway.

The changes in the *.md files that aren't related to GA tracking can be ignored; they're captured in #188.

The version info generated for GA can also be used to satisfy #113. I'll add a footer in another PR.

I'm currently working on UX event tracking in a different branch.

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
@ghost ghost assigned tiffon Feb 12, 2018
@ghost ghost added the review label Feb 12, 2018
@codecov
Copy link

codecov bot commented Feb 12, 2018

Codecov Report

Merging #189 into master will increase coverage by 0.59%.
The diff coverage is 93.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   90.86%   91.45%   +0.59%     
==========================================
  Files          93       95       +2     
  Lines        1937     2130     +193     
  Branches      382      434      +52     
==========================================
+ Hits         1760     1948     +188     
- Misses        154      161       +7     
+ Partials       23       21       -2
Impacted Files Coverage Δ
.../TracePage/TraceTimelineViewer/SpanDetail/index.js 100% <ø> (ø) ⬆️
src/components/App/Page.js 100% <ø> (ø) ⬆️
src/constants/default-config.js 100% <ø> (ø) ⬆️
src/index.js 0% <0%> (ø) ⬆️
src/utils/tracking/fixtures.js 100% <100%> (ø)
src/utils/config/get-config.js 100% <100%> (ø) ⬆️
src/components/App/TopNav.js 75% <100%> (ø) ⬆️
src/components/DependencyGraph/index.js 100% <100%> (ø) ⬆️
src/utils/tracking/index.js 90.14% <90.14%> (ø)
src/utils/tracking/conv-raven-to-ga.js 97.39% <97.39%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97fb871...828f97c. Read the comment docs.

@tiffon
Copy link
Member Author

tiffon commented Feb 12, 2018

@simonrobb Can you look this over when you get the chance?

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
src/index.js Outdated
import 'u-basscss/css/flexbox.css';
import 'u-basscss/css/layout.css';
import 'u-basscss/css/margin.css';
import 'u-basscss/css/padding.css';
import 'u-basscss/css/position.css';
import 'u-basscss/css/typography.css';

initTracking();

const UI_ROOT_ID = 'jaeger-ui-root';

/* istanbul ignore if */
if (document && process.env.NODE_ENV !== 'test') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to test for NODE_ENV !== 'test' or NODE_ENV === 'production'? Probably don't want to track events in staging environments etc, if they exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


import prefixUrl from '../prefix-url';

const UNKONWN_SYM = { sym: '??', word: '??' };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May as well fix this typo (UNKONWN) now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Signed-off-by: Joe Farro <joef@uber.com>
@@ -0,0 +1,92 @@
#!/usr/bin/env node

const spawnSync = require('child_process').spawnSync;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this? Maybe a quick comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -0,0 +1,202 @@
# Google Analytics (GA) Tracking In Jaeger UI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great write up Joe

Although I'm all for documentation my only comment would be, is it really worth to maintain all this stuff for Jaeger UI? Although this is nice to have, my concern is this information will go stale as its not really part of the core of what Jaeger provides.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a valid point. But, it really shouldn't need to change much. Seems like low-maintenance docs, to me.

What do you suggest?

@@ -0,0 +1,357 @@
// @flow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could be a library in itself outside of Jaeger

Signed-off-by: Joe Farro <joef@uber.com>
@tiffon tiffon merged commit 4e7fc85 into master Feb 27, 2018
@ghost ghost removed the review label Feb 27, 2018
@yurishkuro yurishkuro deleted the issue-39-track-js-errors branch January 29, 2020 15:07
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Track errors in GA w raven-js; TODO: tests, readme

Signed-off-by: Joe Farro <joef@uber.com>

* Include CSS selector with last error breadcrumb

Signed-off-by: Joe Farro <joef@uber.com>

* README for GA error tracking

Signed-off-by: Joe Farro <joef@uber.com>

* Misc cleanup

Signed-off-by: Joe Farro <joef@uber.com>

* README info on GA Application Tracking

Signed-off-by: Joe Farro <joef@uber.com>

* Misc fix to tracking README

Signed-off-by: Joe Farro <joef@uber.com>

* Misc cleanup to raven message conversion to GA

Signed-off-by: Joe Farro <joef@uber.com>

* Tests for tracking

Signed-off-by: Joe Farro <joef@uber.com>

* Apply prettier to markdown files

Signed-off-by: Joe Farro <joef@uber.com>

* Run prettier on *.md files

Signed-off-by: Joe Farro <joef@uber.com>

* Add tracking.trackErrors feature flag to UI config

Signed-off-by: Joe Farro <joef@uber.com>

* Upgrade prettier, add doc for tracking.trackErrors

Signed-off-by: Joe Farro <joef@uber.com>

* Check for breadcrumbs when tracking errors

Signed-off-by: Joe Farro <joef@uber.com>

* Fix typo, remove unnecessary NODE_ENV guard

Signed-off-by: Joe Farro <joef@uber.com>

* Comments for get-tracking-version script

Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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.

3 participants