feat: Create errors package #548

AndrewSisley · 2022-06-21T16:53:23Z

Relevant issue(s)

Resolves #545

Description

Creates a defra errors package that can be used by any defra code to create standardized errors and to keep any implementation specifics all in one place.

It does not hook it up to anywhere in our codebase, that can be done in a later PR.

Specify the platform(s) on which this was tested:

Debian Linux

fredcarle · 2022-06-21T16:58:50Z

@AndrewSisley, are you using anything as a reference?

AndrewSisley · 2022-06-21T17:00:44Z

@AndrewSisley, are you using anything as a reference?

The description in the epic, and a vague memory of earlier conversations on this. Plus I'd like to stick to a structured-logging type style - having constant messages makes a lot of things much easier to do (for us and defra consumers)

fredcarle

I meant your reference for the pattern you're using :)

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @AndrewSisley)

AndrewSisley · 2022-06-21T19:09:28Z

I meant your reference for the pattern you're using :)

Ah :) Is roughly the same pattern as the logger package, which in turn is similar to other loggers (and Error[F]/Sprint[F] type funcs)

fredcarle · 2022-06-21T19:21:05Z

I don't think the error package should follow a logger pattern. Have a look at this for reference: https://github.com/upspin/upspin/tree/master/errors

AndrewSisley · 2022-06-21T19:31:34Z

I don't think the error package should follow a logger pattern

Is not a logger pattern. It just structures any attached variables so they don't pollute a core error message. And uses [S] as a func suffix in a similar way to [F] is used in Sprint/Error[F].

Example linked

I would prefer if we would avoid defining our own error type, sticking to the out-of-the-box Go type would be much preferred IMO - should create fewer complications for any consumers of these errors.

AndrewSisley · 2022-06-21T19:45:04Z

errors/errors.go

+
+// ErrorS creates a new Defra error, suffixing any key-value
+// pairs provided, and a stacktrace.
+func ErrorS(message string, keyvals ...KV) error {


Really not sure call stack inclusion should be based on the error - might be better to have that configurable across defra (like log levels)

Changed to global-config approach

fredcarle · 2022-06-21T20:18:14Z

I would prefer if we would avoid defining our own error type, sticking to the out-of-the-box Go type would be much preferred IMO - should create fewer complications for any consumers of these errors.

I disagree here. I think we need our own custom error type. We will still be returning just a normal error interface but to properly track internally what is going I think we need to create our own custom type.

AndrewSisley · 2022-06-21T20:20:00Z

I disagree here. I think we need our own custom error type. We will still be returning just a normal error interface but to properly track internally what is going I think we need to create our own custom type.

We might have a different set of requirements in mind. What functionality do you think our errors should have?

EDIT: My bad, error is an interface, should be a much lower cost in having our own type, but I don't know if I'd want to leak it anywhere...

fredcarle · 2022-06-21T20:24:45Z

We might have a different set of requirements in mind. What functionality do you think our errors should have?

I expect it to keep track of the original error (i.e. system or 3rd party package error), the error we might wrap with (custom message or internal error kind) and the stack trace.

AndrewSisley · 2022-06-21T22:14:42Z

I expect it to keep track of the original error (i.e. system or 3rd party package error), the error we might wrap with (custom message or internal error kind) and the stack trace.

This does that.

fredcarle · 2022-06-22T00:54:32Z

This does that.

It does but it doesn't follow the standard lib error interface implementation and as far as I can tell doesn't seem like it would work with errors.Is and errors.As. I think our errors should still use fmt.ErrorF for wrapping and users should be able to compare our error types with errors.Is. Also we should be able to unwrap the error chain if there is more than one error wrapped. I can see this happening with IPLD errors on queries. We as devs, for example, want to know that it's an IPLD error but the caller probably just wants to know that the CID is not found. I wouldn't want the caller to have to loop through a KV slice to find that out.

orpheuslummis · 2022-06-22T02:57:58Z

idea: A pattern to consider is to have multiple error interfaces that are optimized for the various error domains Defra deals with, as exemplified in https://github.com/carlmjohnson/resperr/blob/master/resperr.go

jsimnz · 2022-06-22T07:01:03Z

I think this error package PR jumped the gun a bit. I think we prob need to ideate the goals/priorities/design of our defra native errors stuff, as there are a bunch of constraints (re standard errors and pkg/errors, err wrapping/unwrapping) we need to be cognisant of, and how the DX effects the rest of the codebase.

I think @fredcarle should take point a bit (obviously with input from everyone) as he has the most (short of me 😃) experience with (idiomatic) Go, and this package has as much to do with Go best practices/patterns as it does with defra internal error goals.

I agree, that it needs to be based on the Go errors "wrapping" semantics support errors.Is and errors.As, along with Stacktrace, and "Human Friendly" errors, similar to what @orpheuslummis posted with resperr.

Happy to take this to a disord thread, or on the original #488 issue.

note: this errors initiative was spawned from a previous PR (can't remember which tbh), and this discord thread.

AndrewSisley · 2022-06-22T12:47:47Z

It does but it doesn't follow the standard lib error interface implementation and as far as I can tell doesn't seem like it would work with errors.Is and errors.As. I think our errors should still use fmt.ErrorF for wrapping and users should be able to compare our error types with errors.Is. Also we should be able to unwrap the error chain if there is more than one error wrapped. I can see this happening with IPLD errors on queries. We as devs, for example, want to know that it's an IPLD error but the caller probably just wants to know that the CID is not found. I wouldn't want the caller to have to loop through a KV slice to find that out.

Again, the current setup allows all of the requirements laid out in your comment.

I wouldn't want the caller to have to loop through a KV slice to find that out.

It does not do this. I think you are misunderstanding things.

fredcarle · 2022-06-22T14:04:20Z

Again, the current setup allows all of the requirements laid out in your comment.

Maybe we need a call where you explain how that is the case because I don't see it. Maybe it's in how you see us use it?

AndrewSisley · 2022-06-22T15:45:58Z

Maybe we need a call where you explain how that is the case because I don't see it. Maybe it's in how you see us use it?

Will probs get this working first - will make chatting about it easier

codecov · 2022-06-29T18:13:48Z

Codecov Report

Merging #548 (af6dc0b) into develop (aa92a70) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #548      +/-   ##
===========================================
+ Coverage    58.07%   58.25%   +0.18%     
===========================================
  Files          144      146       +2     
  Lines        16578    16652      +74     
===========================================
+ Hits          9627     9701      +74     
  Misses        6043     6043              
  Partials       908      908

Impacted Files	Coverage Δ
errors/defraError.go	`100.00% <100.00%> (ø)`
errors/errors.go	`100.00% <100.00%> (ø)`

shahzadlone · 2022-08-22T16:21:58Z

errors/error.go

@@ -0,0 +1,85 @@
+// Copyright 2022 Democratized Data Foundation


nitpick: Rename file to defraError.go ?

sure :)

rename file

shahzadlone · 2022-08-22T16:29:09Z

errors/error.go

+	return e.inner
+}
+
+func (e *defraError) Format(f fmt.State, verb rune) {


suggestion: Perhaps a few words on the possible options for formatting (i.e. %v, %+v, %s and %q) would be useful.

I am unsure if this func should ever be called directly, and as such saw little value in externally visible documentation (the code is brief and more accurate than any possible comment if looking at the implementation). But it is public and so I cannot really argue against a request unless someone with more golang experience in this area chimes in

Probably add func docs here

shahzadlone · 2022-08-22T16:37:39Z

errors/error.go

+}
+
+func (e *defraError) Is(other error) bool {
+	switch otherTyped := other.(type) { //nolint:errorlint


question: does this work when other is itself multiple wrapped errors?

Is only shallow compares other, so other.inner(s) appear irrelevant

suggestion: The stdlib Is "reports whether any error in err's chain matches target." This divergence in how we implement the interface should be documented.

errors/error.go

shahzadlone · 2022-08-22T16:49:44Z

errors/errors.go

+)
+
+const InnerErrorKey string = "Inner"
+const StackKey string = "Stack"


suggestion: move StackKey to the other file (error.go), as it is the only place it is used in.

Will do.

move const (leave public as it is outputted via format and thus public)

errors/errors.go

shahzadlone · 2022-08-22T17:07:21Z

errors/errors_test.go

+	/*
+		The Go test flag `-race` messes with the stacktrace causing this function's frame to be ommited from
+		the stacktrace, as our CI runs with the `-race` flag, these assertions need to be disabled.
+
+		// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
+		// including file, line number, and function reference. An exact string match should not be used as the stacktrace
+		// is machine dependent.
+		assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)
+		// Assert that the error contains this function's name, and a print-out of the generating line.
+		assert.Regexp(t, "TestErrorFmtvWithStacktrace: err := Error\\(errorMessage\\)", result)
+	*/


thought: Why not use go's build tag to turn race off for these assertions or if that's not possible then this file? I think it was something like this:

// +build !race

But never used it before, so not a 100% if it works properly, just an idea.

I searched for a way of doing this and couldn't find one, will try, as it is nice if these assertions run otherwise

Try comment

I quickly looked into this and it seems highly unlikely that -race affects compilation/would not be accessible via build tags. Build tags also can only be applied to the entire file, which would mean breaking up these tests into two files (plus the original shared), which on it's own would probably not be worth it. Leaving as it, but thanks for letting me such a thing exists in go, even if it seems somewhat error prone lol:

To distinguish build constraints from package documentation, a build constraint should be followed by a blank line.

If all it takes is splitting these tests into two files and in return it would let us test these assertions, IMO is still worth it. The final decision is up to your discretion.

It would be 3 files - shared, race, and non-race. But I think we'd need to create our own build flags for the CI to use and -race would still fail if you don't set the custom flags. Leaving as is

In that case agree with omitting.

shahzadlone

LGTM. Would also suggest to wait for atleast one other approval before merging.

AndrewSisley · 2022-08-22T22:02:31Z

LGTM. Would also suggest to wait for atleast one other approval before merging.

Cheers, will leave it for a bit longer, in case anyone else chimes in, but I won't wait until Fred gets back from holiday - easier to just tweak stuff post merge than to leave this PR hanging around.

orpheuslummis · 2022-08-23T12:40:53Z

errors/errors.go

+// New creates a new Defra error, suffixing any key-value
+// pairs provided.
+//
+// A stacktrace will be yielded if formating with a `+`, e.g `fmt.Sprintf("%+v", err)`.


typo: formating -> formatting

orpheuslummis · 2022-08-23T12:44:54Z

errors/defraError.go

+		return e.message == otherTyped.message
+	default:
+		otherString := other.Error()
+		return e.message == otherString || e.Error() == otherString || errors.Is(e.inner, other)


suggestion: because this is extra and different behavior compared above the stdlib - i.e. the stdlib isn't comparing error strings like this - documenting the Is(...) func would be beneficial

What is the standard lib comparing if not the error string? Will look into

maybe doc?

stdlib does essentially compare strings when dealing with errors created by errors.New - leaving as is

orpheuslummis · 2022-08-23T12:50:02Z

errors/errors_test.go

+
+	assert.ErrorIs(t, err2, errors.New(errorMessage1))
+}
+


suggestion: Add a test using a chain of at least 2 defra errors

TestErrorWrap tests this

orpheuslummis · 2022-08-23T12:56:24Z

errors/defraError.go

+	_ fmt.Formatter = (*defraError)(nil)
+)
+
+type defraError struct {


question: why not implement func (e *defraError) As(target any) bool as well?

What behaviour are you looking to gain by doing so?

well essentially to not change too much the existing codebase by using our errors pkg. in my understanding, when we do importing our errors pkg instead of the errors one - everywhere we use errors.Is breaks. we can import both libs but might as well just import one.

everywhere we use errors.Is breaks

This is incorrect as far as I can see, errors.Is should not break, and is even used by the tests for this package - assert.ErrorIs uses/calls errors.Is

just to be a bit more clear: using errors.Is() would only break when import our library as errors because don't implement the function, so that means when we do use our errors in the rest of the codebase we will have to import both stdlib errors and our errors pkg

you mean because of the package name clash? Is easy to solve by aliasing the import. I'm really not looking to replace the errors.Is function in this PR (and am doubtful as to whether we ever should)

orpheuslummis · 2022-08-23T12:58:45Z

errors/errors.go

+}
+
+func withStackTrace(message string, keyvals ...KV) *defraError {
+	stackBuffer := make([]uintptr, MaxStackDepth)


question: which "error generation library" is it from? It still is unclear where the 4 is coming from.

This library. I'll adjust the comment.

Improve wording

orpheuslummis · 2022-08-23T13:00:21Z

errors/errors.go

+	goErrors "github.com/go-errors/errors"
+)
+
+// todo: make this configurable:


thought: in-code todos like this seems to be a natural occurence, and I'm fine with it.

We agreed to only allow todos if linked to a ticket - isn't too bad I think :)

orpheuslummis · 2022-08-23T13:01:07Z

errors/defraError.go

+
+// Format writes the error into the given state.
+//
+// Currently the following runes are supported: `v[+]` (+ also writes out the stacktrace), `s`, `q`


nitpick: end sentence with a period

orpheuslummis · 2022-08-23T13:01:31Z

errors/errors.go

+	}
+}
+
+// New creates a new Defra error, suffixing any key-value


nitpick: can be on one line

Leaving as is - doesn't matter

orpheuslummis · 2022-08-23T13:08:06Z

errors/error.go

+}
+
+func (e *defraError) Is(other error) bool {
+	switch otherTyped := other.(type) { //nolint:errorlint


suggestion: The stdlib Is "reports whether any error in err's chain matches target." This divergence in how we implement the interface should be documented.

orpheuslummis

a few suggestions, question, and nitpicks before approval

orpheuslummis · 2022-08-23T13:21:54Z

errors/defraError.go

+	return builder.String()
+}
+
+func (e *defraError) Is(other error) bool {


suggestion(unsure): additionally implement a func Is(err, target error) bool to facilitate porting from stdlib to use defra errors pkg.

that would potentially allow to avoid import both defra errors and stdlib errors

I am doubtful as to whether that would be worth the time/effort, and I consider it out of scope for this PR. Maybe something to consider in the future though

AndrewSisley · 2022-08-23T14:56:19Z

suggestion: The stdlib Is "reports whether any error in err's chain matches target." This divergence in how we implement the interface should be documented.

I can't find a way to reply to this comment normally so doing it here.

This is the implemented (and tested) behaviour, there should be no divergence :)

orpheuslummis · 2022-08-23T14:58:13Z

IMO there is divergence specifically in how our implementation tests for the error message string equivalent versus the stdlib which doesn't

AndrewSisley · 2022-08-23T15:09:07Z

IMO there is divergence specifically in how our implementation tests for the error message string equivalent versus the stdlib which doesn't

The stlib does implicitly - the 'default' errors.New error is just a string, which will undergo an equality check in errors.Is (including wrapped error.News, errors.Is will unwrap and then do as string equality check)

* Create errors package

AndrewSisley added area/errors Related to the internal management or design of our error handling system action/no-benchmark Skips the action that runs the benchmark. labels Jun 21, 2022

AndrewSisley requested a review from a team June 21, 2022 16:53

AndrewSisley self-assigned this Jun 21, 2022

fredcarle reviewed Jun 21, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch from 75a9a6e to 1554d2a Compare June 21, 2022 19:10

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch from 1554d2a to e1b024f Compare June 21, 2022 19:43

AndrewSisley commented Jun 21, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch 2 times, most recently from 88eac0a to 5633b75 Compare June 21, 2022 20:09

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch 2 times, most recently from d10786e to f08077a Compare June 29, 2022 18:06

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch 2 times, most recently from d09017c to 13eb1fc Compare June 29, 2022 18:20

AndrewSisley changed the title ~~WIP - create errors package~~ Create errors package Aug 22, 2022

AndrewSisley changed the title ~~Create errors package~~ feat: Create errors package Aug 22, 2022

AndrewSisley requested review from orpheuslummis and fredcarle August 22, 2022 16:11

AndrewSisley marked this pull request as ready for review August 22, 2022 16:11

AndrewSisley mentioned this pull request Aug 22, 2022

Replace standard errors with defra errors #750

Closed

shahzadlone requested changes Aug 22, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch 4 times, most recently from d782bad to 9d615e4 Compare August 22, 2022 19:05

AndrewSisley requested a review from shahzadlone August 22, 2022 19:06

shahzadlone approved these changes Aug 22, 2022

View reviewed changes

orpheuslummis reviewed Aug 23, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch 3 times, most recently from 13dfa36 to 70a0333 Compare August 23, 2022 15:17

AndrewSisley requested a review from orpheuslummis August 23, 2022 15:17

AndrewSisley added 2 commits August 26, 2022 13:43

Create errors package

f0facf8

FIXUP - PR requests

af6dc0b

AndrewSisley force-pushed the sisley/refactor/I545-error-package branch from 70a0333 to af6dc0b Compare August 26, 2022 17:43

AndrewSisley merged commit 3727613 into develop Aug 26, 2022

AndrewSisley deleted the sisley/refactor/I545-error-package branch August 26, 2022 18:10

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

feat: Create errors package (sourcenetwork#548)

6563312

* Create errors package

		@@ -0,0 +1,85 @@
		// Copyright 2022 Democratized Data Foundation

feat: Create errors package #548

feat: Create errors package #548

Conversation

AndrewSisley commented Jun 21, 2022 • edited Loading

Relevant issue(s)

Description

fredcarle commented Jun 21, 2022

AndrewSisley commented Jun 21, 2022

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley commented Jun 21, 2022

fredcarle commented Jun 21, 2022

AndrewSisley commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle commented Jun 21, 2022

AndrewSisley commented Jun 21, 2022 • edited Loading

fredcarle commented Jun 21, 2022

AndrewSisley commented Jun 21, 2022

fredcarle commented Jun 22, 2022

orpheuslummis commented Jun 22, 2022

jsimnz commented Jun 22, 2022

AndrewSisley commented Jun 22, 2022 • edited Loading

fredcarle commented Jun 22, 2022

AndrewSisley commented Jun 22, 2022

codecov bot commented Jun 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AndrewSisley Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

AndrewSisley commented Aug 22, 2022

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orpheuslummis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley commented Jun 21, 2022 •

edited

Loading

AndrewSisley commented Jun 21, 2022 •

edited

Loading

AndrewSisley commented Jun 22, 2022 •

edited

Loading

codecov bot commented Jun 29, 2022 •

edited

Loading

AndrewSisley Aug 22, 2022 •

edited

Loading

AndrewSisley Aug 22, 2022 •

edited

Loading

AndrewSisley Aug 22, 2022 •

edited

Loading

AndrewSisley Aug 22, 2022 •

edited

Loading

AndrewSisley Aug 23, 2022 •

edited

Loading

AndrewSisley Aug 23, 2022 •

edited

Loading

AndrewSisley Aug 23, 2022 •

edited

Loading

AndrewSisley Aug 23, 2022 •

edited

Loading

AndrewSisley commented Aug 23, 2022 •

edited

Loading