Integrate Raven-JS for error logging to sentry #387
Conversation
|
Also I want to mention that this does two things:
It's left up to the developer to provide a richer set of logging to their code when they want messages sent to Sentry through Zamboni. We provide a wrapper called raven_proxy. For instance if you want to log a failed ajax transaction you might do something like: require(['raven-proxy'], function (raven_proxy) { |
| require(['settings'], function (settings) { | ||
| Raven.config(settings.zamboni_raven_url, {}).install(); | ||
|
|
||
| window.onerror = function(errorMsg, url, lineNumber) { |
There was a problem hiding this comment.
Probably a good idea to explicitly return false to ensure the default handling happens.
|
Might be nice to make the inclusion of this based on config too, just so it's easy to turn on or off for each env. |
| # Please note that the leading 'none@' is required | ||
| # The trailing integer '12345' is the sentry account id which is found in a sentry DSN | ||
| # ex: https://<user>:<pass>@app.getsentry.com/<sentry_account_id> | ||
| 'zamboni_raven_url': 'TODO', |
There was a problem hiding this comment.
This means that by default raven will be enabled, but fail?
Would prefer an empty string and if there is an empty string, skip sending to raven.
We'll need to change this on a per server basis so add it to https://github.com/mozilla/webpay/tree/master/webpay/settings/sites
|
I've modified our proxy to check whether it's been initialized and provided a wrapper. |
|
Could you put the bug number in the commit messages first line please, usually (bug xxxxxx) on the end. |
|
Done. |
| } | ||
| } | ||
|
|
||
| // Define an includable reference to the proxy |
There was a problem hiding this comment.
You could wrap this file in the define rather than having the require and then an inner define.
There was a problem hiding this comment.
Yeah I tried that but it seems that the inner body of a define is only evaluated when it's included in another file that requires it, so Raven will not be initialized and bound to window.onerror at page load by default (say if it's not specifically required by another module).
It's handling two cases, one is to automatically initialize and bind to the global window handler, and the other is to provide a manually invokable proxy. Is there some better way to organize it that satisfies both of those cases?
There was a problem hiding this comment.
You could require it here: js/pay/pay.js that's sort of like the main file. This will be a lot cleaner/clearer in spartacus.
|
camelCased! |
|
|
||
| Raven.captureMessage(message, options); | ||
| } | ||
| } |
|
As mentioned at the work week after thinking on it and looking at the Raven source code I'm not sure we should use our own client instead of Raven.js because it has a bunch of potentially useful features see http://raven-js.readthedocs.org/en/latest/config/index.html#optional-settings The Raven client uses a GET because it's using image beacons (via Image objects) to poke stacktraces into sentry (albeit in our case we're using a mirror) so we're not needing to workaround the same origin policy that most people would if they're sending errors to sentry directly. When this lands we'll want to get QA eyes on the webpay flow just look out for any problems with the implementation. (Add qa+ to the whiteboard in bugzilla) |
|
Would upstreaming a patch to raven-js that allows us to do GET or POSTs be an option? |
Could try that - though my feeling is they might see adding support for POST as a bit of an edge-case, but I could be wrong. |
|
Looks like there's an existing patch started for this: getsentry/sentry-javascript#89 |
|
And it says he'd consider it if someone made it configurable. So we've just go to do that... :) |
- Depends on ravenjs - Sends a msg to Zamboni to proxy error messages to sentry through the VPN - Binds to window.onerror but allows for custom logging using Raven.captureMessage(msg)
|
r+ from me |
|
r+ lgtm |
Integrate Raven-JS for error logging to sentry
|
Let me know if there's anything I can do to help! With regard to POSTs, my hunch is that this is the direction we're going to go: getsentry/sentry-javascript#183 These pluggable transports will work similar to the way they work in raven-python, for example, that way it can be treated as a plugin and not included in core. This way you could register a DSN like: Either way, I'd gladly accept upstream patches! |
|
@mattrobenolt that sounds great! |
Refs #942912