Fix: SentryEvent.throwable returns the unwrapped throwable instead of the throwableMechanism

[marandaneto]

📜 Description

Fix: SentryEvent.throwable returns the unwrapped throwable instead of the throwableMechanism

💡 Motivation and Context

#304

💚 How did you test it?

📝 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

[bruno-garcia]

@marandaneto not possible to take the same approach as we did in Java and on the getter unwrap our throwable?

having two fields to get a throwable isn't ideal

[marandaneto]

docs getsentry/sentry-docs#3138

[marandaneto]

@marandaneto not possible to take the same approach as we did in Java and on the getter unwrap our throwable?

having two fields to get a throwable isn't ideal

I did exactly the same approach :) our internal SDK needs access to the mechanism but clients would need the unwrapped version, that's why 2 getters

[bruno-garcia]

@marandaneto not possible to take the same approach as we did in Java and on the getter unwrap our throwable?
having two fields to get a throwable isn't ideal

I did exactly the same approach :) our internal SDK needs access to the mechanism but clients would need the unwrapped version, that's why 2 getters

Shouldn't we reconsider this?
For our internal API we can use an internal API instead of adding two APIs one of which is useless to users?

[marandaneto]

we'd break literally every beforeSend and that was the reason adding the originThrowable at that time, why would we change now?

it's gonna be public anyway because of being used by another packages

[bruno-garcia]

we'd break literally every beforeSend and that was the reason adding the originThrowable at that time, why would we change now?

it's gonna be public anyway because of being used by another packages

What would break, I don't follow?
We should no longer returns our wrapping throwable, but return the original one.

For the internal API we can have another getter (package internal) that we can use to get the wrapper exception

How would we even document:

getThrowable - the throwable (maybe wrapped by some Sentry throwable so we can add more data)
originThrowable - The throwable that was captured (maybe the same as getThorwable, sometimes)

Please use only originThrowable.

[marandaneto]

we'd break literally every beforeSend and that was the reason adding the originThrowable at that time, why would we change now?
it's gonna be public anyway because of being used by another packages

What would break, I don't follow?
We should no longer returns our wrapping throwable, but return the original one.

For the internal API we can have another getter (package internal) that we can use to get the wrapper exception

How would we even document:

getThrowable - the throwable (maybe wrapped by some Sentry throwable so we can add more data)
originThrowable - The throwable that was captured (maybe the same as getThorwable, sometimes)

Please use only originThrowable.

getsentry/sentry-java#1201
during stand up we'll have agreed to use getOriginThrowable, the points you've mentioned right now were already on the table before, we just avoided a breaking change.

[bruno-garcia]

Not sure it's worth increasing the public API surface in the expense of potential more confusion just to avoid a breaking change without at least putting some thought into it.

Could we please consider simply returning the unwrapped Throwable and having an internal API instead for the internal behavior? Should we be using hints like the other SDKs to add the mechanism information instead? Maybe we should do things differently here altogether, I added this mechanism stuff back on Java 1.7, there might be a better way to go about this now.

We can bump a major if that's needed.

Regarding the name: why 'originThrowable"? Is it the "original throwable"?

What about "sourceThrowable" I'm not sure any of these names are much better given we already have getThrowable().

[marandaneto]

Not sure it's worth increasing the public API surface in the expense of potential more confusion just to avoid a breaking change without at least putting some thought into it.

Could we please consider simply returning the unwrapped Throwable and having an internal API instead for the internal behavior? Should we be using hints like the other SDKs to add the mechanism information instead? Maybe we should do things differently here altogether, I added this mechanism stuff back on Java 1.7, there might be a better way to go about this now.

We can bump a major if that's needed.

Regarding the name: why 'originThrowable"? Is it the "original throwable"?

What about "sourceThrowable" I'm not sure any of these names are much better given we already have getThrowable().

let's keep the discussion in a single PR, right now it's spread across a few others.
I've changed this PR to unwrap the Throwable for Event.throwable and added a Event.throwableMechanism that returns the wrapped ThrowableMechanism, as this was not released yet, it was in time to fix it, on Java, we can discuss later as it'd be a breaking change.

[codecov-io]

Codecov Report

Merging #311 (1320bf7) into main (e8fbf20) will increase coverage by 0.75%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
+ Coverage   86.63%   87.39%   +0.75%     
==========================================
  Files          52       47       -5     
  Lines        1751     1531     -220     
==========================================
- Hits         1517     1338     -179     
+ Misses        234      193      -41     

Impacted Files	Coverage Δ
dart/lib/src/protocol/sentry_event.dart	94.89% <75.00%> (-1.85%)	⬇️
dart/lib/src/sentry_client.dart	90.90% <100.00%> (ø)
.../lib/src/navigation/sentry_navigator_observer.dart
flutter/lib/src/sentry_flutter_options.dart
flutter/lib/src/widgets_binding_observer.dart
flutter/lib/src/sentry_flutter.dart
flutter/lib/src/file_system_transport.dart

---

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 e8fbf20...25face6. Read the comment docs.

[marandaneto]

btw looks like JS use hints for this https://docs.sentry.io/platforms/javascript/configuration/filtering/#using-hints similar to https://forum.sentry.io/t/capture-sentry-error/12811

[bruno-garcia]

btw looks like JS use hints for this docs.sentry.io/platforms/javascript/configuration/filtering/#using-hints similar to forum.sentry.io/t/capture-sentry-error/12811

Yes on other SDKs is all about hints.

[bruno-garcia]

Nice! LGTM