Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Go 1.20 Unwrap() with a slice of errors #693

Closed
ainar-g opened this issue Aug 8, 2023 · 11 comments
Closed

Support Go 1.20 Unwrap() with a slice of errors #693

ainar-g opened this issue Aug 8, 2023 · 11 comments

Comments

@ainar-g
Copy link

ainar-g commented Aug 8, 2023

Summary

Go 1.20 added another way to unwrap errors, interface { Unwrap() []error }. As far as I can see, as of now Event.SetException doesn't support it.

sentry-go/interfaces.go

Lines 356 to 363 in cda691d

switch previous := err.(type) {
case interface{ Unwrap() error }:
err = previous.Unwrap()
case interface{ Cause() error }:
err = previous.Cause()
default:
err = nil
}

Motivation

Developers are updating to Go 1.20, as Go 1.19 is EOL now, and they will expect sentry-go to support the new interface and include all errors into the event data.

Additional Context

@ainar-g ainar-g changed the title Support Go 1.21 Unwrap() with a slice of errors Support Go 1.20 Unwrap() with a slice of errors Aug 8, 2023
@tonyo
Copy link
Contributor

tonyo commented Aug 16, 2023

Thanks, makes sense, added to the backlog.

@pierrre
Copy link

pierrre commented Sep 25, 2023

What should we do ? Get the first error in the slice ? Or explore the error's tree recursively (but the exceptions are a simple slice)

@ainar-g
Copy link
Author

ainar-g commented Sep 25, 2023

The Python API seems to already have a handler for a similar mechanism called ExceptionGroup:

https://github.com/getsentry/sentry-python/blob/641822dcf3cc90ee0c3e9726d4a5a979d4755c10/sentry_sdk/utils.py#L815-L832

Perhaps the same flatting algorithm could be used?

@pierrre
Copy link

pierrre commented Sep 25, 2023

I'm not very familiar with Python, sorry 😅
Is it adding recursively all errors to the array of exception ?

@ainar-g
Copy link
Author

ainar-g commented Sep 25, 2023

Neither do I really, heh. My point was more that there are similar idioms in other languages, and when the Sentry team starts working on the issue, they should probably follow the same principles as the APIs in those languages do.

And yes, it seems to add them one after the other, at least based on what I can read.

@pierrre
Copy link

pierrre commented Sep 25, 2023

This ?

// SetException appends the unwrapped errors to the event's exception list.
//
// maxErrorDepth is the maximum depth of the error chain we will look
// into while unwrapping the errors.
func (e *Event) SetException(exception error, maxErrorDepth int) {
	err := exception
	if err == nil {
		return
	}

	e.setException(err, maxErrorDepth)

	// Add a trace of the current stack to the most recent error in a chain if
	// it doesn't have a stack trace yet.
	// We only add to the most recent error to avoid duplication and because the
	// current stack is most likely unrelated to errors deeper in the chain.
	if e.Exception[0].Stacktrace == nil {
		e.Exception[0].Stacktrace = NewStacktrace()
	}

	// event.Exception should be sorted such that the most recent error is last.
	reverse(e.Exception)
}

func (e *Event) setException(err error, maxErrorDepth int) {
	for i := 0; i < maxErrorDepth && err != nil; i++ {
		e.Exception = append(e.Exception, Exception{
			Value:      err.Error(),
			Type:       reflect.TypeOf(err).String(),
			Stacktrace: ExtractStacktrace(err),
		})
		switch previous := err.(type) {
		case interface{ Unwrap() error }:
			err = previous.Unwrap()
		case interface{ Unwrap() []error }:
			errs := previous.Unwrap()
			for _, errw := range errs {
				e.setException(errw, maxErrorDepth-i-1)
			}
			err = nil
		case interface{ Cause() error }:
			err = previous.Cause()
		default:
			err = nil
		}
	}
}

@greywolve
Copy link
Contributor

Seems like supporting these kind of error groups is a newish feature, see: getsentry/sentry#37716

@greywolve
Copy link
Contributor

@greywolve
Copy link
Contributor

Here's the sentry-javascript implementation, so this change will be a bit more involved.

@proost
Copy link

proost commented Dec 29, 2023

hello! any progress? Also joinError does not get proper stack trace too.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Dec 29, 2023
@cleptric
Copy link
Member

No progress, still in our backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

7 participants