Stack: remove github.com/pkg/errors as a dependency

[skormos]

The .Stack() context method depends on pkg/errors which is now in Maintenance mode.

Recommend removing this dependency, and bringing the stack functionality directly into this project.

[rs]

Would you consider sending a PR?

[skormos]

@rs Possibly, though likely couldn't be for a few weeks. If anyone else is interested in taking a crack at it, would be happy to see it!

[mitar]

I have made new errors package https://gitlab.com/tozd/go/errors some time ago which addresses many issues filled through time at github.com/pkg/errors. I was planing to offer it as v2 of github.com/pkg/errors, but github.com/pkg/errors went into maintenance mode before that and I was unable to contact its maintainer through other means. So my package now lives its own (maintained) life, keeping the spirit of github.com/pkg/errors alive. It internally keeps dependency on github.com/pkg/errors (because sadly one needs to have access to the Stacktrace type to obtain stack traces in Go - one of the issues my package address by exposing stack trace using only standard types) but provides a maintained package to the outside world. Moreover, it keeps up to date with latest Go features like joining multiple errors into one error. And in general it made it clear that there is a difference between wrapping errors (like what stdlib does) and recording error's cause (like what github.com/pkg/errors did with its Wrap function) - closer to Python's cause recording.

And this is where my main issue with zerolog's integration with github.com/pkg/errors is: it just extracts the stack trace, but there are at least two more other things to extract: which were original errors which caused the particular error a, and with new joins of multiple errors, what are those original errors, so one in fact gets a tree of potential other errors (and their messages and stack traces). My package supports JSON marshaling and you can see that there is more than just a stack trace to marshal.

I have until now configured zerolog with support for my package for my own needs. But maybe there is a way that: a) replace github.com/pkg/errors with a dependency on my own package b) get JSON marshaling support also the tree of errors c) when pretty-printing errors with stack traces, also print out the stack trace. @rs: If you would be open to such a change I can make a PR to start in this direction.

[inifares23lab]

Hi, I have made an errors package which

• is a drop-in replacement for standard errors
• has some extra sugar
• implements the stack marshaler

using this github.com/inifares23lab/errors could replace the github.com/pkg/errors
mine seems much less featureful than yours @mitar but I believe it would be a viable option

[mitar]

As I mentioned here, I think zerolog should support only extracting raw []uintptr from the error and then do conversion to JSON itself. From looking at github.com/inifares23lab/errors it seems that it supports only obtaining recorded stack as a string.

implements the stack marshaler

I could not find that.

Moreover, the whole idea of me suggesting the use of gitlab.com/tozd/go/errors is so that zerolog could drop dependency on unmaintained github.com/pkg/errors (with gitlab.com/tozd/go/errors wrapping it and exposing it in a stable and maintained way). I think we all learned from the github.com/pkg/errors that returning a stack trace from an error with a custom type is a bad pattern so let's not repeat that.

My overall proposal would be that error handling of errors in zerolog should:

• first check if the error implements MarshalJSON, if so, use it, that might leave error field to be a nested object and not a string
• check if the error implements StackTrace() []uintptr and extract stack
• then Callers() []uintptr
• then StackTrace() pkgerrors.StackTrace and convert it to []uintptr (backwards compatibility with pkgerrors)
• otherwise call Errros() to obtain the string

And stack trace should then be converted to JSON from []uintptr. And console writer would then parse that JSON and pretty-print it.

This is BTW all already supported through globals ErrorStackMarshaler and ErrorMarshalFunc so one can configure it this way. So the question here is what could zerolog support by default.

There are two issues which zerolog does not answer well currently though:

• What about errors with stack traces which wrap other errors with stack traces (you have an error with a strack trace and a recorded cause error with another stack trace). Then current structure of having two fields for errors (error and stack by default) is not a good structure.
• What about new joined errors, where an error can wrap a slice of other errors.

The last two issues his is why in my zerolog configuration I prefer having error field be an object which contains a string message, and potential additional fields for stack traces, and recursive cause and joined errors.

[rs]

I would favor a solution that removes pkgerrors dep and does not add a new one.

[mitar]

I would favor a solution that removes pkgerrors dep and does not add a new one.

That would be a breaking change. Are you OK with that?

In that case I suggest removing whole stack formatting and everything. I think ErrorStackMarshaler and ErrorMarshalFunc together with #416 is enough that one can customize error and stack handling.

[rs]

I would probably be ok, but I’d like to get more feedback on such a breaking change. I can’t easily gauge how many people depend on this feature.

[inifares23lab]

Hi, I get now what you mean @mitar (I am quite experienced at misunderstanding ;)), also I agree with your view.

It may be out of scope for this but personally I think it could make sense to differentiate two not exclusive types of stacktraces:

1. the stacktrace -- the real one, and focus of this issue, as you mentioned also here
2. the traced errors -- the manual one, as it is often in golang fashion to leave things more in the hands of the developer. this would be implementing a chain of errors like in the standard library and optionally locate them, the "manual stack" would be extracted unwrapping the errors, this is what I tried to achieve in my library, and should be compatible with the standard lib.

That being said at the moment I wouldn't know how to implement point 2. in other than these three ways:

• relying on 3rd party libs (NO)
• implement and maintain an errors lib in zerolog (sounds like extra work for too little value)
• functionality is part of std lib (unlikely in the near future)

So this comment may be a bit unconclusive if not as food for thought.

[mitar]

Nice summary. I agree with your two types. In gitlab.com/tozd/go/errors 2. type is implemented through causer interface (Cause method on the error), similar to wrapper interface, but with different name. This allows one to distinguish wrapping errors for various reasons (changing message, adding stack, adding structured details) from creating a chain of errors as a developer (inspired by Python's exception causes) to understand the progression of the error.

That being said at the moment I wouldn't know how to implement point 2.

My proposal does address it: I think all those errors should handle JSON marshaling themselves and zerolog should not care about that. Zerolog might have some fallback for standard errors which do not implement JSON marshaling, but it could just be calling Error and wrapping it into {"error": ...}. For example, in my case I marshal just the latest wrapped error and do not unwrap them, but I do include in JSON (recursively) causes and joined errors. Each of them have a stack trace and potential additional structured details. That is all that is really needed from zerolog's perspective.

The only tricky question here is then how to pretty-print those JSON marshaled errors, to example show stack trace nicely but that requires just some heuristic in parsing out error's JSON, ideally having standard fields but there are also not so many combinations in there. In any case, zerolog does not really require a dependency on a 3rd party library for that either.

And if you are using error package which does not support JSON marshal, you can do that yourself inside ErrorMarshalFunc.