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.

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

Create a virtual stack trace by merging multiple stack traces #13

Merged
merged 7 commits into from Aug 19, 2018

Conversation

creasty
Copy link
Member

@creasty creasty commented Aug 18, 2018

Why

The only scenario where multiple stack traces might be useful is when an error is passed between goroutines (and thus jumps stacks) before getting handled.

pkg/errors#75

However, this data structure makes it difficult to grasp the flow, and in most cases, it's not compatible with error reporting services.

What

Instead of retaining all stack traces, it creates a virtual stack trace.
It retrives stack trace on every wrap just as pkg/errors does and merges that with the underlying stack trace of the given error.

@codecov-io
Copy link

codecov-io commented Aug 18, 2018

Codecov Report

Merging #13 into master will increase coverage by 0.54%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage      95%   95.54%   +0.54%     
==========================================
  Files           5        5              
  Lines         140      157      +17     
==========================================
+ Hits          133      150      +17     
  Misses          4        4              
  Partials        3        3
Impacted Files Coverage Δ
pkgerrors.go 100% <100%> (ø) ⬆️
error.go 96% <100%> (ø) ⬆️
stack.go 90.74% <93.54%> (+6.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 857d428...5811a7e. Read the comment docs.

assert.Equal(t, []string{
"errFunc1",
"errFunc2",
"errFunc3Goroutine.func1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Even when the error is passed between goroutines, a virtual stack trace contains all information.

f.File = trimGOPATH(fpc.Name(), file)
f.Line = int64(line)

if strings.HasPrefix(f.File, "runtime/") {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ignores runtime functions.

@@ -57,3 +57,113 @@ func TestTrimGOPATH(t *testing.T) {

assert.Equal(t, "pkg/sub/file.go", trimGOPATH(funcName, file))
}

func TestMergeStackTraces(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the strategy for merging multiple stacks.

})
}

func TestReduceStackTraces(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

var origin = New("test")

func f1() error {
	return Wrap(origin)
}

func f2() error {
	return Wrap(f1())
}

func f3() chan error {
	c := make(chan error)
	go func() {
		c <- Wrap(f2())
	}()
	return c
}

func main() {
	// init
	// f1
	// f2
	// f3.func1
	// main
	Wrap(<-f3())
}

stack.go Outdated
@@ -20,6 +21,31 @@ type Frame struct {
Line int64
}

func (f Frame) hash() string {
return fmt.Sprintf("%s:%s:%d", f.Func, f.File, f.Line)
Copy link
Member

Choose a reason for hiding this comment

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

I feel that calculating hash string is unnecessary.
I guess you should define equal() method and comparing each attribute of this struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Didn't even need to customize equal at all...

Copy link
Member

@izumin5210 izumin5210 left a comment

Choose a reason for hiding this comment

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

Looks good 😎

@creasty creasty merged commit 5c7a913 into master Aug 19, 2018
@creasty creasty deleted the creasty/merge_stack branch August 19, 2018 02:36
sagikazarmark added a commit to emperror/emperror that referenced this pull request Aug 30, 2018
Currently github.com/pkg/errors.Wrap/Wrapf functions
overwrite already attached stack trace.

From a UX/DX point of view one would like to add
stack trace to an error if none is attached yet.

There are several open issues discussing the problem in the original repo:

pkg/errors#75
pkg/errors#144
pkg/errors#158

At the moment there is no solution provided,
but it looks like the author is willing to make some changes.
Until then this commit adds custom Wrap/Wrapf functions
which implement this desired behaviour.
Other than that, they behave the same way.

There is also a pending PR in the original repo:

pkg/errors#122

It worths mentioning that there are alternative solutions,
like merging stack traces:

srvc/fail#13

After examining the solution it seems that it's
probably too complicated and unnecessary for most cases.
It's also unlikely that the original errors package
will implement this behaviour, so for now
we stick to the most expected behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants