Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use []runtime.Frame instead of exporting Stack (#45) #52

Merged
merged 4 commits into from
Mar 5, 2019
Merged

Use []runtime.Frame instead of exporting Stack (#45) #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e78678e
Use []runtime.Frame instead of Stack
fgblomqvist Feb 14, 2019
60d9b98
Update documentation
fgblomqvist Feb 20, 2019
170922f
Revert traceChain nil slice declaration
fgblomqvist Feb 20, 2019
64643b0
Remove comment for no longer existent argument
fgblomqvist Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion rollbar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/http"
"os"
"regexp"
"runtime"
)

const (
Expand Down Expand Up @@ -523,5 +524,5 @@ func WrapAndWait(f func()) interface{} {
type CauseStacker interface {
error
Cause() error
Stack() Stack
Stack() []runtime.Frame
}
33 changes: 19 additions & 14 deletions rollbar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/http"
"os"
"runtime"
"testing"
)

Expand Down Expand Up @@ -361,14 +362,14 @@ func TestFilterFlatten(t *testing.T) {
type cs struct {
error
cause error
stack Stack
stack []runtime.Frame
}

func (cs cs) Cause() error {
return cs.cause
}

func (cs cs) Stack() Stack {
func (cs cs) Stack() []runtime.Frame {
return cs.stack
}

Expand All @@ -387,33 +388,36 @@ func TestGetCauseOfCauseStacker(t *testing.T) {
}

func TestGetOrBuildStackOfStdErrWithoutParent(t *testing.T) {
err := cs{fmt.Errorf(""), nil, BuildStack(0)}
if nil == getOrBuildStack(err, nil, 0) {
err := cs{fmt.Errorf(""), nil, getCallersFrames(0)}
if nil == getOrBuildFrames(err, nil, 0) {
t.Error("should build stack if parent is not a CauseStacker")
}
}

func TestGetOrBuildStackOfStdErrWithParent(t *testing.T) {
cause := fmt.Errorf("cause")
effect := cs{fmt.Errorf("effect"), cause, BuildStack(0)}
if 0 != len(getOrBuildStack(cause, effect, 0)) {
effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
if 0 != len(getOrBuildFrames(cause, effect, 0)) {
t.Error("should return empty stack of stadard error if parent is CauseStacker")
}
}

func TestGetOrBuildStackOfCauseStackerWithoutParent(t *testing.T) {
cause := fmt.Errorf("cause")
effect := cs{fmt.Errorf("effect"), cause, BuildStack(0)}
if effect.Stack()[0] != getOrBuildStack(effect, nil, 0)[0] {
effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
if len(effect.Stack()) == 0 {
t.Fatal("stack should not be empty")
}
if effect.Stack()[0] != getOrBuildFrames(effect, nil, 0)[0] {
t.Error("should use stack from effect")
}
}

func TestGetOrBuildStackOfCauseStackerWithParent(t *testing.T) {
cause := fmt.Errorf("cause")
effect := cs{fmt.Errorf("effect"), cause, BuildStack(0)}
effect2 := cs{fmt.Errorf("effect2"), effect, BuildStack(0)}
if effect.Stack()[0] != getOrBuildStack(effect2, effect, 0)[0] {
effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
effect2 := cs{fmt.Errorf("effect2"), effect, getCallersFrames(0)}
if effect2.Stack()[0] != getOrBuildFrames(effect2, effect, 0)[0] {
t.Error("should use stack from effect2")
}
}
Expand Down Expand Up @@ -441,8 +445,8 @@ func TestErrorBodyWithoutChain(t *testing.T) {

func TestErrorBodyWithChain(t *testing.T) {
cause := fmt.Errorf("cause")
effect := cs{fmt.Errorf("effect1"), cause, BuildStack(0)}
effect2 := cs{fmt.Errorf("effect2"), effect, BuildStack(0)}
effect := cs{fmt.Errorf("effect1"), cause, getCallersFrames(0)}
effect2 := cs{fmt.Errorf("effect2"), effect, getCallersFrames(0)}
errorBody, fingerprint := errorBody(configuration{fingerprint: true}, effect2, 0)
if nil != errorBody["trace"] {
t.Error("should not have trace element")
Expand All @@ -463,7 +467,8 @@ func TestErrorBodyWithChain(t *testing.T) {
if "cause" != traces[2]["exception"].(map[string]interface{})["message"] {
t.Error("chain should contain cause third")
}
if effect2.Stack().Fingerprint()+effect.Stack().Fingerprint()+"0" != fingerprint {

if buildStack(effect2.Stack()).Fingerprint()+buildStack(effect.Stack()).Fingerprint()+"0" != fingerprint {
t.Error("fingerprint should be the fingerprints in chain concatenated together. got: ", fingerprint)
}
}
38 changes: 16 additions & 22 deletions stack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ var (
}
)

// Frame represents one frame in a stack trace
type Frame struct {
// frame represents one frame in a stack trace
type frame struct {
// Filename is the name of the file for this frame
Filename string `json:"filename"`
// Method is the name of the method for this frame
Expand All @@ -27,21 +27,17 @@ type Frame struct {
Line int `json:"lineno"`
}

// A Stack is a slice of frames
type Stack []Frame
// A stack is a slice of frames
type stack []frame

// BuildStack uses the Go runtime to construct a slice of frames optionally skipping the number of
// frames specified by the input skip argument.
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get rid of the "optionally skipping ..." as we are not taking a skip argument anymore.

func buildStack(frames []runtime.Frame) stack {
stack := make(stack, len(frames))

for i := skip; ; i++ {
pc, file, line, ok := runtime.Caller(i)
if !ok {
break
}
file = shortenFilePath(file)
stack = append(stack, Frame{file, functionName(pc), line})
for i, fr := range frames {
file := shortenFilePath(fr.File)
stack[i] = frame{file, functionName(fr.Function), fr.Line}
}

return stack
Expand All @@ -51,7 +47,7 @@ func BuildStack(skip int) Stack {
// callstack, including file names. That ensure that there are no false duplicates
// but also means that after changing the code (adding/removing lines), the
// fingerprints will change. It's a trade-off.
func (s Stack) Fingerprint() string {
func (s stack) Fingerprint() string {
hash := crc32.NewIEEE()
for _, frame := range s {
fmt.Fprintf(hash, "%s%s%d", frame.Filename, frame.Method, frame.Line)
Expand Down Expand Up @@ -80,12 +76,10 @@ func shortenFilePath(s string) string {
return s
}

func functionName(pc uintptr) string {
fn := runtime.FuncForPC(pc)
if fn == nil {
func functionName(pcFuncName string) string {
if pcFuncName == "" {
return "???"
}
name := fn.Name()
end := strings.LastIndex(name, string(os.PathSeparator))
return name[end+1:]
end := strings.LastIndex(pcFuncName, string(os.PathSeparator))
return pcFuncName[end+1:]
}
19 changes: 10 additions & 9 deletions stack_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import (
)

func TestBuildStack(t *testing.T) {
frame := BuildStack(1)[0]
frame := buildStack(getCallersFrames(0))[0]

if !strings.HasSuffix(frame.Filename,"rollbar-go/stack_test.go") {
t.Errorf("got: %s", frame.Filename)
}
Expand All @@ -21,25 +22,25 @@ func TestBuildStack(t *testing.T) {
func TestStackFingerprint(t *testing.T) {
tests := []struct {
Fingerprint string
Stack Stack
Stack stack
}{
{
"9344290d",
Stack{
Frame{"foo.go", "Oops", 1},
stack{
frame{"foo.go", "Oops", 1},
},
},
{
"a4d78b7",
Stack{
Frame{"foo.go", "Oops", 2},
stack{
frame{"foo.go", "Oops", 2},
},
},
{
"50e0fcb3",
Stack{
Frame{"foo.go", "Oops", 1},
Frame{"foo.go", "Oops", 2},
stack{
frame{"foo.go", "Oops", 1},
frame{"foo.go", "Oops", 2},
},
},
}
Expand Down
40 changes: 27 additions & 13 deletions transforms.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/url"
"reflect"
"regexp"
"runtime"
"strings"
"time"
)
Expand Down Expand Up @@ -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{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fingerprint := ""
for {
stack := getOrBuildStack(err, parent, skip)
stack := buildStack(getOrBuildFrames(err, parent, 1 + skip))
traceChain = append(traceChain, buildTrace(err, stack))
if configuration.fingerprint {
fingerprint = fingerprint + stack.Fingerprint()
Expand All @@ -210,7 +211,7 @@ func errorBody(configuration configuration, err error, skip int) (map[string]int
}

// builds one trace element in trace_chain
func buildTrace(err error, stack Stack) map[string]interface{} {
func buildTrace(err error, stack stack) map[string]interface{} {
message := nilErrTitle
if err != nil {
message = err.Error()
Expand All @@ -231,20 +232,33 @@ func getCause(err error) error {
return nil
}

// gets Stack from errors that provide one of their own
// otherwise, builds a new stack
func getOrBuildStack(err error, parent error, skip int) Stack {
// gets stack frames from errors that provide one of their own
// otherwise, builds a new stack trace
func getOrBuildFrames(err error, parent error, skip int) []runtime.Frame {
if cs, ok := err.(CauseStacker); ok {
if s := cs.Stack(); s != nil {
return s
}
} else {
if _, ok := parent.(CauseStacker); !ok {
return BuildStack(4 + skip)
return cs.Stack()
} else if _, ok := parent.(CauseStacker); !ok {
return getCallersFrames(1 + skip)
}

return nil
}

func getCallersFrames(skip int) []runtime.Frame {
pc := make([]uintptr, 100)
runtime.Callers(2 + skip, pc)
fr := runtime.CallersFrames(pc)
frames := make([]runtime.Frame, 0)

for frame, more := fr.Next(); frame != (runtime.Frame{}); frame, more = fr.Next() {
frames = append(frames, frame)

if !more {
break
}
}

return make(Stack, 0)
return frames
}

// Build a message inner-body for the given message string.
Expand Down