Feat: Error Cause Extractor

[denrase]

📜 Description

Relates to #1045

Introduce a way for errors to provide a cause to the SDK, so we can extract it.

💡 Motivation and Context

This builds on the discussions in #1045. Instead of returning a list, we just return the cause, making it easier to apply the recursion, as we don't have to deal with lists of lists or injecting extractors in other extractors. It also makes the interface simpler to understand.

So the goal is for users to register the cause extractors for their own application errors, or for any other 3rd party object where they want to extract an error cause. The SDK would then try to flatten errors & then transform with error/stacktrace factories. Event processors can check the throwable list in the event and use the data as needed.

💚 How did you test it?

• Unit tests.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• I updated the docs if needed
• All tests passing
• No breaking changes

🔮 Next steps

This is pretty much a small POC. Would be great if you (@marandaneto & @ueman) could give me some feedback on this approach, before I'll continue with the "real" implementation.

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 22196e6

[ueman]

This is awesome, I like it! I'm already thinking about all the ways to make use of this, haha.

Instead of returning a list, we just return the cause, making it easier to apply the recursion, as we don't have to deal with lists of lists or injecting extractors in other extractors. It also makes the interface simpler to understand.

My thinking was, that there's maybe something similar to C#'s AggregateException, which could be supported, too. I've never seen something like that in Dart, though.

Apart from whether that should be supported or not, you could easily support that by changing current to a stack and letting the CauseExtractor return a list.

[marandaneto]

@denrase sounds like a good option, nice.
Just to double-check, SentryOptions would have a Map<Type, CauseExtractor> where people can add the type and its extractors, so the SDK would be able to do it automatically while manually or automatically capturing errors, right? I don't think users would need to add event processors for that.

[kuhnroyal]

Yea I like this as well :)

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (v7.0.0@16ab023). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff            @@
##             v7.0.0    #1198   +/-   ##
=========================================
  Coverage          ?   88.94%           
=========================================
  Files             ?      125           
  Lines             ?     3827           
  Branches          ?        0           
=========================================
  Hits              ?     3404           
  Misses            ?      423           
  Partials          ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[denrase]

@marandaneto @ueman I have added a check for circularity and also more unit tests. Think we could do a first review of this approach and afterwards document the new classes/method if it's the way we want to go.

[denrase]

@ueman @marandaneto

There's also currently no way of getting the original throwable object of the inner exceptions. That's still only available for the first one.
That makes it impossible to do any processing for inner exceptions in the event processors, because we can't compare based on the type or the name (because of obfuscation).

To get the original throwable we could run the extractor again on the initial throwable. That way we have them in hand. The list of these and the list of sentry exceptions in the event "should" have the same order, so we could match them with the index.

What I'm not sure about is how to "migrate" the dio event processor though, as it does something different.

If we provide an extractor for the DioError, it would just create an additional SentryException with the current approach, but in the processor there is a lot more going on (removing/cleaning stacktraces, adding response/request, etc).

[ueman]

Can't we just add the throwable to the SentryException?

[marandaneto]

Can't we just add the throwable to the SentryException?

yeah, I was thinking about this as well, the throwable does not need to be serialized/deserialized but can be used in order to enrich the event with extra info, when needed.

[denrase]

That would make things easier, whats your take on this @marandaneto ?

[marandaneto]

@ueman @marandaneto

There's also currently no way of getting the original throwable object of the inner exceptions. That's still only available for the first one.
That makes it impossible to do any processing for inner exceptions in the event processors, because we can't compare based on the type or the name (because of obfuscation).

To get the original throwable we could run the extractor again on the initial throwable. That way we have them in hand. The list of these and the list of sentry exceptions in the event "should" have the same order, so we could match them with the index.

What I'm not sure about is how to "migrate" the dio event processor though, as it does something different.

If we provide an extractor for the DioError, it would just create an additional SentryException with the current approach, but in the processor there is a lot more going on (removing/cleaning stacktraces, adding response/request, etc).

Get your point, but the processor does something that the new approach would do, which is converting the inner exception to a sentry exception, also its stack trace, this at least should be possible.
The other thing is that event processors should have access to the inner exceptions in one way or another, so maybe inner exceptions are held in the SentryException as suggested by #1198 (comment)
This is the moment that the Dio event processor would look up into the exception list looking for the Dio error and enriching the event with extra context, I guess that's doable.

[denrase]

@ueman @marandaneto I tried to apply the new approach in the dio event processor in combination with an dio error extractor. WDYT?

[ueman]

I like it

[ueman]

Even though it wouldn't look as nice without this code, I think it would greatly simplify the code if we just remove it.

[denrase]

I have no strong opinion here, opted to keep the previous functionality intact.

[denrase]

I removed it for now. Lets see if @marandaneto also has an opinion here.

[marandaneto]

Let's add docs for public classes and properties, that apply to some other files as well.

[marandaneto]

Better to wrap that with a try-catch, otherwise, we will silently swallow the exception in case the extract has a bug.
Logging the error too.

[marandaneto]

We have to do something similar to the lines below here, L27-28, only adding the dio extractor if already not there, just for defense programming, otherwise calling this extension multiple times would add multiple extractors, maybe causing side effects.

[marandaneto]

@denrase left a few minor comments, please open up a new PR on top of the v7 branch with the fixes, then we can cut a new version, thanks for doing this, I like the approach.
@ueman thanks for helping us out on that.