#391; implement error interface #399

psrebniak · 2018-12-24T11:26:49Z

Fixes #391
Had to change apm.captureError behavior a little bit - instead of returning nil, Error with nil ErrorData is returned. Comments are adjusted.

I think Error.sent() name may be adjusted as there are more cases when ErrorData is nil.

Tests for causer and error interfaces added.

elasticmachine · 2018-12-24T11:26:50Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

axw

Thanks for the PR! A couple of queries, otherwise looking good.

axw · 2019-01-02T01:26:01Z

error.go

+}
+
+// Error returns string message for error.
+// if Error or Error.cause is nil, "[EMPTY]" will be used.


I think for log-type errors, the log message should be returned by Error(), while the associated error is returned by Cause() (as you do already.)

Perhaps change NewError and NewErrorLog to set another error message field at construction time, alongside the cause?

Hmm, you mean I should add another error field to both ErrorLogRecord and ErrorData structures?
Not sure when should I use them. Could you explain?

I mean add an error message field to Error, so it looks something like:

type Error struct { *ErrorData cause error err string } func (e *Error) Error() string { return e.err } func (e *Error) Cause() error { return e.cause }

NewErrorLog would set the ErrorLogRecord.Message value, and NewError would just copy across the value from the provided error.

What do you think?

Good idea - I just wonder about scenario when NewErrorLog is called with nil error.
This scenario exists in validation_test.go:236 TestValidateErrorLog and not sure what to do with this.
At the moment I bypassed problem by settings Error.err manually afterwards but I really don't like this solution. Do you think NewErrorLog should be protected from calling with nil error as NewError is? I saw this is optional but seems like it should be non-nill in all runtime scenarios.

At the moment I bypassed problem by settings Error.err manually afterwards but I really don't like this solution.

I would suggest either:

(1) revert newError to having no parameters, and initialise both err and cause afterward

func (t *Tracer) NewError(err error) *Error { ... e := t.newError() ... e.cause = err e.err = err.Error() ... } func (t *Tracer) NewErrorLog(r ErrorLogRecord) *Error { ... e := t.newError() ... e.cause = r.Error e.err = e.log.Message ... }

(2) change newError to accept both the err and cause parameters, and compute the final "message" value before constructing the Error object

func (t *Tracer) NewError(err error) *Error { ... e := t.newError(err, err.Error()) ... } func (t *Tracer) NewErrorLog(r ErrorLogRecord) *Error { if r.Message == "" { r.Message = "[EMPTY]" } e := t.newError(r.Error, r.Message) ... }

Do you think NewErrorLog should be protected from calling with nil error as NewError is? I saw this is optional but seems like it should be non-nill in all runtime scenarios.

No, the error is definitely optional. The use case is for logging frameworks, where you can optionally associate a log record with an error, e.g. in the apmlogrus module:

err, _ := entry.Data[logrus.ErrorKey].(error) errlog := tracer.NewErrorLog(apm.ErrorLogRecord{ Message: entry.Message, Level: entry.Level.String(), Error: err, })

(The logrus.ErrorKey field is optional, hence the comma-ok type assertion.)

Thank you for detailed explanation - I revert changes made to newError function and initialize fields later.

axw · 2019-01-02T01:27:26Z

gocontext.go

@@ -64,13 +64,15 @@ func StartSpanOptions(ctx context.Context, name, spanType string, opts SpanOptio
 // set either from err, or from the caller.
 //
 // If there is no span or transaction in the context, CaptureError returns
-// nil. As a convenience, if the provided error is nil, then CaptureError
-// will also return nil.
+// Error with nil ErrorData field. As a convenience, if the provided error is nil,


Can you please explain the need for this change? It's not clear to me, and I'm reluctant to change this behaviour.

Wanted to avoid situation when returned type is nil pointer to interface - it can be tricky sometimes https://stackoverflow.com/questions/13476349/check-for-nil-and-nil-interface-in-go

Second thing is to keep original error even if error was already sent.

If you like, I can revert that change - was made to provide stable interface, no matter error was sent previously or not.

I think I can see reason in that: we would be saying that if the result is nil if and only if the input error is nil. If the input error is non-nil, but there is no span or transaction in the context, then we'll get a non-nil Error that wraps the input error, only its (Send, SetTransaction, SetSpan) methods will be no-ops.

I'm fine with keeping this, but I'd like a change to the doc comment: it should be stated in terms of functionality, rather than whether the ErrorData field is nil or not. Maybe something like this?

If the provided error is nil, then CaptureError will also return nil; otherwise a non-nil Error will always be returned. If there is no transaction or span in the context, then the returned Error's Send method will have no effect.

codecov-io · 2019-01-03T17:32:30Z

Codecov Report

Merging #399 into master will decrease coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #399     +/-   ##
=========================================
- Coverage   81.15%   80.95%   -0.2%     
=========================================
  Files         104      104             
  Lines        6452     6374     -78     
=========================================
- Hits         5236     5160     -76     
+ Misses        936      934      -2     
  Partials      280      280

Impacted Files	Coverage Δ
error.go	`76.02% <100%> (-5.7%)`	⬇️
gocontext.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5aec9e...30398a1. Read the comment docs.

axw · 2019-01-10T02:06:47Z

@psrebniak do you expect to finish this off soon? I'd love to get it in for the next minor release, which I'm hoping to release in the next week or two.

If you're unsure about the changes I requested, we can just land it as it is (after rebasing of course) and I can look into making minor adjustments later.

psrebniak · 2019-01-10T07:50:35Z

@axw I'll finish changes on Monday, sorry for delay

axw · 2019-01-10T07:51:51Z

@axw I'll finish changes on Monday, sorry for delay

No worries, thank you!

axw · 2019-01-16T01:32:44Z

@psrebniak thank you, looks good. I think this is ready to merge, with a few minor things:

run "make fmt"
add a brief description of the change under "Unreleased" in CHANGELOG.md
rebase on master, squash down to one commit

psrebniak · 2019-01-16T07:58:06Z

@axw done - thanks for your help and patience :)

axw · 2019-01-16T08:16:56Z

@psrebniak not at all, thank you very much for your contribution.

axw requested changes Jan 2, 2019

View reviewed changes

axw added the [zube]: In Review label Jan 9, 2019

axw approved these changes Jan 16, 2019

View reviewed changes

type Error implements error interface

30398a1

psrebniak force-pushed the feature/issue-391-imeplement-error-interface branch from a802c75 to 30398a1 Compare January 16, 2019 07:50

axw merged commit 18846c7 into elastic:master Jan 16, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#391; implement error interface #399

#391; implement error interface #399

psrebniak commented Dec 24, 2018 •

edited by axw

Loading

elasticmachine commented Dec 24, 2018

axw left a comment

axw Jan 2, 2019

psrebniak Jan 4, 2019

axw Jan 5, 2019

psrebniak Jan 14, 2019 •

edited

Loading

axw Jan 15, 2019

psrebniak Jan 15, 2019

axw Jan 2, 2019

psrebniak Jan 4, 2019

axw Jan 4, 2019

codecov-io commented Jan 3, 2019 •

edited

Loading

axw commented Jan 10, 2019

psrebniak commented Jan 10, 2019

axw commented Jan 10, 2019

axw commented Jan 16, 2019

psrebniak commented Jan 16, 2019

axw commented Jan 16, 2019

#391; implement error interface #399

#391; implement error interface #399

Conversation

psrebniak commented Dec 24, 2018 • edited by axw Loading

elasticmachine commented Dec 24, 2018

axw 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

psrebniak Jan 14, 2019 • 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

codecov-io commented Jan 3, 2019 • edited Loading

Codecov Report

axw commented Jan 10, 2019

psrebniak commented Jan 10, 2019

axw commented Jan 10, 2019

axw commented Jan 16, 2019

psrebniak commented Jan 16, 2019

axw commented Jan 16, 2019

psrebniak commented Dec 24, 2018 •

edited by axw

Loading

psrebniak Jan 14, 2019 •

edited

Loading

codecov-io commented Jan 3, 2019 •

edited

Loading