add base Telemetry JavaScript snippet (fixes issue #172) #174
Conversation
| /* global localStorage, location, Raven */ | ||
| (function () { | ||
| // This checks for "production"-looking origins (e.g., `https://example.com`). | ||
| if (window.isSecureContext === false || |
There was a problem hiding this comment.
Not blocking but perhaps we should move this to GA. What do you think?
There was a problem hiding this comment.
move what to GA? I'm not entirely clear what what the question is, sorry
There was a problem hiding this comment.
I thought this was for GA and not for Sentry, sorry. But my point still holds. what I mean is not to do this on the client but in the service somehow. I know GA allow to create a user segmentation with domain meeting some criterail. I never used Sentry but perhpahs filtering out development domains is something we can do there.
There was a problem hiding this comment.
yeah, understood. we could move this to the server level. that's what I did with the WebVR Agent. or like how Stripe Checkout handles test/staging/prod environments. 👍
let me play with this today. I think I can this to a simpler setup, so we don't have to do all these manual checks at the client level. I had similar concerns, so I'm glad they were shared with you.
and thanks for the spot-on reviews here.
There was a problem hiding this comment.
@cvan, are you continue playing with this. If not, I would prefer to merge this PR since the GA relies on it. What do you think?
There was a problem hiding this comment.
I've removed this file in favour of a remote JavaScript file that we can update as needed
| try { | ||
| var queue = JSON.parse(localStorage.getItem(offlineStorageKey)) || []; | ||
| queue.push(event.data); | ||
| localStorage.setItem(offlineStorageKey, JSON.stringify(queue)); |
There was a problem hiding this comment.
I'm worried about using synchronous storage here. If the application is offline and the application starts to error, the synchronous process would interfere with the developer debugging session. I would use asynchronous storage as IndexedDB with localForage and requestIdleCallback (if available) for a non-intrusive serialization. Until that moment, I recommend to not include this feature.
There was a problem hiding this comment.
yeah, I am too. fwiw, the Unity Engine already uses IndexedDB a ton and fires off several XHR to Unity services. and, yep, if we use IndexedDB, localForage is good, although we could probably get away with just the idb library.
thanks for bringing this up - very good call. I'll remove the offlineStoragePlugin function entirely until it's matured and merged properly in Raven.js core: getsentry/sentry-javascript#1165
| (function () { | ||
| // This checks for "production"-looking origins (e.g., `https://example.com`). | ||
| if (window.isSecureContext === false || | ||
| (location.hostname === 'localhost' || |
There was a problem hiding this comment.
I thought a pretty good check could also be checking if location.port is truthy, but it's possible for false positive
|
I'm looking into Rollbar possibly as an alternative to Sentry + Google Analytics. I'll keep you posted of my findings. Until then, I'll leave this PR open but marked as a WIP. |
|
@cvan, I can remove the plugin from your branch and get it merged, and continue editing this file to configure GA. Is that OK? |
feel free to do that if you'd like. my only concern is #174 (comment) I assume you're referring to hooks for only Mozilla GA? not the developer GA as well? like from issue #169: #169 (comment) assuming we do end up using Sentry plus Google Analytics (instead of Rollbar), does this snippet help at all for the stuff you're looking at? https://gist.github.com/cvan/78966124dd757c65e2f482b767052e44#file-ga-js |
Build/telemetry.js
Outdated
| @@ -0,0 +1,109 @@ | |||
| /* global localStorage, location, Raven */ | |||
There was a problem hiding this comment.
I'll remove the Build/ files
There was a problem hiding this comment.
Oh, right. Anyway, when I'm about to submit a version to the store, I rebuild the demo scene to align with that version.
|
@cvan please notice that I've forced a branch update to avoid |
cvan
changed the title
telemetry.js file; connect to Sentry via Raven.js to catch JavaScript errors (addresses issue #166, fixes issue #172)
No description provided.