Use []runtime.Frame instead of exporting Stack (#45) #52
Conversation
|
@fgblomqvist thanks for this too. We'll review shortly. |
Stack is now stack and is solely used to convert []runtime.Frame into a json serializable struct.
|
Rebased the PR since #51 got merged. |
|
If we are making rollbar.go doc.go |
transforms.go
Outdated
| @@ -191,10 +192,10 @@ func filterIp(ip string, captureIp captureIp) string { | |||
| // method, the causes will be traversed until nil. | |||
| func errorBody(configuration configuration, err error, skip int) (map[string]interface{}, string) { | |||
| var parent error | |||
| traceChain := []map[string]interface{}{} | |||
| var traceChain []map[string]interface{} | |||
There was a problem hiding this comment.
Sorry it was just a reflex to change that due to my IDE complaining about declaring an empty slice with a literal. I believe it's considered a good convention to use nil declarations (https://stackoverflow.com/a/29164565/998919), but by all means, I can revert it if this project prefers the other (previous) style.
There was a problem hiding this comment.
I guess that we always append something to it so this change is okay. The one difference is that for JSON serialization the nil slice becomes null and the empty slice becomes [] and the backend will treat those differently.
There was a problem hiding this comment.
True. I did pay attention to this in at least one other spot, to make sure we get [] in the JSON. Hmm I do feel now that the benefit of leaving that line as-is (e.g. what it originally was) outweighs the benefit of not allocating it (since we do "always" append to it, assuming no errors occur). I'm thinking if the code changes down the line it might become a frustrating error in case a nil slice slips through to the JSON marshaling. I'm gonna go ahead and revert it if that's fine.
|
@rokob I'll do the docs changes (to the best of my knowledge) and push up in a moment, thanks for the suggestion |
stack.go
Outdated
| func BuildStack(skip int) Stack { | ||
| stack := make(Stack, 0) | ||
| // buildStack converts []runtime.Frame into a JSON serializable slice of frames | ||
| // optionally skipping the number of frames specified by the input skip argument. |
There was a problem hiding this comment.
You can get rid of the "optionally skipping ..." as we are not taking a skip argument anymore.
|
@rokob Thanks, done. |
|
Too late now I'd say, but this should have been released as a major version bump as it breaks previously compiling code. |
|
@mmlb It should. I take part of the blame (I should have suggested that), but yeah, too late now. |
|
🤷♂️ |
|
Looking forward to updating to this code though :D |
This is my proposed solution to issue #45. I have tried to preserve as much as possible of the existing code structure to make it a less impactful change, but naturally it will be a breaking one no matter what. I concluded that it made the most sense to use
[]runtime.Frameinstead of e.g.runtime.Framesor[]uintptr. A runtime.Frame essentially holds the same information as the oldFrame, with some additions.With this change in place, it will be easier to extend the package to be able to get stack traces from other error interfaces, as well as to be able to take a stack trace straight (e.g. similar to what https://github.com/stvp/roll allows for).
While the tests are passing, I have yet to test it during some real usage. I'm planning on doing that over the next few days/week, and will report back once I know how it went (assuming this PR is still of interest).
For now, it depends on #51 simply because that's the kind of environment I've worked in. If that one isn't of interest, it shouldn't be hard to just rebase this onto master (and is something that I'll of course do in that case).
Open to feedback if there is any change/piece of code that isn't up to par/to your liking! :)