Write custom errors package with stack trace functionality

[bisakhmondal]

To test

package errors_test

import (
	"testing"

	"github.com/thanos-io/thanos/pkg/errors"
)

func TestErrors(t *testing.T) {
	err := errors.New("an error occurred")
	err = errors.Wrap(err, "wraper1")
	t.Logf("error: %+v", err)
}

/home/bisakh/go/go1.17.8/bin/go tool test2json -t /tmp/GoLand/___TestErrors_in_github_com_thanos_io_thanos_pkg_errors.test -test.v -test.paniconexit0 -test.run ^\QTestErrors\E$
=== RUN   TestErrors
    errors_test.go:11: error: wraper1
        > github.com/thanos-io/thanos/pkg/errors_test.TestErrors	/home/bisakh/Desktop/OSS/thanos/pkg/errors/errors_test.go:10
        > testing.tRunner	/home/bisakh/go/go1.17.8/src/testing/testing.go:1259
        > runtime.goexit	/home/bisakh/go/go1.17.8/src/runtime/asm_amd64.s:1581
        an error occurred
        > github.com/thanos-io/thanos/pkg/errors_test.TestErrors	/home/bisakh/Desktop/OSS/thanos/pkg/errors/errors_test.go:9
        > testing.tRunner	/home/bisakh/go/go1.17.8/src/testing/testing.go:1259
        > runtime.goexit	/home/bisakh/go/go1.17.8/src/runtime/asm_amd64.s:1581
--- PASS: TestErrors (0.00s)
PASS

closes #5176

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Verification

TODO:

• write tests
• full migration of existing error packages

[bisakhmondal]

[WIP] How does the output look, @matej-g ?

[saswatamcode]

This looks good overall! Awesome work! 💫
Would love to see some more examples of this in action.

Just some suggestions.

[saswatamcode]

I wonder if we need to mention Chromium's license here too. I believe we do it for some Prometheus code that we use.

[bisakhmondal]

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.
Here's their license https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast/LICENSE

What do you think we should do here?

[saswatamcode]

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. 🙂
WDYT @matej-g?

[bisakhmondal]

I think we should replace all the instances of /pkg/errors in a separate PR instead of unnecessarily bloating this one.

💥➜ thanos git:(pkg/errors) ✗ make lint 2>&1 | wc -l
144

Your views @matej-g?

[bwplotka]

definitely separate PR yes.

[metalmatze]

Do I understand this implementation correctly, that it's mostly a shim for the stdlib errors packages that supports stack traces as part of errors now?

[bisakhmondal]

Do I understand this implementation correctly, that it's mostly a shim for the stdlib errors packages that supports stack traces as part of errors now?

Hi @metalmatze, yes you are right. It extends the functionality errors package by combining

• a stacktrace with error chaining capabilities
• a Wrap and Wrapf functionality
• an Errorf functionality (closest alternative fmt.Errorf)

In simple words, a very minimalistic, simple and readable replacement of github.com/pkg/errors package : )
Thank you!

[bwplotka]

I wonder if we should not start by adding this error lib in https://github.com/efficientgo/tools/tree/main/core to keep util things as ultra small module?

[bisakhmondal]

I wonder if we should not start by adding this error lib in https://github.com/efficientgo/tools/tree/main/core to keep util things as ultra small module?

Hi, thanks for the suggestion @bwplotka and I am sorry for the late reply. I just took a look at the package and especially the merrors pkg. Here are my few concerns.

• One question that's repeatedly coming to my mind is what's the utility of multierrors. Go has been evolved in such a language where errors are handled right after where they have been generated. Errors are errors - no matter how small/big the impact might be in the runtime. It's definitely a bad idea to not to act where err1 has been generated and add the err2 or others. Also the program/function should return the first error it has encountered not the aggregation of all errors (as this is implied, if step1 fails, it is highly likely that step2 and step... will fail).

//  merr := merrors.New(err1)
//  merr.Add(err2, errOrNil3)
//  for _, err := range errs {
//    merr.Add(err)
//  }
//  return merr.Err()

• As per the actual issue, if I am not wrong we are looking for an alternative to github.com/pkg/errors. I am afraid, this package doesn't suffices so. There is no wrap or unwrap method implemented in the multierrors. To use this package we have to again fall back to errors.New of fmt.Errorf package
  (with something like merrors.New(errors.New("the extra context"), err).

  Even if we do so, the Is and As method will be inefficient in this case,
  https://github.com/efficientgo/tools/blob/fe763185946be83b20da626605319733bd7f97cb/core/pkg/merrors/errors.go#L116
  It will iterate through the extra context which we have to pass as an error(due to the design of the package).

• Taking from the issue itself,

There is no stack trace functionality which we use heavily https://github.com/thanos-io/thanos/blob/main/cmd/thanos/main.go#L131

• And finally, the methods exposed from the package will completely destroy backward compatibility. It's not just a simple change in the import path anymore (from 'pkg/errors to 'github.com/thanos-io/thanos/pkg/errors). We have to change each and every instance of the package where err has been used. In the initial days after the migration, there will be certain development toil among the community to adapt to something new.

Hey, I am new to the community and I don't have much context about the discussions and decisions : ) Here I just tried to do some research and share my findings if the efficientgo/tools meets the requirements. I think no for the aforementioned reasons. WDYT?

Thanks : )

[saswatamcode]

Hey @bisakhmondal , thanks for the detailed analysis! :)

To address some of your concerns,

One question that's repeatedly coming to my mind is what's the utility of multierrors. Go has been evolved in such a language where errors are handled right after where they have been generated. Errors are errors - no matter how small/big the impact might be in the runtime. It's definitely a bad idea to not to act where err1 has been generated and add the err2 or others. Also the program/function should return the first error it has encountered not the aggregation of all errors (as this is implied, if step1 fails, it is highly likely that step2 and step... will fail).
// merr := merrors.New(err1)
// merr.Add(err2, errOrNil3)
// for _, err := range errs {
// merr.Add(err)
// }
// return merr.Err()

Hmm, I'd say there is definitely utility in having merrors package. :)
Even in Go, we might not always want to handle an error as soon as it occurs! Sometimes, we may just want to return all errors in one go, and this package is definitely useful for that.

For example, say in our docs CI, we have a tool running called mdox, which checks each markdown file and ensures that they are correctly formatted and have correct links. You run it once via make docs and it outputs all the errors that might exist in all the files.

If we were to just return the very first error encountered, users would have to run it again and again until they have fixed each error, which is definitely not optimal and would be a painful process.

There are use cases for this in Thanos too, for example, the tools rule-check command which checks if all the rule files passed are valid or not. See

thanos/cmd/thanos/tools.go

Line 49 in 19dcc79

func checkRulesFiles(logger log.Logger, patterns *[]string) error {

You'd also find such cases scattered throughout the codebase.

So there are definitely use cases for this! I think there are also other implementations of this like https://github.com/hashicorp/go-multierror.

But a question I have for your point is, why does the existence of merrors matter here? This PR is just a replacement for the errors package that we currently use, not related to the way we handle errors. So the suggestion is to add it to our utils repo efficientgo/tools and then use it from there. We don't really need to change existing merrors, but add a new errors package, right?

As per the actual issue, if I am not wrong we are looking for an alternative to github.com/pkg/errors. I am afraid, this package doesn't suffices so. There is no wrap or unwrap method implemented in the multierrors. To use this package we have to again fall back to errors.New of fmt.Errorf package
(with something like merrors.New(errors.New("the extra context"), err).
Even if we do so, the Is and As method will be inefficient in this case,
https://github.com/efficientgo/tools/blob/fe763185946be83b20da626605319733bd7f97cb/core/pkg/merrors/errors.go#L116
It will iterate through the extra context which we have to pass as an error(due to the design of the package).

Not sure what you mean here. I think @bwplotka's point was to add this as a separate "errors" package in the tools repo. Not as an extension or modification to the existing merrors one.

And finally, the methods exposed from the package will completely destroy backward compatibility. It's not just a simple change in the import path anymore (from 'pkg/errors to 'github.com/thanos-io/thanos/pkg/errors). We have to change each and every instance of the package where err has been used. In the initial days after the migration, there will be certain development toil among the community to adapt to something new.

Hmm. Why would it destroy backward compatibility? We aren't changing/removing any of the existing functionality, right?

If this package is added to efficientgo/tools repo, iiuc, it would still be a change in import path from pkg/errors to github.com/efficientgo/tools/pkg/errors + a version bump in go.mod. There wouldn't be any difference in dev toil when compared with adding a custom errors package that this PR is proposing.

I might be understanding something differently about your points here, so correct me if I'm wrong :).

[bisakhmondal]

But a question I have for your point is, why does the existence of merrors matter here? This PR is just a replacement for the errors package that we currently use, not related to the way we handle errors. So the suggestion is to add it to our utils repo efficientgo/tools and then use it from there. We don't really need to change existing merrors, but add a new errors package, right?

Ahh, Thanks for the clarification @saswatamcode. I misinterpreted @bwplotka's comment thinking we are going to use the error package (merrors) that is currently present in the tools package.

Definitely, there are certain use cases where combining multiple errors makes sense. As I said, I misinterpreted that we are going to make this a default throughout the thanos project which actually doesn't make sense.

As per the actual issue, if I am not wrong we are looking for an alternative to github.com/pkg/errors. I am afraid, this package doesn't suffices so. There is no wrap or unwrap method implemented in the multierrors. To use this package we have to again fall back to errors.New of fmt.Errorf package
(with something like merrors.New(errors.New("the extra context"), err).
Even if we do so, the Is and As method will be inefficient in this case,
https://github.com/efficientgo/tools/blob/fe763185946be83b20da626605319733bd7f97cb/core/pkg/merrors/errors.go#L116
It will iterate through the extra context which we have to pass as an error(due to the design of the package).

Not sure what you mean here. I think @bwplotka's point was to add this as a separate "errors" package in the tools repo. Not as an extension or modification to the existing merrors one.

Yep, I got it now : ) I am totally fine with keeping the error package as a part of thanos project or inside efficientgo/tools. I have no strong opinion here : )

[bwplotka]

Not sure what you mean here. I think @bwplotka's point was to add this as a separate "errors" package in the tools repo. Not as an extension or modification to the existing merrors one.

Correct. I just mentioned utility. (:

[bwplotka]

But we can start small with just implementing it here for now

[bisakhmondal]

But we can start small with just implementing it here for now

Great! Would you mind giving it a quick look and sharing some reviews? I'll write the test suite then : )
Thank you!

[bwplotka]

Great job. I have a few proposals - feel free to challenge non sensible former ideas ppl used to do. Especially with Wrap vs Wrapf is something we could improve 🤔

Let's make sure it's clear and ready.

We also need some at least basic tests. Thanks!

[bwplotka]

definitely separate PR yes.

[bwplotka]

Same here - again - it's our package, we can innovate.

What about only

errors.Newf and errors.Wrapf?

Or even naming it without f but supporting fmt? No harm here, but simper API for everyone. So:

errors.New(format string, args ...interface{}) and errors.Wrap(err, format string, args ...interface{}) would work too

[bisakhmondal]

Yeah, as it's going to be our custom error package, having simpler APIs would be very useful in the long run.
I was just a bit worried about the performance penalty (though it's linear O(N) (ref) )for extra iteration over the format string. So I ran a quick benchmark and the results are equivalent in terms of memory and time.

func benchWrap(errorMsg *string, b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = errors.Wrap(errors.New("random error"), *errorMsg)
	}
}

func benchWrapf(errorMsg *string, b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = errors.Wrap(errors.New("random error"), *errorMsg)
	}
}

// >>> len("something terrible has happened.")
// 32

var (
	len32  = "something terrible has happened."
	len64  = "something terrible has happened.something terrible has happened."
	len128 = "something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened."
	len512 = "something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened.something terrible has happened."
)

func BenchmarkWrap_32(b *testing.B)  { benchWrap(&len32, b) }
func BenchmarkWrapf_32(b *testing.B) { benchWrapf(&len32, b) }

func BenchmarkWrap_64(b *testing.B) { benchWrap(&len64, b) }
func BenchmarkWrapf_64(b *testing.B) {
	benchWrapf(&len64, b)
}

func BenchmarkWrap_512(b *testing.B) {
	benchWrap(&len512, b)
}
func BenchmarkWrapf_512(b *testing.B) {
	benchWrapf(&len512, b)
}

//🔥➜ errors git:(pkg/errors) ✗ go test -bench=. -benchmem
//goos: linux
//goarch: amd64
//pkg: github.com/thanos-io/thanos/pkg/errors
//cpu: Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz
//BenchmarkWrap_32-8        909074              1301 ns/op             256 B/op          2 allocs/op
//BenchmarkWrapf_32-8       942794              1297 ns/op             256 B/op          2 allocs/op
//BenchmarkWrap_64-8        936003              1289 ns/op             256 B/op          2 allocs/op
//BenchmarkWrapf_64-8       941866              1288 ns/op             256 B/op          2 allocs/op
//BenchmarkWrap_512-8       964816              1286 ns/op             256 B/op          2 allocs/op
//BenchmarkWrapf_512-8      883635              1285 ns/op             256 B/op          2 allocs/op

As you have suggested, it's no harm to trim the extra f from this package : )
Updating

[bisakhmondal]

Fun fact:
actually, it's faster and more memory-optimized than github/pkg/errors ^_^

goos: linux
goarch: amd64
pkg: github.com/thanos-io/thanos/pkg/errors
cpu: Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz
BenchmarkWrap_32-8                        776246              1312 ns/op             256 B/op          2 allocs/op
BenchmarkWrap_pkg_errors_32-8             805689              1498 ns/op             640 B/op          7 allocs/op
BenchmarkWrap_64-8                        767688              1324 ns/op             256 B/op          2 allocs/op
BenchmarkWrap_pkg_errors_64-8             831584              1523 ns/op             640 B/op          7 allocs/op
BenchmarkWrap_512-8                       952338              1342 ns/op             256 B/op          2 allocs/op
BenchmarkWrap_pkg_errors_512-8            813520              1508 ns/op             640 B/op          7 allocs/op

[bwplotka]

Nice.

After some thinking, let's keep f word suffix so we are consistent with some lints/tooling that tries to verify formatters. Just removing non f and being able to produce format string without variables is fine. WDYT?

[bwplotka]

Also next time, just in case use

func benchWrapf(errorMsg *string, b *testing.B) {
       b.ResetAllocs()
	for i := 0; i < b.N; i++ {
		_ = errors.Wrap(errors.New("random error"), *errorMsg)
	}
}

[bisakhmondal]

Great! Thanks

[bwplotka]

wonder if alias var Is = errors.Is would not work? (and same for As).

[bisakhmondal]

It would work, updating

[bisakhmondal]

Hey, sorry, I have reverted back to the old one.
Reason: Though using var Is = errors.Is would definitely make things work but users will have a slightly poor IDE experience as the ide will treat those functions as variables (as actually, they are in the modified pkg). So the auto-completion works a bit weird way
see the screenshot

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.
ref: https://github.com/temporalio/sdk-go/blob/fd0d1eb548eb0621a5395581cfe2c418704b007c/client/client.go#L435-L476

I hope you are okay with it. @bwplotka?

[bwplotka]

Makes sense, but worth adding comment why we did this this way 🙃

[bisakhmondal]

Sure : )

[bwplotka]

BTW I know why it did not work for you. We can do type Is = errors.Is I think. Let's merge and iterate (:

[bisakhmondal]

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).

[sthaha]

This implementation has the same issue as pkg/errors i.e. the pc escapes to heap thus the entirety of the 16 uintptr is kept in memory even if only the stack is only 1 call frame deep. I have explained the issue in detail here; based on a real-world scenario.

Please try go build -gcflags='-m -m' ./pkg/errors/stacktrace.go and you would find

pkg/errors/stacktrace.go:21:12: make([]uintptr, stackDepth) escapes to heap:
pkg/errors/stacktrace.go:21:12:   flow: pc = &{storage for make([]uintptr, stackDepth)}:
pkg/errors/stacktrace.go:21:12:     from make([]uintptr, stackDepth) (spill) at pkg/errors/stacktrace.go:21:12
pkg/errors/stacktrace.go:21:12:     from pc := make([]uintptr, stackDepth) (assign) at 

The solution is to allocate the buffer pc on stack like the original implementation and then copy whats needed off it to prevent escaping to heap.

Suggested change

	pc := make([]uintptr, stackDepth)
	const maxDepth = 16
	var pcs [maxDepth]uintptr. // allocate on stack
	n := runtime.Callers(3, pcs[:])
	st := make(stack, n)
	copy(st, pcs[:n])
	return st

Another thought is if we really need to hold on to the entire CallFrame until String() is called? At the expense of some compute, we could calculate Stacktrace string immediately.

[bisakhmondal]

Nice nitpick and great catch @sthaha.

the pc escapes to heap thus the entirety of the 16 uintptr is kept in memory even if only the stack is only 1 call frame deep.

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 stacktrace(pc[:n]) the capacity of PC still remains 16 (allocated memory), right?

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

./stacktrace.go:20:6: cannot inline newStackTrace: function too complex: cost 96 exceeds budget 80
.
.
./stacktrace.go:20:6: cannot inline newStackTrace: function too complex: cost 88 exceeds budget 80

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 return stacktrace(pc[:n:n]) would yield the same effect of the suggestion you proposed.

Thanks a lot. That was really awesome.

Another thought is if we really need to hold on to the entire CallFrame until String() is called? At the expense of some compute, we could calculate Stacktrace string immediately.

I think we shouldn't do it for the following reasons

• Its compute expensive - runtime needs to retrieve caller information
• More memory usage - the String method is meant for human consumption so naturally, this adds a lot of extra information, text etc compared to the 16 element slice of type unitptr. During error changing it would be worse.
• Not every error needs the stacktrace. For specific format verbs like %+v stacktrace gets dumped recursively (only then String gets called).

[sthaha]

Since go1.2 we have 3 index slice capability where the third param can define the capacity of the newly created slice.

💯 .. Thank you, I wasn't aware :)

I think we shouldn't do it for the following reasons ...

Its compute expensive - runtime needs to retrieve caller information

Have you tried a benchmark?

More memory usage - the String method is meant for human consumption so naturally, this adds a lot of extra information, text etc compared to the 16 element slice of type unitptr. During error changing it would be worse.

What I meant was to only hold on to information that you need than have uintptr which now points to the callframe and IIUC, holding onto the callframe will now prevent GC from cleaning the callframe info until the error is garbage collected as well.

It may be worth seeing the inuse allocation if we don't hold on to the callframes.

[bisakhmondal]

I think we shouldn't do it for the following reasons ...

Its compute expensive - runtime needs to retrieve caller information

Have you tried a benchmark?

IIUC, in the new approach that you are suggesting we should call the String immediately at the expense of an extra call which might not be required if the error is not logged with an "%+v" format verb. So we are adhering to extra computation which might not be of any use. So obviously it will be expensive in time against the current approach as the current one only calls string when it's required.

More memory usage - the String method is meant for human consumption so naturally, this adds a lot of extra information, text etc compared to the 16 element slice of type unitptr. During error chaining it would be worse.

What I meant was to only hold on to information that you need than have uintptr which now points to the callframe and IIUC, holding onto the callframe will now prevent GC from cleaning the callframe info until the error is garbage collected as well.

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 fmt.Sprintf("> %s\t%s:%d\n", frame.Func.Name(), frame.File, frame.Line) this to something else memory footprint gets changed. So I think this optimization won't be valid in longer run.

After giving it a thought, I think it's a tradeoff, why?
Memory usage:

• current approach: 16*4 bytes uintptr + unreleased memory from gc (not sure what it stores on that PC address, might be some function related metadata to populate the callFrames. definitely releases all the resources used inside the function)
• proposed approach: large unstructured string + still some unreleased memory (not sure when will the next cycle of go GC will kick off and it's dependent on the config).

I am not sure if we can benchmark this anyhow or not.

the error is garbage collected as well.

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 : )

[bwplotka]

If you see, in Go world err has the comparatively shortest lifespan than anything else.

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.

[bisakhmondal]

It looks like the Lift and DCO CIs are pending forever. Does this take this long, especially the code analysis by lift? Is there a way we can optimize it? I'd love to.

Hi guys, could you please take another look at this PR when you have some time : )

cc @bwplotka

[bwplotka]

This is an amazing job. LGTM. Before merge my only question is really about doing: Newf and Wrapf (A) vs New and Wrap (B).

I like the fact that with no-f (B) version it's cleaner and we show that we don't ever support Wrapper/New without sprintf like formatting. On other hand people got used to things, plus there might be nice tooling which work only if method is f suffix (don't have example - I saw linters for Printf formatting errors).

I think I am leaning towards (A) actually... Thoughts?

[bwplotka]

Again, why nolint?

[bisakhmondal]

Tast project

spellchecker thinks the project name should be Taste instead of the keyword Tast 🙃

[bwplotka]

Let's try to push (or repush) another commit for DCO to unstuck. You can ignore lift - it's not required and rarely working 🙃

[bisakhmondal]

This is an amazing job. LGTM. Before merge my only question is really about doing: Newf and Wrapf (A) vs New and Wrap (B).

I like the fact that with no-f (B) version it's cleaner and we show that we don't ever support Wrapper/New without sprintf like formatting. On other hand people got used to things, plus there might be nice tooling which work only if method is f suffix (don't have example - I saw linters for Printf formatting errors).

I think I am leaning towards (A) actually... Thoughts?

Hi, thanks for the nice feedback and the reviews.
I am fine with both approaches as through benchmarks we have seen that they are of equivalent performance.

Yes, to support approach A, what you have said related to developers' experience and the linters definitely make sense. People are accustomed to having both f and non f APIs and having only non f APIs (approach B) will definitely incur some adaptation cost and multiple visit to the package codes to understand what/how actually it does things.

Okay then, do let me know your final thoughts - we are going with approach A, right? I'll make the necessary changes ASAP : )
Thank you!

[bwplotka]

Yes, I think A is safer. 🤗

[bisakhmondal]

Yes, I think A is safer.

Sure, updating : )

[bwplotka]

I see we left our both non f and f methods at the end? 🤔

[bisakhmondal]

Hey, I thought we are focussing on end-users experience that's why ended up keeping both.
I have changed my mind, let's simplify things and stick to only the f versions 😄

[bisakhmondal]

Mission accomplished : )

[bwplotka]

Nice, LGTM, just minor nits. We have opportunity to simplify this package, but we ended up having both New, Errorf, Wrap and Wrapf. Again, there is an opportunity to simplify this, so reviewers do not need to be a pain and saying "Why you used Errrof("non formattable error") and not New (and similar for Wrap)

[bisakhmondal]

I think this needs rebasing, the docs check will keep failing due to an outdated link in the docs disappointed cc @bisakhmondal

Done. Thanks to you both for the awesome review : )

[bisakhmondal]

Hi Guys, all the CIs (well, as usual except for the lift) have passed. Please approve and merge this. Then I'll proceed with the rest of the refactoring : )

[bisakhmondal]

Ping @bwplotka @matej-g
Let's merge this guys : )

[bwplotka]

Sorry for the massive lag! KubeCon toil. LGTM!

Let's try to use it and improve on the way. Ideally at some point move to efficientgo/core - or better - separate repo for others to import it.

[bwplotka]

Thank you! 🎉 Amazing work!

[bisakhmondal]

Thank you everyone for those awesome reviews :)