Reverse stack traces

[pieter]

We were sending stack traces in the wrong order. Using
unshift instead of push makes sure we sent them in the
correct order.

Fixes #86

[mattrobenolt]

You can change how it's displayed in Sentry. The way that Raven ships it is correct. https://app.getsentry.com/account/settings/appearance/

And if Sentry is displaying it incorrect by default, it's a thing for Sentry.

[pieter]

No it's not, look at the screenshot in #86. It says "most recent call first" but shows the last call first. It's nothing to do with the appearance setting.

Either Raven-JS is sending it in the wrong order, or the string in Sentry is incorrect and should be reversed.

[bisrael]

I put some console.logs in right where you change the push to unshift, and the stack traces raven creates appear correct with push. at least as far as the array is concerned.

Are we sure that sentry does not want the stack trace sent in reverse order though?

The sentry docs are rather unhelpful, especially where the protocol is concerned.

[mattrobenolt]

I'm pretty sure I'm doing this backwards based on how it's expected to be received in Sentry. I think this should and can very easily be handled properly on Sentry automatically. Each language has a different "natural" order, and that natural order can and should be preserved. So as of now, since it's literally just a slightly confusing use of language, I'm going to keep this closed, and propose a solution for Sentry instead.

[pieter]

You really shouldn't go that route. You may not like it, but the current API is just "stacktraces are backwards from JS". You can't just change that in Sentry, you'll break clients that currently do the right thing. It'll be impossible for clients to always do the right thing, since they can't just guess the version of Sentry and then send stack traces accordingly. You're basically introducing a new major API version that is incompatible with older versions.

Sure, you could add another parameter to sentry &stacktrace_order=naturaly or similar, but really, what's the point? It's much easier to just let Raven-JS send the traces in the right order. You won't need a new sentry version, you won't break other clients, you don't need a fuzzy stacktrace-order-according-to-language resolver.

If you're really concerned about performance of unshift(), just call reverse() at the end. But it won't matter for the max 100-ish element arrays you'll get here.

Also, the language thing might be more confusing than you think. The order preference is per account, so if you have both a Python and a JS project in your account, one of them will show incorrect and the other will show correct, whatever option you choose. It's not just a misleading string, it's really wrong data we're sending here.

[acdha]

I'm with @pieter on not feeling unshift() performance is worth optimizing for – it's probably better to reverse() since it's so easy but for any plausible stacktrace size it's just not worth taking on additional complexity for.

[mattrobenolt]

Yeah, I'm definitely not against it because of unshift().

@pieter, the current language stuff in Sentry is a hack. It was a hack to hold over until we have proper language support. Sentry already knows which language each event is, so it'd be easy to apply certain rules per language.

I already have a patch waiting that will start rendering them with language specific stack traces so everything doesn't look like Python.

The next step is just handling the stack order per language.

The point being, it's not something that I want to solve just for raven-js. It should be solved for all languages, and it'd be very simple to do in Sentry itself. The concept of determining what language an event is has already been done.

By doing this, I'm really not worried about breaking Sentry or needing to force someone to upgrade. Really, this is the first that it has ever been brought up from any users, including myself, and nobody has noticed that it was wrong in the first place. :) The data is there, it's just backwards. So I'd hardly say that people should be required to upgrade.