Use node-raw-stacktrace to retrieve the structured stack traces #32

Merged
merged 8 commits into from Apr 7, 2013

Projects

None yet

2 participants

@azylman
Contributor
azylman commented Mar 12, 2013

Hey Matt,

Here's that PR I mentioned yesterday.

Note that one of these commits ("Simplify") is purely some style stuff that I was thinking about as I was going through this. If you don't like it, I can revert that commit.

Thanks!

@azylman
Contributor
azylman commented Mar 25, 2013

Have you had a chance to look at this yet, @mattrobenolt?

@mattrobenolt
Member

Sorry, I did take a look, but got distracted by all of the other simplifications you did. :) Do you mind bringing this diff down to just what is affected, and if you want, submit another PR for the cleanup.

@azylman azylman Revert "Simplify"
This reverts commit f1290bb.

Conflicts:
	lib/client.js
	lib/parsers.js
ee99656
@azylman
Contributor
azylman commented Mar 26, 2013

Np, sorry about that - done!

@mattrobenolt
Member

Awesome, much better. I will look through this today. :) Thanks.

@mattrobenolt
Member

@azylman Can you fix this up to be merged? It looks good to me. I'll merge it in and do some more functional testing on my end to make sure that it handles everything correctly.

I'm also curious to see if it fixes up #38 or #39 by any chance.

@azylman
Contributor
azylman commented Apr 5, 2013

It should be good to go now - I ran the test cases again and they're still passing.

@azylman
Contributor
azylman commented Apr 5, 2013

re: #38 and #39,

You can check to make sure someone else isn't overwriting Error.prepareStackTrace (a la Q) pretty easily after this change - checking that err.structuredStackTrace exists before sending it to parseStack. I had actually thought about doing that in this PR but 1) I wasn't aware of any other libraries that use Error.prepareStackTrace so wasn't sure it was worth it (though I am now aware of two - both Q and coffee-script do) and 2) wasn't sure how you'd want to handle that case so didn't want to do it without at least talking it over first.

@mattrobenolt
Member

Is structuredStackTrace a thing? Or did we just make that up? I haven't seen it before in any specs, granted none of this stuff is really a "spec". I'd feel more comfortable using a namespaced property, like: error.raven_structuredStackTrace, just to avoid the risk of collision.

@azylman
Contributor
azylman commented Apr 5, 2013

structuredStackTrace is the same variable name that V8 uses for this. We could definitely attach it to a different property on the error instance if you'd like, but I just named it that since that's what V8 calls it. Would you like me to change it to something namespaced?

@mattrobenolt
Member

Welp, here goes nothing.

@mattrobenolt mattrobenolt merged commit 7b8acaa into getsentry:master Apr 7, 2013

1 check passed

default The Travis build passed
Details
@mattrobenolt
Member

@azylman Mind checking out latest master? It appears good to me, just want another set of eyes on it for things you've been testing wiht just to double check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment