feat: Sensible non-Error exception serializer #1253
Conversation
kamilogorek
force-pushed
the
sensible-error-serializer
branch
from
0ab8c75 to
c10432c
Compare
|
this is a big change so I want to give it a proper review and test drive it |
| }; | ||
|
|
||
| // Unfortunately older browsers are not capable of extracting method names | ||
| // therefore we have to use `oneOf` here |
test/utils.test.js
Outdated
| assert.equal(actual, expected); | ||
| }); | ||
|
|
||
| it('shouldnt append ellipsis if have enough space', function() { |
There was a problem hiding this comment.
maybe a test case for when you use exactly the limit? (it shouldn't ellipsize, but this isn't a big deal)
There was a problem hiding this comment.
it looks like you handle this right in the logic though
|
|
||
| it('should default to no-keys message if empty array provided', function() { | ||
| var actual = serializeKeysForMessage([]); | ||
| var expected = '[object has no keys]'; |
There was a problem hiding this comment.
what do you think about "[Empty Object]" or "{}"?
There was a problem hiding this comment.
I thought about it, but I think it may be slightly confusing, as it looks like something coming from the browser itself and people may start to google for it :D
There was a problem hiding this comment.
[Object (no keys)]
[Object with no keys]
?
I don't know what kind of precedent we could follow here, but I think people might google for it either way (and ideally they'd find this repo?). I don't think this is that important since it shouldn't be super common, I'm just throwing out other options
There was a problem hiding this comment.
Let's leave it like this for now, as Raven-node is already using this string and change it if we'll get any reports of it being confusing :)
There was a problem hiding this comment.
oh, i didn't realize raven-node uses it already, that's good enough for me
test/utils.test.js
Outdated
| assert.equal(actual, expected); | ||
| }); | ||
|
|
||
| it('should with with provided maxLength', function() { |
|
|
||
| it('should serialize arbitrary number of depths', function() { | ||
| var actual = serializeException( | ||
| { |
There was a problem hiding this comment.
how does this deal with cyclic objects? (if it just keeps following the links until it bottoms out, that's not that bad)
There was a problem hiding this comment.
That's exactly what it does. And MAX_SERIALIZE_EXCEPTION_DEPTH is set to 3, so the only way to "break" it is consciously calling serializeException(something, 10e100) or some other very large number that will take too much memory to calculate :p
And it's only in serializeObject, as for checking the size itself, we use safe-json-stringify which we use for Raven, which handles cyclic objects itself.
src/raven.js
Outdated
| // This will allow us to group events based on top-level keys | ||
| // which is much better than creating new group when any key/value change | ||
|
|
||
| var keys = Object.keys(ex).sort(); |
There was a problem hiding this comment.
maybe this logic should go in its own function? only because captureException is so central and might be the first thing someone reading the code would encounter
| var MAX_SERIALIZE_KEYS_LENGTH = 40; | ||
|
|
||
| function utf8Length(value) { | ||
| return ~-encodeURI(value).split(/%..|./).length; |
There was a problem hiding this comment.
are you only using ~- this for subtracting 1? I trust it's right, just want to understand.
There was a problem hiding this comment.
There was a problem hiding this comment.
why not use ).length - 1? (it's NBD, just curious)
There was a problem hiding this comment.
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
| } else if ( | ||
| typeof value === 'number' || | ||
| typeof value === 'boolean' || | ||
| typeof value === 'undefined' |
There was a problem hiding this comment.
any other value types here we could want... dates?
There was a problem hiding this comment.
Dates are treated as object with the representation of [object Date]. It's taken care of by JSON.stringify which extracts the date from it
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 stringify.
| var serialized = serializeObject(ex, depth); | ||
|
|
||
| if (jsonSize(stringify(serialized)) > maxSize) { | ||
| return serializeException(ex, depth - 1); |
| 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.
i mentioned this in the test but I think this string could be improved a bit to be more declarative/official looking
|
Thanks, that's a great review @MaxBittker! How about |
|
I think you improved captureException and it could be merged in its current form, but I took a crack at another layout for the function that reduces some of the logic and might be better: https://github.com/getsentry/raven-js/pull/1259/files |
|
Duuuuh! That's much more clever! Thank you :D |
|
Thanks, @MaxBittker! I'll release it tomorrow though, as I don't want to put out an eventual fire at night :P |
|
agreed 100% on not releasing it tonight |
getsentry/raven-node#416 adapted to Raven.js needs.
Should help to fix most of the issues with logging a large amount of
[object Object]events.