Add support for stack trace extration of error fields #35

rs · 2018-02-08T06:03:26Z

Proposal for #11.

@sean-: what do you think about this implementation?

I'm not super happy of the API. I'm not sure having Stack and Err separated is great design. What would you think about a new set of error-with-stack methods that would package the error message and the stack together.

For instance:

log.Log().ErrStack("error", err).Msg("")

would generate:

{"error": {"message": "error message", "stack": [{...}, {...}]}}

soulne4ny · 2018-02-12T10:46:59Z

.ErrStack() looks good for me. Definitely adding stack on every error is not a thing that everyone requires.

In the example for have a field name parameter like .ErrStack(name, error), however in the code there is no name parameter.
Here could be two ways for adding stack

error as an object, like in the code "error":{"message":"...", "stack":[...]"}.
or as two separate fields "error":"...", "stack":[...].

The first one makes sense if one wants to add more attributes to an error. While second one allows to keep log processing more simple and prevents from questions what should be inside the "error":{} object — "m" vs "message" for error text, etc...

soulne4ny · 2018-02-12T11:07:18Z

I see a way to configure root logger with an ErrorMarshaller implementation that could be like follows

type ErrorMarshaller func (buf []byte, err error, stack bool) []byte

or

type ErrorMarshaller func (e *Event, err error, stack bool) *Event

or

type ErrorMarshaller interface {
   func (e *Event) Err(err error) *Event
   func (e *Event) ErrStack(err error) *Event  
}

This would it make possible for a user to decide does he/she needs "error":{"message":"...", "stack":[...]"} or "error":"...", "stack":[...]". As well would provide a way to support third-party error wrapping libraries like pkg/errors without adding any dependency. And pkgerrors/stacktrace.go 3786fbf#diff-b36532f7d1ee3bc3766fb82447076e69
would be a good standard implementation for the zerolog's log stack trace object. :)

soulne4ny · 2018-02-16T17:48:32Z

@rs, please, take a look at a commit soulne4ny@863bbc6
There is a variant of ErrorMarshaler from the comment above.
However, a support for log.Context().Stack().Err() needed, as well as better handling for log.Info().AnErr("an_error", err) and log.Info().Errors(manyerrors...).

rs · 2018-02-16T20:53:43Z

@soulne4ny: one downside I see with your approach is that we have to pay for the cost of the interface, even for the default (current) mode with no stack support.

soulne4ny · 2018-02-17T11:34:32Z

Yes, it is the cost. It could be wrapped with if logger.defaultErr {} else { /*custom via interface*/ }.

However, this is only event.Err(). I suppose when there are a lot of log.Error().Err() calls it implies hard times, and the interface indirection for logging is just a minor detail.

On the other hand, this is not just support for error stacks, but for custom wrapped-error types. This requires either tight coupling with the library, or an interface, or a code generation.

Passing an error formatter as an interface or a function pointer, implies a cost for every event.Err() call. While it is a global setting, it introduces one and the very same indirection all though a compiled binary. So for every one who is not OK with default error formatting, will pay the cost.

Is there any other way to remove the indirection besides code generation?
Is the cost really that high comparing to handling errors itself?

soulne4ny · 2018-02-19T14:21:06Z

@rs Would you consider true the hypothesis that error messages should be rather infrequent during regular runs? It yes, then does the cost of indirection matter?

felipeumpierre · 2018-02-19T15:51:48Z

pkgerrors/stacktrace.go

+		StackTrace() errors.StackTrace
+	}
+	var st errors.StackTrace
+	if err, ok := err.(stackTracer); ok {


You can remove this else by inverting the validation logic.

err, ok := err.(stackTracer) if !ok { return nil } return appendJSONStack(make([]byte, 0, 500), err.StackTrace())

Just a different point of view on this block 😃

rs · 2018-02-19T19:30:43Z

We often use Err in access logs for instance, with err as nil most of the time. It is so skipped by the condition on nil which happen after the added code.

soulne4ny · 2018-02-24T00:57:59Z

Solving just stack reporting case, the following notation could be used

log.Error().Err(err).Stack(err).Msg("")

Then event.Stack(err) would be indirect call wrapped with if err != nil {...}. It looks affordable, doesn't it?

rs · 2018-02-24T03:29:54Z

Sounds reasonable.

FarhadF · 2018-05-27T13:00:19Z

Any news on this? really looking forward to stacktraces

rs · 2018-05-29T02:32:59Z

@soulne4ny would you consider rebasing your work and implement the last discussed changes?

As per #9 and to offer a different approach from #11 and #35 this PR introduces custom error serialization with sane defaults without breaking the existing APIs. This is just a first draft and is missing tests. Also, a bit of code duplication which I feel could be reduced but it serves to get the idea across. It provides global error marshalling by exposing a `var ErrorMarshalFunc func(error) interface{}` in zerolog package that by default is a function that returns the passed argument. It should be overriden if you require custom error marshalling. Then in every function that accept error or array of errors `ErrorMarshalFunc` is called on the error and then the result of it is processed like this: - if it implements `LogObjectMarshaler`, serialize it as an object - if it is a string serialize as a string - if it is an error, serialize as a string with the result of `Error()` - else serialize it as an interface The side effect of this change is that the encoders don't need the `AppendError/s` methods anymore, as the errors are serialized directly to other types.

mvrhov · 2018-07-24T15:42:34Z

@soulne4ny ping

istomisgood · 2018-11-28T00:51:01Z

Looking forward to this feature. Press the merge button already!

krak3n · 2019-01-02T16:06:40Z

Any chance we can get this merged? The rebase looks fairly easy, I had a go in a closed PR but ran into CI issues.

rs · 2019-01-02T18:37:11Z

context.go

+	e.Stack()
+}
+
+var sh = callerHook{}


Shouldn't this be var sh = stackTraceHook{}?

rs · 2019-01-02T18:56:38Z

@krak3n: I just tried to rebase, and it's not trivial. The pkgerrors package needs to access the encoder which is now "dynamic" and not easily accessible from outside the zerolog package. The MarshalStack should work on a zerolog.Event instead of trying to handle its own encoding IMHO.

rs · 2019-01-02T19:13:53Z

Here is a rebase with a change of API for the stack marshaler that work with the new encoding system. Please have a look before I merge.

sean-

LGTM, thank you for chasing this to the end and incorporating this request.

sean- · 2019-01-02T20:27:42Z

pkgerrors/stacktrace.go

+		out = append(out, map[string]string{
+			StackSourceFileName:     fmt.Sprintf("%s", frame),
+			StackSourceLineName:     fmt.Sprintf("%d", frame),
+			StackSourceFunctionName: fmt.Sprintf("%n", frame),


Can we change this to be something like:

StackSourceFileName: frame.String(), StackSourceLineName: strconv.FormatInt(frame, 10),

I'm not sure what the %n verb is for StackSourceFunctionName(), but finding something that isn't fmt.Sprintf() would be preferred if there's interest in reducing the overhead of this call.

I think pkg/errors is using the fmt.Formatter interface to expose the different info. I'm not a big fan of this type of magic API, but that's the way it is.

Check the last commit, I removed some allocs by calling the Format method directly. It's not a huge win though:

name old time/op new time/op delta LogStack-8 11.0µs ± 3% 8.9µs ± 1% -18.97% (p=0.008 n=5+5) name old alloc/op new alloc/op delta LogStack-8 4.16kB ± 0% 4.24kB ± 0% +2.12% (p=0.008 n=5+5) name old allocs/op new allocs/op delta LogStack-8 89.0 ± 0% 86.0 ± 0% -3.37% (p=0.008 n=5+5)

krak3n · 2019-01-03T17:15:10Z

thanks guys, looking forward to this getting merged 😄

mvrhov · 2019-01-04T08:35:41Z

🎉

Add support for stack trace extration of error fields

3786fbf

rs force-pushed the stacktrace branch from 0f0fd64 to 3786fbf Compare February 12, 2018 01:55

rs mentioned this pull request Feb 12, 2018

Teach zerolog how to return errors, with and without stack traces #11

Closed

felipeumpierre reviewed Feb 19, 2018

View reviewed changes

rs force-pushed the master branch from ad34871 to 8c1c6a0 Compare February 20, 2018 10:33

vith mentioned this pull request May 9, 2018

Better logging kiwiirc/plugin-fileuploader#20

Merged

DusanKasan mentioned this pull request Jun 21, 2018

Add custom error serialization support and provide sane defaults #78

Merged

rs force-pushed the master branch 4 times, most recently from 1cea9c5 to 8479412 Compare September 17, 2018 17:28

krak3n mentioned this pull request Jan 2, 2019

Add support for stack trace extration of error fields #127

Closed

rs commented Jan 2, 2019

View reviewed changes

context.go Outdated

e.Stack()

}

var sh = callerHook{}

Copy link

Owner Author

rs Jan 2, 2019

Choose a reason for hiding this comment

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

Shouldn't this be var sh = stackTraceHook{}?

rs force-pushed the stacktrace branch from 973d1fe to 186137c Compare January 2, 2019 19:41

Merge branch 'master' into stacktrace

adb19ff

rs force-pushed the stacktrace branch from 186137c to adb19ff Compare January 2, 2019 20:12

sean- approved these changes Jan 2, 2019

View reviewed changes

rs added 3 commits January 2, 2019 13:19

pkgerrors: remove fmt dep

2988c1e

Revert change on go.mod

6d21538

pkgerrors: quick refactoring for better readibility

9f1f25d

rs merged commit aa55558 into master Jan 3, 2019

rs mentioned this pull request Jan 3, 2019

Error wrapping and returned error types... #9

Closed

axw mentioned this pull request Jan 15, 2019

module/apmzerolog: add apmzerolog module elastic/apm-agent-go#428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for stack trace extration of error fields #35

Add support for stack trace extration of error fields #35

rs commented Feb 8, 2018

soulne4ny commented Feb 12, 2018

soulne4ny commented Feb 12, 2018 •

edited

soulne4ny commented Feb 16, 2018

rs commented Feb 16, 2018

soulne4ny commented Feb 17, 2018

soulne4ny commented Feb 19, 2018

felipeumpierre Feb 19, 2018

rs commented Feb 19, 2018

soulne4ny commented Feb 24, 2018 •

edited

rs commented Feb 24, 2018

FarhadF commented May 27, 2018

rs commented May 29, 2018

mvrhov commented Jul 24, 2018

istomisgood commented Nov 28, 2018

krak3n commented Jan 2, 2019

rs Jan 2, 2019

rs commented Jan 2, 2019

rs commented Jan 2, 2019

sean- left a comment

sean- Jan 2, 2019

rs Jan 2, 2019

rs Jan 2, 2019

sean- Jan 3, 2019

krak3n commented Jan 3, 2019

mvrhov commented Jan 4, 2019

Add support for stack trace extration of error fields #35

Add support for stack trace extration of error fields #35

Conversation

rs commented Feb 8, 2018

soulne4ny commented Feb 12, 2018

soulne4ny commented Feb 12, 2018 • edited

soulne4ny commented Feb 16, 2018

rs commented Feb 16, 2018

soulne4ny commented Feb 17, 2018

soulne4ny commented Feb 19, 2018

felipeumpierre Feb 19, 2018

Choose a reason for hiding this comment

rs commented Feb 19, 2018

soulne4ny commented Feb 24, 2018 • edited

rs commented Feb 24, 2018

FarhadF commented May 27, 2018

rs commented May 29, 2018

mvrhov commented Jul 24, 2018

istomisgood commented Nov 28, 2018

krak3n commented Jan 2, 2019

rs Jan 2, 2019

Choose a reason for hiding this comment

rs commented Jan 2, 2019

rs commented Jan 2, 2019

sean- left a comment

Choose a reason for hiding this comment

sean- Jan 2, 2019

Choose a reason for hiding this comment

rs Jan 2, 2019

Choose a reason for hiding this comment

rs Jan 2, 2019

Choose a reason for hiding this comment

sean- Jan 3, 2019

Choose a reason for hiding this comment

krak3n commented Jan 3, 2019

mvrhov commented Jan 4, 2019

soulne4ny commented Feb 12, 2018 •

edited

soulne4ny commented Feb 24, 2018 •

edited