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
Write custom errors
package with stack trace functionality
#5239
Changes from all commits
acda158
2efd448
fe02268
b865155
a86b267
75c813a
c3cf039
1bd7e6f
d859d82
ab05bf4
971fc25
f038e2c
be8006b
63d961f
e6eed48
d977de6
d67cc36
52cb5df
5f0c859
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
// Copyright (c) The Thanos Authors. | ||
// Licensed under the Apache License 2.0. | ||
|
||
//nolint | ||
// The idea of writing errors package in thanos is highly motivated from the Tast project of Chromium OS Authors. However, instead of | ||
// copying the package, we end up writing our own simplified logic borrowing some ideas from the errors and github.com/pkg/errors. | ||
// A big thanks to all of them. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we need to mention Chromium's license here too. I believe we do it for some Prometheus code that we use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, thanks for the review. That's what I am also wondering too. To be honest, the current program is not an exact replication (here and there I have got rid of some unnecessary codes, interfaces, used string Builder for efficient formatting, changed the return signature of a few functions) of the Chromium OS Package. What do you think we should do here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine if we just add a line pointing to this license here, as we don't copy but rather adapt code from Chromium. But I don't feel too strongly about this either way. 🙂 |
||
|
||
// Package errors provides basic utilities to manipulate errors with a useful stacktrace. It combines the | ||
// benefits of errors.New and fmt.Errorf world into a single package. | ||
package errors | ||
|
||
import ( | ||
//lint:ignore faillint Custom errors package needs to import standard library errors. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏽 |
||
"errors" | ||
"fmt" | ||
"strings" | ||
) | ||
|
||
// base is the fundamental struct that implements the error interface and the acts as the backbone of this errors package. | ||
type base struct { | ||
// info contains the error message passed through calls like errors.Wrap, errors.New. | ||
info string | ||
// stacktrace stores information about the program counters - i.e. where this error was generated. | ||
stack stacktrace | ||
// err is the actual error which is being wrapped with a stacktrace and message information. | ||
err error | ||
} | ||
|
||
// Error implements the error interface. | ||
func (b *base) Error() string { | ||
if b.err != nil { | ||
return fmt.Sprintf("%s: %s", b.info, b.err.Error()) | ||
} | ||
return b.info | ||
} | ||
|
||
// Unwrap implements the error Unwrap interface. | ||
func (b *base) Unwrap() error { | ||
return b.err | ||
} | ||
|
||
// Format implements the fmt.Formatter interface to support the formatting of an error chain with "%+v" verb. | ||
// Whenever error is printed with %+v format verb, stacktrace info gets dumped to the output. | ||
func (b *base) Format(s fmt.State, verb rune) { | ||
if verb == 'v' && s.Flag('+') { | ||
s.Write([]byte(formatErrorChain(b))) | ||
return | ||
} | ||
|
||
s.Write([]byte(b.Error())) | ||
} | ||
|
||
// Newf formats according to a format specifier and returns a new error with a stacktrace | ||
// with recent call frames. Each call to New returns a distinct error value even if the text is | ||
// identical. An alternative of the errors.New function. | ||
// | ||
// If no args have been passed, it is same as `New` function without formatting. Character like | ||
// '%' still has to be escaped in that scenario. | ||
func Newf(format string, args ...interface{}) error { | ||
return &base{ | ||
info: fmt.Sprintf(format, args...), | ||
stack: newStackTrace(), | ||
err: nil, | ||
} | ||
} | ||
|
||
// Wrapf returns a new error by formatting the error message with the supplied format specifier | ||
// and wrapping another error with a stacktrace containing recent call frames. | ||
// | ||
// If cause is nil, this is the same as fmt.Errorf. If no args have been passed, it is same as `Wrap` | ||
// function without formatting. Character like '%' still has to be escaped in that scenario. | ||
func Wrapf(cause error, format string, args ...interface{}) error { | ||
return &base{ | ||
info: fmt.Sprintf(format, args...), | ||
stack: newStackTrace(), | ||
err: cause, | ||
} | ||
} | ||
|
||
// Cause returns the result of repeatedly calling the Unwrap method on err, if err's | ||
// type implements an Unwrap method. Otherwise, Cause returns the last encountered error. | ||
// The difference between Unwrap and Cause is the first one performs unwrapping of one level | ||
// but Cause returns the last err (whether it's nil or not) where it failed to assert | ||
// the interface containing the Unwrap method. | ||
// This is a replacement of errors.Cause without the causer interface from pkg/errors which | ||
// actually can be sufficed through the errors.Is function. But considering some use cases | ||
// where we need to peel off all the external layers applied through errors.Wrap family, | ||
// it is useful ( where external SDK doesn't use errors.Is internally). | ||
func Cause(err error) error { | ||
for err != nil { | ||
e, ok := err.(interface { | ||
Unwrap() error | ||
}) | ||
if !ok { | ||
return err | ||
} | ||
err = e.Unwrap() | ||
} | ||
return nil | ||
} | ||
|
||
// formatErrorChain formats an error chain. | ||
func formatErrorChain(err error) string { | ||
var buf strings.Builder | ||
for err != nil { | ||
if e, ok := err.(*base); ok { | ||
buf.WriteString(fmt.Sprintf("%s\n%v", e.info, e.stack)) | ||
err = e.err | ||
} else { | ||
buf.WriteString(fmt.Sprintf("%s\n", err.Error())) | ||
err = nil | ||
} | ||
} | ||
return buf.String() | ||
} | ||
|
||
// The functions `Is`, `As` & `Unwrap` provides a thin wrapper around the builtin errors | ||
// package in go. Just for sake of completeness and correct autocompletion behaviors from | ||
// IDEs they have been wrapped using functions instead of using variable to reference them | ||
// as first class functions (eg: var Is = errros.Is ). | ||
|
||
// Is is a wrapper of built-in errors.Is. It reports whether any error in err's | ||
// chain matches target. The chain consists of err itself followed by the sequence | ||
// of errors obtained by repeatedly calling Unwrap. | ||
func Is(err, target error) bool { | ||
return errors.Is(err, target) | ||
} | ||
Comment on lines
+123
to
+128
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wonder if alias There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would work, updating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey, sorry, I have reverted back to the old one. I dug in a little bit to find an alternative approach to tackle this, but it seems popular packages do use the wrapping while exposing their client-side APIs from the internal packages. I hope you are okay with it. @bwplotka? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, but worth adding comment why we did this this way 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW I know why it did not work for you. We can do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi sure 👍 thanks for the suggestion, we can tackle it on the next pr. On second thought I doubt if Go will allow putting a new type over a function definition (for function signature it can be done). |
||
|
||
// As is a wrapper of built-in errors.As. It finds the first error in err's | ||
// chain that matches target, and if one is found, sets target to that error | ||
// value and returns true. Otherwise, it returns false. | ||
func As(err error, target interface{}) bool { | ||
return errors.As(err, target) | ||
} | ||
|
||
// Unwrap is a wrapper of built-in errors.Unwrap. Unwrap returns the result of | ||
// calling the Unwrap method on err, if err's type contains an Unwrap method | ||
// returning error. Otherwise, Unwrap returns nil. | ||
func Unwrap(err error) error { | ||
return errors.Unwrap(err) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
// Copyright (c) The Thanos Authors. | ||
// Licensed under the Apache License 2.0. | ||
|
||
package errors | ||
|
||
import ( | ||
//lint:ignore faillint Custom errors package tests need to import standard library errors. | ||
stderrors "errors" | ||
"fmt" | ||
"regexp" | ||
"strconv" | ||
"testing" | ||
|
||
"github.com/thanos-io/thanos/pkg/testutil" | ||
) | ||
|
||
const msg = "test_error_message" | ||
const wrapper = "test_wrapper" | ||
|
||
func TestNewf(t *testing.T) { | ||
err := Newf(msg) | ||
testutil.Equals(t, err.Error(), msg, "the root error message must match") | ||
|
||
reg := regexp.MustCompile(msg + `[ \n]+> github\.com\/thanos-io\/thanos\/pkg\/errors\.TestNewf .*\/pkg\/errors\/errors_test\.go:\d+`) | ||
testutil.Equals(t, reg.MatchString(fmt.Sprintf("%+v", err)), true, "matching stacktrace in errors.New") | ||
} | ||
|
||
func TestNewfFormatted(t *testing.T) { | ||
fmtMsg := msg + " key=%v" | ||
expectedMsg := msg + " key=value" | ||
|
||
err := Newf(fmtMsg, "value") | ||
testutil.Equals(t, err.Error(), expectedMsg, "the root error message must match") | ||
reg := regexp.MustCompile(expectedMsg + `[ \n]+> github\.com\/thanos-io\/thanos\/pkg\/errors\.TestNewfFormatted .*\/pkg\/errors\/errors_test\.go:\d+`) | ||
testutil.Equals(t, reg.MatchString(fmt.Sprintf("%+v", err)), true, "matching stacktrace in errors.New with format string") | ||
} | ||
|
||
func TestWrapf(t *testing.T) { | ||
err := Newf(msg) | ||
err = Wrapf(err, wrapper) | ||
|
||
expectedMsg := wrapper + ": " + msg | ||
testutil.Equals(t, err.Error(), expectedMsg, "the root error message must match") | ||
|
||
reg := regexp.MustCompile(`test_wrapper[ \n]+> github\.com\/thanos-io\/thanos\/pkg\/errors\.TestWrapf .*\/pkg\/errors\/errors_test\.go:\d+ | ||
[[:ascii:]]+test_error_message[ \n]+> github\.com\/thanos-io\/thanos\/pkg\/errors\.TestWrapf .*\/pkg\/errors\/errors_test\.go:\d+`) | ||
|
||
testutil.Equals(t, reg.MatchString(fmt.Sprintf("%+v", err)), true, "matching stacktrace in errors.Wrap") | ||
} | ||
|
||
func TestUnwrap(t *testing.T) { | ||
// test with base error | ||
err := Newf(msg) | ||
|
||
for i, tc := range []struct { | ||
err error | ||
expected string | ||
isNil bool | ||
}{ | ||
{ | ||
// no wrapping | ||
err: err, | ||
isNil: true, | ||
}, | ||
{ | ||
err: Wrapf(err, wrapper), | ||
expected: "test_error_message", | ||
}, | ||
{ | ||
err: Wrapf(Wrapf(err, wrapper), wrapper), | ||
expected: "test_wrapper: test_error_message", | ||
}, | ||
// check primitives errors | ||
{ | ||
err: stderrors.New("std-error"), | ||
isNil: true, | ||
}, | ||
{ | ||
err: Wrapf(stderrors.New("std-error"), wrapper), | ||
expected: "std-error", | ||
}, | ||
{ | ||
err: nil, | ||
isNil: true, | ||
}, | ||
} { | ||
t.Run("TestCase"+strconv.Itoa(i), func(t *testing.T) { | ||
unwrapped := Unwrap(tc.err) | ||
if tc.isNil { | ||
testutil.Equals(t, unwrapped, nil) | ||
return | ||
} | ||
testutil.Equals(t, unwrapped.Error(), tc.expected, "Unwrap must match expected output") | ||
}) | ||
} | ||
} | ||
|
||
func TestCause(t *testing.T) { | ||
// test with base error that implements interface containing Unwrap method | ||
err := Newf(msg) | ||
|
||
for i, tc := range []struct { | ||
err error | ||
expected string | ||
isNil bool | ||
}{ | ||
{ | ||
// no wrapping | ||
err: err, | ||
isNil: true, | ||
}, | ||
{ | ||
err: Wrapf(err, wrapper), | ||
isNil: true, | ||
}, | ||
{ | ||
err: Wrapf(Wrapf(err, wrapper), wrapper), | ||
isNil: true, | ||
}, | ||
// check primitives errors | ||
{ | ||
err: stderrors.New("std-error"), | ||
expected: "std-error", | ||
}, | ||
{ | ||
err: Wrapf(stderrors.New("std-error"), wrapper), | ||
expected: "std-error", | ||
}, | ||
{ | ||
err: nil, | ||
isNil: true, | ||
}, | ||
} { | ||
t.Run("TestCase"+strconv.Itoa(i), func(t *testing.T) { | ||
cause := Cause(tc.err) | ||
if tc.isNil { | ||
testutil.Equals(t, cause, nil) | ||
return | ||
} | ||
testutil.Equals(t, cause.Error(), tc.expected, "Cause must match expected output") | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,53 @@ | ||||||||||||||||||
// Copyright (c) The Thanos Authors. | ||||||||||||||||||
// Licensed under the Apache License 2.0. | ||||||||||||||||||
|
||||||||||||||||||
package errors | ||||||||||||||||||
|
||||||||||||||||||
import ( | ||||||||||||||||||
"fmt" | ||||||||||||||||||
"runtime" | ||||||||||||||||||
"strings" | ||||||||||||||||||
) | ||||||||||||||||||
|
||||||||||||||||||
// stacktrace holds a snapshot of program counters. | ||||||||||||||||||
type stacktrace []uintptr | ||||||||||||||||||
|
||||||||||||||||||
// newStackTrace captures a stack trace. It skips first 3 frames to record the | ||||||||||||||||||
// snapshot of the stack trace at the origin of a particular error. It tries to | ||||||||||||||||||
// record maximum 16 frames (if available). | ||||||||||||||||||
func newStackTrace() stacktrace { | ||||||||||||||||||
const stackDepth = 16 // record maximum 16 frames (if available). | ||||||||||||||||||
|
||||||||||||||||||
pc := make([]uintptr, stackDepth) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation has the same issue as Please try
The solution is to allocate the buffer
Suggested change
Another thought is if we really need to hold on to the entire CallFrame until There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice nitpick and great catch @sthaha.
Yes, you are right - there is no point in holding an extra chunk of unused heap memory, And I think you are copying to another temporary slice because even though we are returning But this additional copy adds some extra complexity : ) (though it's very minor and negligible as we are dealing with length of 16) From escape analysis the function complexity gets increased from 88 -> 96
Since go1.2 we have 3 index slice capability where the third param can define the capacity of the newly created slice. So just updating the return statement to Thanks a lot. That was really awesome.
I think we shouldn't do it for the following reasons
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💯 .. Thank you, I wasn't aware :)
Have you tried a benchmark?
What I meant was to only hold on to information that you need than have It may be worth seeing the inuse allocation if we don't hold on to the callframes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IIUC, in the new approach that you are suggesting we should call the
Now coming to the memory footprint, IMHO storing unstructured data (the stack trace output string) is generally a bad idea. If we change the output string format from After giving it a thought, I think it's a tradeoff, why?
I am not sure if we can benchmark this anyhow or not.
If you see, in Go world err has the comparatively shortest lifespan than anything else. In a well written program they gets handled immediately by being returned to the caller function or logged into the sink. So I think we are good here. After all it's an internal package and if we found some untoward runtime behaviour we can always do different optimization. Thank you : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not always. Check our fixed issue: #5257 Anyway, I asked @sthaha to remind us about the optimization emperor did - to make sure we are aware. As usually we can iterate over it, we won't do it perfectly over time - the main thing is to get APIs as best as possible - it's hard to change them later. |
||||||||||||||||||
// using skip=3 for not to count the program counter address of | ||||||||||||||||||
bisakhmondal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
// 1. the respective function from errors package (eg. errors.New) | ||||||||||||||||||
// 2. newStacktrace itself | ||||||||||||||||||
// 3. the function used in runtime.Callers | ||||||||||||||||||
n := runtime.Callers(3, pc) | ||||||||||||||||||
|
||||||||||||||||||
// this approach is taken to reduce long term memory footprint (obtained through escape analysis). | ||||||||||||||||||
// We are returning a new slice by re-slicing the pc with the required length and capacity (when the | ||||||||||||||||||
// no of returned callFrames is less that stackDepth). This uses less memory compared to pc[:n] as | ||||||||||||||||||
// the capacity of new slice is inherited from the parent slice if not specified. | ||||||||||||||||||
return stacktrace(pc[:n:n]) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// String implements the fmt.Stringer interface to provide formatted text output. | ||||||||||||||||||
func (s stacktrace) String() string { | ||||||||||||||||||
var buf strings.Builder | ||||||||||||||||||
|
||||||||||||||||||
// CallersFrames takes the slice of Program Counter addresses returned by Callers to | ||||||||||||||||||
// retrieve function/file/line information. | ||||||||||||||||||
cf := runtime.CallersFrames(s) | ||||||||||||||||||
for { | ||||||||||||||||||
// more indicates if the next call will be successful or not. | ||||||||||||||||||
frame, more := cf.Next() | ||||||||||||||||||
// used formatting scheme <`>`space><function name><tab><filepath><:><line><newline> for example: | ||||||||||||||||||
// > testing.tRunner /home/go/go1.17.8/src/testing/testing.go:1259 | ||||||||||||||||||
buf.WriteString(fmt.Sprintf("> %s\t%s:%d\n", frame.Func.Name(), frame.File, frame.Line)) | ||||||||||||||||||
if !more { | ||||||||||||||||||
break | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
return buf.String() | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// Copyright (c) The Thanos Authors. | ||
// Licensed under the Apache License 2.0. | ||
|
||
package errors | ||
|
||
import ( | ||
"strings" | ||
"testing" | ||
) | ||
|
||
func caller() stacktrace { | ||
return newStackTrace() | ||
} | ||
|
||
func TestStacktraceOutput(t *testing.T) { | ||
st := caller() | ||
expectedPhrase := "/pkg/errors/stacktrace_test.go:16" | ||
if !strings.Contains(st.String(), expectedPhrase) { | ||
t.Fatalf("expected %v phrase into the stacktrace, received stacktrace: \n%v", expectedPhrase, st.String()) | ||
} | ||
} | ||
|
||
func TestStacktraceProgramCounterLen(t *testing.T) { | ||
st := caller() | ||
output := st.String() | ||
lines := len(strings.Split(strings.TrimSuffix(output, "\n"), "\n")) | ||
if len(st) != lines { | ||
t.Fatalf("output lines vs program counter size mismatch: program counter size %v, output lines %v", len(st), lines) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why nolint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spellchecker thinks the project name should be
Taste
instead of the keywordTast
🙃