Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
Support options in captureException and captureMessage#38
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
d355d9ff1cb857fd417960cddd5fbfae481c1c872774b1457883cb638f5e791e3105b6File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| test/ | ||
| dist/ | ||
| src/vendor/ | ||
| phantom-js-loader.js |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "strict": true, | ||
| "browser": true, | ||
| "predef": [ | ||
| "parseUri" | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -28,5 +28,6 @@ raven: | ||
| mv ${TMP} ${RAVEN_MIN} | ||
| test: | ||
| jshint . | ||
| phantomjs phantom-js-loader.js | ||
| phantomjs phantom-js-loader.js zepto | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -9,6 +9,8 @@ | ||
| (function(){ | ||
| // Save a reference to the global object (`window` in the browser, `global` | ||
| // on the server). | ||
| "use strict"; | ||
| var root = this; | ||
| var Raven; | ||
| @@ -61,14 +63,14 @@ | ||
| }); | ||
| // Expand server base URLs into API URLs | ||
| $.each(self.options['servers'], function(i, server) { | ||
| $.each(self.options.servers, function(i, server) { | ||
| // Add a trailing slash if one isn't provided | ||
| if (server.slice(-1) !== '/') { | ||
| server += '/'; | ||
| } | ||
| servers.push(server + 'api/' + self.options['projectId'] + '/store/'); | ||
| servers.push(server + 'api/' + self.options.projectId + '/store/'); | ||
| }); | ||
| self.options['servers'] = servers; | ||
| self.options.servers = servers; | ||
| }; | ||
| @@ -102,7 +104,7 @@ | ||
| .getAllResponseHeaders(); | ||
| } | ||
| headers["Referer"] = document.referrer; | ||
| headers.Referer = document.referrer; | ||
| headers["User-Agent"] = navigator.userAgent; | ||
| return headers; | ||
| }; | ||
| @@ -145,8 +147,8 @@ | ||
| return header; | ||
| }; | ||
| Raven.captureException = function(e) { | ||
| var lineno, traceback, fileurl; | ||
| Raven.captureException = function(e, options) { | ||
| var label, lineno, fileurl, traceback; | ||
| if (e.line) { // WebKit | ||
| lineno = e.line; | ||
| @@ -160,25 +162,85 @@ | ||
| fileurl = e.fileName; | ||
| } | ||
| if (e.arguments && e.stack) { | ||
| traceback = this.chromeTraceback(e); | ||
| if (e["arguments"] && e.stack) { | ||
| traceback = self.chromeTraceback(e); | ||
| } else if (e.stack) { | ||
| // Detect edge cases where Chrome doesn't have 'arguments' | ||
| if (e.stack.indexOf('@') == -1) { | ||
| traceback = this.chromeTraceback(e); | ||
| traceback = self.chromeTraceback(e); | ||
| } else { | ||
| traceback = this.firefoxOrSafariTraceback(e); | ||
| traceback = self.firefoxOrSafariTraceback(e); | ||
| } | ||
| } else { | ||
| traceback = [{"filename": fileurl, "lineno": lineno}]; | ||
| traceback = traceback.concat(this.otherTraceback(arguments.callee)); | ||
| traceback = traceback.concat(self.otherTraceback(Raven.captureException)); | ||
| } | ||
| self.process(e, fileurl, lineno, traceback); | ||
| self.process(e, fileurl, lineno, traceback, options); | ||
| }; | ||
| Raven.captureMessage = function(msg) { | ||
| self.process(msg); | ||
| Raven.captureMessage = function(msg, options) { | ||
| var data = self.arrayMerge({ | ||
| 'message': msg | ||
| }, options); | ||
| self.send(data); | ||
| }; | ||
| Raven.process = function(message, fileurl, lineno, traceback, options) { | ||
| var type, stacktrace, label, data; | ||
| if (typeof(message) === 'object') { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could return misleading results. typeof(null)==="object"typeof([])==="object"perhaps you'd want to do something like: if(message["message"]!==undefined&&message["name"]!==undefined){///...Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Underscore has a proper Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no underscore. :( Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, yeah – recanted. | ||
| type = message.name; | ||
| message = message.message; | ||
| } | ||
| if ($.inArray(message, self.options.ignoreErrors) >= 0) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to play devils advocate here, rather than using jquery to do native JS things -- something like this? if(self.options.ignoreErrors.indexOf(message)>-1){//...ex: ["foo","bar","baz"].indexOf("bar")===1["foo","bar","baz"].indexOf("snazz")===-1Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just do something like: for(vari=0,j=self.options.ignoreErrors;i<j;i++){if(self.options.ignoreErrors[i]===message)return;}Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🍰 good catch Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifically IE8 and below doesn't support it: | ||
| return; | ||
| } | ||
| if (traceback) { | ||
| stacktrace = {"frames": traceback}; | ||
| fileurl = fileurl || traceback[0].filename; | ||
| } else if (fileurl) { | ||
| stacktrace = { | ||
| "frames": [{ | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is a style choice or not, but generally we don't quote object keys: | ||
| "filename": fileurl, | ||
| "lineno": lineno | ||
| }] | ||
| }; | ||
| } | ||
| for (var i = 0; i < self.options.ignoreUrls.length; i++) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. js convention is to set a variable for the length as well so it's not calculated on each iteration, like:
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good interpreters optimize this for you. I would never optimize something like this unless it came up in a profiler. Here's a presentation from a Chrome developer in which he explains how this case is optimized in V8: http://s3.mrale.ph/jsconf2012.pdf Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does everyone else hoist Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. however, since there is a desire for supporting interpreters that aren't as awesome as V8 (looking at you IE), there's no reason to be lazy about it. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. http://jsperf.com/caching-array-length/123 It's all basically the same. /me shrugs I just do it this way out of habit now. Maybe I can stop it. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. O_o Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually (digressing a bit), wouldn't the interpreter have to actually check .length on every iteration since there's always a chance that the array was mutated while being iterated. If it's cached, it definitely doesn't have to keep checking. In Chrome for, there's about a 1M ops/s difference, but really, this is bikeshedding. It's a ~10% difference on something that is called once on an array of a few items. :) Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this version has IE9 in the test results as well: http://jsperf.com/caching-array-length/120 and it only reports a marginal improvement between hoisting and not. although decrementing instead of incrementing is nearly twice as fast. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. I believe V8 looks ahead to see if the loop will I recommend watching the video: On Thu, Nov 29, 2012 at 12:58 PM, Matt Robenolt notifications@github.comwrote:
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, he made a point that it only does certain optimizations when "under pressure". And he slapped a while (true) in front of the function call, which makes sense. No need to prematurely optimize if it doesn't matter. | ||
| if (self.options.ignoreUrls[i].test(fileurl)) { | ||
| return; | ||
| } | ||
| } | ||
| label = lineno ? message + " at " + lineno : message; | ||
| data = self.arrayMerge({ | ||
| "sentry.interfaces.Exception": { | ||
| "type": type, | ||
| "value": message | ||
| }, | ||
| "sentry.interfaces.Stacktrace": stacktrace, | ||
| "culprit": fileurl, | ||
| "message": label | ||
| }, options); | ||
| self.send(data); | ||
| }; | ||
| Raven.arrayMerge = function(arr1, arr2) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming thing: should probably be called objectMerge. and by the way you're using it you could probably have just used MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our goal is to kill the zepto/jquery requirement since the ajax/each code should be minor, and we dont need dom/etc Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, I just assumed you were okay with it since you're using Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not ok with Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then I'd settle for something like: for(varkinobj2){if(obj2.hasOwnProperty(k){obj1[k]=obj2[k];}}returnobj1;Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A generic functioneach(obj,callback){vari,j;if(obj.length===undefined){for(iinobj){if(obj.hasOwnProperty(i)){callback.call(null,i,obj[i]);}}}else{for(i=0,j=obj.length;i<j;i++){callback.call(null,i,obj[i]);}}}I have this in my no-jQuery branch. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why you'd bother with removing jQuery/Zepto as a requirement. What's the harm? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it's only really needed to facilitate the XHR stuff. It's not used for anything else except $.each, which is super easy to include our own. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, right, this is a Sentry client. For some reason I thought I was reviewing a patch for Sentry itself. | ||
| if (typeof(arr2) === "undefined") { | ||
| return arr1; | ||
| } | ||
| $.each(arr2, function(key, value){ | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to nip this | ||
| arr1[key] = value; | ||
| }); | ||
| return arr1; | ||
| }; | ||
| Raven.trimString = function(str) { | ||
| @@ -306,9 +368,9 @@ | ||
| traceback = [], | ||
| max = 9; | ||
| while (callee && traceback.length < max) { | ||
| fn = callee.name || (this.funcNameRE.test(callee.toString()) ? RegExp.$1 || ANON : ANON); | ||
| if (callee.arguments) { | ||
| args = this.stringifyArguments(callee.arguments); | ||
| fn = callee.name || (self.funcNameRE.test(callee.toString()) ? RegExp.$1 || ANON : ANON); | ||
| if (callee["arguments"]) { | ||
| args = self.stringifyArguments(callee["arguments"]); | ||
| } else { | ||
| args = undefined; | ||
| } | ||
| @@ -359,61 +421,66 @@ | ||
| return results; | ||
| }; | ||
| Raven.process = function(message, fileurl, lineno, traceback, timestamp) { | ||
| var label, stacktrace, data, encoded_msg, type, | ||
| url = root.location.protocol + '//' + root.location.host + root.location.pathname, | ||
| querystring = root.location.search.slice(1); // Remove the ? | ||
| Raven.getUTCNow = function() { | ||
| var now = new Date(); | ||
| return new Date(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate(), | ||
| now.getUTCHours(), now.getUTCMinutes(), now.getUTCSeconds()); | ||
| }; | ||
| if (typeof(message) === 'object') { | ||
| type = message.name; | ||
| message = message.message; | ||
| Raven.pad = function(n, amount) { | ||
| var i, | ||
| len = ('' + n).length; | ||
| if (typeof(amount) === "undefined") { | ||
| amount = 2; | ||
| } | ||
| if ($.inArray(message, self.options.ignoreErrors) >= 0) { | ||
| return; | ||
| if (len >= amount) { | ||
| return n; | ||
| } | ||
| for (var i = 0; i < self.options.ignoreUrls.length; i++) { | ||
| if (self.options.ignoreUrls[i].test(fileurl)) { | ||
| return; | ||
| } | ||
| for (i=0; i < (amount - len); i++) { | ||
| n = '0' + n; | ||
| } | ||
| return n; | ||
| }; | ||
| label = lineno ? message + " at " + lineno : message; | ||
| if (traceback) { | ||
| stacktrace = {"frames": traceback}; | ||
| fileurl = fileurl || traceback[0].filename; | ||
| } else if (fileurl) { | ||
| stacktrace = { | ||
| "frames": [{ | ||
| "filename": fileurl, | ||
| "lineno": lineno | ||
| }] | ||
| }; | ||
| Raven.dateToISOString = function(date) { | ||
| if (Date.prototype.toISOString) { | ||
| return date.toISOString(); | ||
| } | ||
| return date.getUTCFullYear() + '-' + | ||
| self.pad(date.getUTCMonth() + 1) + '-' + | ||
| self.pad(date.getUTCDate()) + 'T' + | ||
| self.pad(date.getUTCHours()) + ':' + | ||
| self.pad(date.getUTCMinutes()) + ':' + | ||
| self.pad(date.getUTCSeconds()) + '.' + | ||
| self.pad(date.getUTCMilliseconds(), 3) + 'Z'; | ||
| }; | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👎 Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I wrote that comment (deleted), I wasn't aware this was a Sentry client. My bad. | ||
| data = { | ||
| "message": label, | ||
| "culprit": fileurl, | ||
| "sentry.interfaces.Stacktrace": stacktrace, | ||
| "sentry.interfaces.Exception": { | ||
| "type": type, | ||
| "value": message | ||
| }, | ||
| Raven.send = function(data) { | ||
| var encoded_msg, | ||
| timestamp= new Date().getTime(), | ||
| url = root.location.protocol + '//' + root.location.host + root.location.pathname, | ||
| querystring = root.location.search.slice(1); // Remove the ? | ||
| data = self.arrayMerge({ | ||
| "project": self.options.projectId, | ||
| "logger": self.options.logger, | ||
| "site": self.options.site | ||
| }; | ||
| "site": self.options.site, | ||
| "timestamp": self.getUTCNow(), | ||
| "sentry.interfaces.Http": { | ||
| "url": url, | ||
| "querystring": querystring, | ||
| "headers": self.getHeaders() | ||
| } | ||
| }, data); | ||
| data["sentry.interfaces.Http"] = { | ||
| "url": url, | ||
| "querystring": querystring, | ||
| "headers": self.getHeaders() | ||
| }; | ||
| if (typeof(self.options.dataCallback) == 'function') { | ||
| data = self.options.dataCallback(data); | ||
| } | ||
| if (typeof(self.options.dataCallback) == 'function') data = self.options.dataCallback(data); | ||
| data.timestamp = self.dateToISOString(data.timestamp); | ||
| timestamp = timestamp || (new Date()).getTime(); | ||
| encoded_msg = JSON.stringify(data); | ||
| self.getSignature(encoded_msg, timestamp, function(signature) { | ||
| var header = self.getAuthHeader(signature, timestamp); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -29,6 +29,8 @@ $(document).ready(function() { | ||
| } | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changing this section of the test to something like the following proves that you have leaky variables. test("should collect error information and report to Sentry",function(){varisSupported,frame,caughtErr;try{giveMeAnError();}catch(err){caughtErr=err;Raven.captureException(err);ok(typeof(label)==="undefined");ok(typeof(data)==="undefined");if(err.stack){isSupported=true;}else{isSupported=false;}}equal(ajax_calls.length,1);vardata=JSON.parse(ajax_calls[0].data);// ... | ||
| } | ||
| equal(ajax_calls.length, 1); | ||
| data = JSON.parse(ajax_calls[0].data); | ||
| equal(data.logger, 'javascript', | ||
| @@ -43,18 +45,18 @@ $(document).ready(function() { | ||
| 'the culprit should be the exception.js unit test file'); | ||
| frame = data['sentry.interfaces.Stacktrace'].frames[0]; | ||
| equal(frame.function, 'outlandishClaim'); | ||
| equal(frame["function"], 'outlandishClaim'); | ||
| equal(frame.lineno, '7'); | ||
| // if the browser provides the arguments in the error | ||
| // verify they were parsed | ||
| if (caughtErr.stack.indexOf("I am Batman") !== -1) { | ||
| equal(frame.vars.arguments[0], '"I am Batman"'); | ||
| equal(frame.vars.arguments[1], '"Seriously"'); | ||
| equal(frame.vars["arguments"][0], '"I am Batman"'); | ||
| equal(frame.vars["arguments"][1], '"Seriously"'); | ||
| } | ||
| frame = data['sentry.interfaces.Stacktrace'].frames[1]; | ||
| equal(frame.function, 'giveMeAnError'); | ||
| equal(frame["function"], 'giveMeAnError'); | ||
| equal(frame.lineno, '3'); | ||
| } | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the switch from
e.argumentstoe["arguments"]?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixes the linter in my editor :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this is because
argumentsis a reserved word, so it needs to be accessed with []