-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Sensible non-Error exception serializer #1253
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
var stringify = require('../vendor/json-stringify-safe/stringify'); | ||
|
||
var _window = | ||
typeof window !== 'undefined' | ||
? window | ||
|
@@ -441,6 +443,98 @@ function safeJoin(input, delimiter) { | |
return output.join(delimiter); | ||
} | ||
|
||
// Default Node.js REPL depth | ||
var MAX_SERIALIZE_EXCEPTION_DEPTH = 3; | ||
// 50kB, as 100kB is max payload size, so half sounds reasonable | ||
var MAX_SERIALIZE_EXCEPTION_SIZE = 50 * 1024; | ||
var MAX_SERIALIZE_KEYS_LENGTH = 40; | ||
|
||
function utf8Length(value) { | ||
return ~-encodeURI(value).split(/%..|./).length; | ||
} | ||
|
||
function jsonSize(value) { | ||
return utf8Length(JSON.stringify(value)); | ||
} | ||
|
||
function serializeValue(value) { | ||
var maxLength = 40; | ||
|
||
if (typeof value === 'string') { | ||
return value.length <= maxLength ? value : value.substr(0, maxLength - 1) + '\u2026'; | ||
} else if ( | ||
typeof value === 'number' || | ||
typeof value === 'boolean' || | ||
typeof value === 'undefined' | ||
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. any other value types here we could want... dates? 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. or are those objects? 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. Dates are treated as JSON.stringify(new Date('02/03/2017'))
// ""2017-02-02T23:00:00.000Z"" There are definitely some other types that we can add in the future, but right now, they'll be gracefully handled by |
||
) { | ||
return value; | ||
} | ||
|
||
var type = Object.prototype.toString.call(value); | ||
|
||
// Node.js REPL notation | ||
if (type === '[object Object]') return '[Object]'; | ||
if (type === '[object Array]') return '[Array]'; | ||
if (type === '[object Function]') | ||
return value.name ? '[Function: ' + value.name + ']' : '[Function]'; | ||
|
||
return value; | ||
} | ||
|
||
function serializeObject(value, depth) { | ||
if (depth === 0) return serializeValue(value); | ||
|
||
if (isPlainObject(value)) { | ||
return Object.keys(value).reduce(function(acc, key) { | ||
acc[key] = serializeObject(value[key], depth - 1); | ||
return acc; | ||
}, {}); | ||
} else if (Array.isArray(value)) { | ||
return value.map(function(val) { | ||
return serializeObject(val, depth - 1); | ||
}); | ||
} | ||
|
||
return serializeValue(value); | ||
} | ||
|
||
function serializeException(ex, depth, maxSize) { | ||
if (!isPlainObject(ex)) return ex; | ||
|
||
depth = typeof depth !== 'number' ? MAX_SERIALIZE_EXCEPTION_DEPTH : depth; | ||
maxSize = typeof depth !== 'number' ? MAX_SERIALIZE_EXCEPTION_SIZE : maxSize; | ||
|
||
var serialized = serializeObject(ex, depth); | ||
|
||
if (jsonSize(stringify(serialized)) > maxSize) { | ||
return serializeException(ex, depth - 1); | ||
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. 💯 |
||
} | ||
|
||
return serialized; | ||
} | ||
|
||
function serializeKeysForMessage(keys, maxLength) { | ||
if (typeof keys === 'number' || typeof keys === 'string') return keys.toString(); | ||
if (!Array.isArray(keys)) return ''; | ||
|
||
keys = keys.filter(function(key) { | ||
return typeof key === 'string'; | ||
}); | ||
if (keys.length === 0) return '[object has no keys]'; | ||
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 mentioned this in the test but I think this string could be improved a bit to be more declarative/official looking |
||
|
||
maxLength = typeof maxLength !== 'number' ? MAX_SERIALIZE_KEYS_LENGTH : maxLength; | ||
if (keys[0].length >= maxLength) return keys[0]; | ||
|
||
for (var usedKeys = keys.length; usedKeys > 0; usedKeys--) { | ||
var serialized = keys.slice(0, usedKeys).join(', '); | ||
if (serialized.length > maxLength) continue; | ||
if (usedKeys === keys.length) return serialized; | ||
return serialized + '\u2026'; | ||
} | ||
|
||
return ''; | ||
} | ||
|
||
module.exports = { | ||
isObject: isObject, | ||
isError: isError, | ||
|
@@ -470,5 +564,7 @@ module.exports = { | |
isSameStacktrace: isSameStacktrace, | ||
parseUrl: parseUrl, | ||
fill: fill, | ||
safeJoin: safeJoin | ||
safeJoin: safeJoin, | ||
serializeException: serializeException, | ||
serializeKeysForMessage: serializeKeysForMessage | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,11 +106,8 @@ describe('integration', function() { | |
}, | ||
function() { | ||
var ravenData = iframe.contentWindow.ravenData[0]; | ||
assert.isAtLeast(ravenData.stacktrace.frames.length, 1); | ||
assert.isAtMost(ravenData.stacktrace.frames.length, 3); | ||
|
||
// verify trimHeadFrames hasn't slipped into final payload | ||
assert.isUndefined(ravenData.trimHeadFrames); | ||
assert.isAtLeast(ravenData.exception.values[0].stacktrace.frames.length, 1); | ||
assert.isAtMost(ravenData.exception.values[0].stacktrace.frames.length, 3); | ||
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. is it correct to say that the old assertions would also have passed here? 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 checking my understanding of this change 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. It's behavior change some time ago afaik. We now always call |
||
} | ||
); | ||
}); | ||
|
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.
are you only using
~-
this for subtracting 1? I trust it's right, just want to understand.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.
Correct :) https://www.joezimjs.com/javascript/great-mystery-of-the-tilde/
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 not use
).length - 1
? (it's NBD, just curious)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.
Because I trust in every line of code that Substack writes tbh, nothing more - https://github.com/substack/utf8-length/blob/master/index.js :P